diff --git a/java/com/google/gerrit/server/query/change/ConflictKey.java b/java/com/google/gerrit/server/query/change/ConflictKey.java index 7af0058c44..9daf886d21 100644 --- a/java/com/google/gerrit/server/query/change/ConflictKey.java +++ b/java/com/google/gerrit/server/query/change/ConflictKey.java @@ -14,68 +14,80 @@ package com.google.gerrit.server.query.change; +import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Converter; +import com.google.common.base.Enums; +import com.google.common.collect.Ordering; import com.google.gerrit.extensions.client.SubmitType; -import java.io.Serializable; -import java.util.Objects; +import com.google.gerrit.server.cache.CacheSerializer; +import com.google.gerrit.server.cache.ProtoCacheSerializers; +import com.google.gerrit.server.cache.ProtoCacheSerializers.ObjectIdConverter; +import com.google.gerrit.server.cache.proto.Cache.ConflictKeyProto; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.ObjectId; -public class ConflictKey implements Serializable { - private static final long serialVersionUID = 2L; - +@AutoValue +public abstract class ConflictKey { public static ConflictKey create( AnyObjectId commit, AnyObjectId otherCommit, SubmitType submitType, boolean contentMerge) { - return new ConflictKey(commit.copy(), otherCommit.copy(), submitType, contentMerge); - } - - private final ObjectId commit; - private final ObjectId otherCommit; - private final SubmitType submitType; - private final boolean contentMerge; - - public ConflictKey( - ObjectId commit, ObjectId otherCommit, SubmitType submitType, boolean contentMerge) { - if (SubmitType.FAST_FORWARD_ONLY.equals(submitType) || commit.compareTo(otherCommit) < 0) { - this.commit = commit; - this.otherCommit = otherCommit; - } else { - this.commit = otherCommit; - this.otherCommit = commit; + ObjectId commitCopy = commit.copy(); + ObjectId otherCommitCopy = otherCommit.copy(); + if (submitType == SubmitType.FAST_FORWARD_ONLY) { + // The conflict check for FF-only is non-symmetrical, and we need to treat (X, Y) differently + // from (Y, X). Store the commits in the input order. + return new AutoValue_ConflictKey(commitCopy, otherCommitCopy, submitType, contentMerge); } - this.submitType = submitType; - this.contentMerge = contentMerge; + // Otherwise, the check is symmetrical; sort commit/otherCommit before storing, so the actual + // key is independent of the order in which they are passed to this method. + return new AutoValue_ConflictKey( + Ordering.natural().min(commitCopy, otherCommitCopy), + Ordering.natural().max(commitCopy, otherCommitCopy), + submitType, + contentMerge); } - public ObjectId getCommit() { - return commit; + @VisibleForTesting + static ConflictKey createWithoutNormalization( + AnyObjectId commit, AnyObjectId otherCommit, SubmitType submitType, boolean contentMerge) { + return new AutoValue_ConflictKey(commit.copy(), otherCommit.copy(), submitType, contentMerge); } - public ObjectId getOtherCommit() { - return otherCommit; - } + public abstract ObjectId commit(); - public SubmitType getSubmitType() { - return submitType; - } + public abstract ObjectId otherCommit(); - public boolean isContentMerge() { - return contentMerge; - } + public abstract SubmitType submitType(); - @Override - public boolean equals(Object o) { - if (!(o instanceof ConflictKey)) { - return false; + public abstract boolean contentMerge(); + + public static enum Serializer implements CacheSerializer { + INSTANCE; + + private static final Converter SUBMIT_TYPE_CONVERTER = + Enums.stringConverter(SubmitType.class); + + @Override + public byte[] serialize(ConflictKey object) { + ObjectIdConverter idConverter = ObjectIdConverter.create(); + return ProtoCacheSerializers.toByteArray( + ConflictKeyProto.newBuilder() + .setCommit(idConverter.toByteString(object.commit())) + .setOtherCommit(idConverter.toByteString(object.otherCommit())) + .setSubmitType(SUBMIT_TYPE_CONVERTER.reverse().convert(object.submitType())) + .setContentMerge(object.contentMerge()) + .build()); } - ConflictKey other = (ConflictKey) o; - return commit.equals(other.commit) - && otherCommit.equals(other.otherCommit) - && submitType.equals(other.submitType) - && contentMerge == other.contentMerge; - } - @Override - public int hashCode() { - return Objects.hash(commit, otherCommit, submitType, contentMerge); + @Override + public ConflictKey deserialize(byte[] in) { + ConflictKeyProto proto = ProtoCacheSerializers.parseUnchecked(ConflictKeyProto.parser(), in); + ObjectIdConverter idConverter = ObjectIdConverter.create(); + return create( + idConverter.fromByteString(proto.getCommit()), + idConverter.fromByteString(proto.getOtherCommit()), + SUBMIT_TYPE_CONVERTER.convert(proto.getSubmitType()), + proto.getContentMerge()); + } } } diff --git a/java/com/google/gerrit/server/query/change/ConflictsCache.java b/java/com/google/gerrit/server/query/change/ConflictsCache.java index e8b2fef89e..c7ee79b9db 100644 --- a/java/com/google/gerrit/server/query/change/ConflictsCache.java +++ b/java/com/google/gerrit/server/query/change/ConflictsCache.java @@ -18,7 +18,7 @@ import com.google.gerrit.common.Nullable; public interface ConflictsCache { - void put(ConflictKey key, Boolean value); + void put(ConflictKey key, boolean value); @Nullable Boolean getIfPresent(ConflictKey key); diff --git a/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java b/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java index 11856770f4..0b8c5ee3c9 100644 --- a/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java +++ b/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.query.change; import com.google.common.cache.Cache; +import com.google.gerrit.server.cache.BooleanCacheSerializer; import com.google.gerrit.server.cache.CacheModule; import com.google.inject.Inject; import com.google.inject.Module; @@ -29,7 +30,11 @@ public class ConflictsCacheImpl implements ConflictsCache { return new CacheModule() { @Override protected void configure() { - persist(NAME, ConflictKey.class, Boolean.class).maximumWeight(37400); + persist(NAME, ConflictKey.class, Boolean.class) + .version(1) + .keySerializer(ConflictKey.Serializer.INSTANCE) + .valueSerializer(BooleanCacheSerializer.INSTANCE) + .maximumWeight(37400); bind(ConflictsCache.class).to(ConflictsCacheImpl.class); } }; @@ -43,7 +48,7 @@ public class ConflictsCacheImpl implements ConflictsCache { } @Override - public void put(ConflictKey key, Boolean value) { + public void put(ConflictKey key, boolean value) { conflictsCache.put(key, value); } diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java index f870951b68..7dc7a0b080 100644 --- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java +++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java @@ -115,19 +115,19 @@ public class ConflictsPredicate { ObjectId other = ObjectId.fromString(object.currentPatchSet().getRevision().get()); ConflictKey conflictsKey = - new ConflictKey( + ConflictKey.create( changeDataCache.getTestAgainst(), other, str.type, projectState.is(BooleanProjectConfig.USE_CONTENT_MERGE)); - Boolean conflicts = args.conflictsCache.getIfPresent(conflictsKey); - if (conflicts != null) { - return conflicts; + Boolean maybeConflicts = args.conflictsCache.getIfPresent(conflictsKey); + if (maybeConflicts != null) { + return maybeConflicts; } try (Repository repo = args.repoManager.openRepository(otherChange.getProject()); CodeReviewRevWalk rw = CodeReviewCommit.newRevWalk(repo)) { - conflicts = + boolean conflicts = !args.submitDryRun.run( str.type, repo, diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD index 6d55cd76ec..09e3243e8b 100644 --- a/javatests/com/google/gerrit/server/query/change/BUILD +++ b/javatests/com/google/gerrit/server/query/change/BUILD @@ -62,10 +62,13 @@ junit_tests( "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/reviewdb:server", "//java/com/google/gerrit/server", + "//java/com/google/gerrit/server/cache/testing", "//java/com/google/gerrit/testing:gerrit-test-util", "//lib:guava", "//lib:gwtorm", "//lib/jgit/org.eclipse.jgit:jgit", "//lib/truth", + "//lib/truth:truth-proto-extension", + "//proto:cache_java_proto", ], ) diff --git a/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java b/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java index 2cfb652cce..b87bbf73c0 100644 --- a/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java +++ b/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java @@ -15,9 +15,15 @@ package com.google.gerrit.server.query.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; import static com.google.gerrit.extensions.client.SubmitType.FAST_FORWARD_ONLY; import static com.google.gerrit.extensions.client.SubmitType.MERGE_IF_NECESSARY; +import static com.google.gerrit.server.cache.testing.CacheSerializerTestUtil.bytes; +import static com.google.gerrit.server.cache.testing.SerializedClassSubject.assertThatSerializedClass; +import com.google.common.collect.ImmutableMap; +import com.google.gerrit.extensions.client.SubmitType; +import com.google.gerrit.server.cache.proto.Cache.ConflictKeyProto; import org.eclipse.jgit.lib.ObjectId; import org.junit.Test; @@ -29,8 +35,10 @@ public class ConflictKeyTest { ConflictKey id1First = ConflictKey.create(id1, id2, FAST_FORWARD_ONLY, true); ConflictKey id2First = ConflictKey.create(id2, id1, FAST_FORWARD_ONLY, true); - assertThat(id1First).isEqualTo(new ConflictKey(id1, id2, FAST_FORWARD_ONLY, true)); - assertThat(id2First).isEqualTo(new ConflictKey(id2, id1, FAST_FORWARD_ONLY, true)); + assertThat(id1First) + .isEqualTo(ConflictKey.createWithoutNormalization(id1, id2, FAST_FORWARD_ONLY, true)); + assertThat(id2First) + .isEqualTo(ConflictKey.createWithoutNormalization(id2, id1, FAST_FORWARD_ONLY, true)); assertThat(id1First).isNotEqualTo(id2First); } @@ -40,9 +48,51 @@ public class ConflictKeyTest { ObjectId id2 = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); ConflictKey id1First = ConflictKey.create(id1, id2, MERGE_IF_NECESSARY, true); ConflictKey id2First = ConflictKey.create(id2, id1, MERGE_IF_NECESSARY, true); - ConflictKey expected = new ConflictKey(id1, id2, MERGE_IF_NECESSARY, true); + ConflictKey expected = + ConflictKey.createWithoutNormalization(id1, id2, MERGE_IF_NECESSARY, true); assertThat(id1First).isEqualTo(expected); assertThat(id2First).isEqualTo(expected); } + + @Test + public void serializer() throws Exception { + ConflictKey key = + ConflictKey.create( + ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee"), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), + SubmitType.MERGE_IF_NECESSARY, + false); + byte[] serialized = ConflictKey.Serializer.INSTANCE.serialize(key); + assertThat(ConflictKeyProto.parseFrom(serialized)) + .isEqualTo( + ConflictKeyProto.newBuilder() + .setCommit( + bytes( + 0xba, 0xdc, 0x0f, 0xee, 0xba, 0xdc, 0x0f, 0xee, 0xba, 0xdc, 0x0f, 0xee, + 0xba, 0xdc, 0x0f, 0xee, 0xba, 0xdc, 0x0f, 0xee)) + .setOtherCommit( + bytes( + 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, + 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef)) + .setSubmitType("MERGE_IF_NECESSARY") + .setContentMerge(false) + .build()); + assertThat(ConflictKey.Serializer.INSTANCE.deserialize(serialized)).isEqualTo(key); + } + + /** + * See {@link com.google.gerrit.server.cache.testing.SerializedClassSubject} for background and + * what to do if this test fails. + */ + @Test + public void methods() throws Exception { + assertThatSerializedClass(ConflictKey.class) + .hasAutoValueMethods( + ImmutableMap.of( + "commit", ObjectId.class, + "otherCommit", ObjectId.class, + "submitType", SubmitType.class, + "contentMerge", boolean.class)); + } } diff --git a/proto/cache.proto b/proto/cache.proto index 7e2e75a801..a826f8cc6e 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -184,3 +184,12 @@ message ChangeNotesStateProto { int64 read_only_until = 17; bool has_read_only_until = 18; } + + +// Serialized form of com.google.gerrit.server.query.change.ConflictKey +message ConflictKeyProto { + bytes commit = 1; + bytes other_commit = 2; + string submit_type = 3; + bool content_merge = 4; +}