Serialize ConflictsCache as proto

Convert ConflictKey to AutoValue as well. Rewrite the normalization
logic and add comments to make the intent clearer.

Since boolean serialization doesn't support nulls, change the
ConflictsCache interface to use primitive boolean in the put method.
Inspecting the only caller in ConflictsCacheImpl shows that put can
never be called with null.

Change-Id: I293cffba7609362dabb93e48c1929a05cd0c76fc

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 com.google.gerrit.server.cache.CacheSerializer;
-import java.util.Objects;
+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 {
+@AutoValue
-  private static final long serialVersionUID = 2L;
+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);
+    ObjectId commitCopy = commit.copy();
-  }
+    ObjectId otherCommitCopy = otherCommit.copy();
-
+    if (submitType == SubmitType.FAST_FORWARD_ONLY) {
-  private final ObjectId commit;
+      // The conflict check for FF-only is non-symmetrical, and we need to treat (X, Y) differently
-  private final ObjectId otherCommit;
+      // from (Y, X). Store the commits in the input order.
-  private final SubmitType submitType;
+      return new AutoValue_ConflictKey(commitCopy, otherCommitCopy, submitType, contentMerge);
-  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;
     }
-    this.submitType = submitType;
+    // Otherwise, the check is symmetrical; sort commit/otherCommit before storing, so the actual
-    this.contentMerge = contentMerge;
+    // 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() {
+  @VisibleForTesting
-    return commit;
+  static ConflictKey createWithoutNormalization(
+      AnyObjectId commit, AnyObjectId otherCommit, SubmitType submitType, boolean contentMerge) {
+    return new AutoValue_ConflictKey(commit.copy(), otherCommit.copy(), submitType, contentMerge);
   }
 
-  public ObjectId getOtherCommit() {
+  public abstract ObjectId commit();
-    return otherCommit;
-  }
 
-  public SubmitType getSubmitType() {
+  public abstract ObjectId otherCommit();
-    return submitType;
-  }
 
-  public boolean isContentMerge() {
+  public abstract SubmitType submitType();
-    return contentMerge;
-  }
 
-  @Override
+  public abstract boolean contentMerge();
-  public boolean equals(Object o) {
+
-    if (!(o instanceof ConflictKey)) {
+  public static enum Serializer implements CacheSerializer<ConflictKey> {
-      return false;
+    INSTANCE;
+
+    private static final Converter<String, SubmitType> 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
+    @Override
-  public int hashCode() {
+    public ConflictKey deserialize(byte[] in) {
-    return Objects.hash(commit, otherCommit, submitType, contentMerge);
+      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());
+    }
   }
 }

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);

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);
   }
 

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);
+      Boolean maybeConflicts = args.conflictsCache.getIfPresent(conflictsKey);
-      if (conflicts != null) {
+      if (maybeConflicts != null) {
-        return conflicts;
+        return maybeConflicts;
       }
 
       try (Repository repo = args.repoManager.openRepository(otherChange.getProject());
           CodeReviewRevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
-        conflicts =
+        boolean conflicts =
             !args.submitDryRun.run(
                 str.type,
                 repo,

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",
     ],
 )

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(id1First)
-    assertThat(id2First).isEqualTo(new ConflictKey(id2, id1, FAST_FORWARD_ONLY, true));
+        .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));
+  }
 }

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;
+}