Convert RevId proto to be based on ObjectId

See Iff5644e2 context on removing RevId usages.

Change-Id: I635facafaa97abc3936a1ed3911c03f4b76e156d

java/com/google/gerrit/reviewdb/converter/ObjectIdProtoConverter.java

@@ -0,0 +1,50 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.reviewdb.converter;
+
+import com.google.gerrit.proto.Entities;
+import com.google.protobuf.Parser;
+import org.eclipse.jgit.lib.ObjectId;
+
+/**
+ * Proto converter for {@code ObjectId}s.
+ *
+ * <p>This converter uses the hex representation of object IDs embedded in a wrapper proto type,
+ * rather than a more parsimonious implementation (e.g. a raw byte array), for two reasons:
+ *
+ * <ul>
+ *   <li>Hex strings are easier to read and work with when reading and writing protos in text
+ *       formats, for example in test failure messages, or when using command-line tools.
+ *   <li>This maintains backwards wire compatibility with a pre-NoteDb implementation.
+ * </ul>
+ */
+public enum ObjectIdProtoConverter implements ProtoConverter<Entities.ObjectId, ObjectId> {
+  INSTANCE;
+
+  @Override
+  public Entities.ObjectId toProto(ObjectId objectId) {
+    return Entities.ObjectId.newBuilder().setName(objectId.name()).build();
+  }
+
+  @Override
+  public ObjectId fromProto(Entities.ObjectId proto) {
+    return ObjectId.fromString(proto.getName());
+  }
+
+  @Override
+  public Parser<Entities.ObjectId> getParser() {
+    return Entities.ObjectId.parser();
+  }
+}

java/com/google/gerrit/reviewdb/converter/PatchSetProtoConverter.java

@@ -21,13 +21,15 @@ import com.google.gerrit.reviewdb.client.RevId;
 import com.google.protobuf.Parser;
 import java.sql.Timestamp;
 import java.util.List;
+import org.eclipse.jgit.lib.ObjectId;
 
 public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet, PatchSet> {
   INSTANCE;
 
   private final ProtoConverter<Entities.PatchSet_Id, PatchSet.Id> patchSetIdConverter =
       PatchSetIdProtoConverter.INSTANCE;
-  private final ProtoConverter<Entities.RevId, RevId> revIdConverter = RevIdProtoConverter.INSTANCE;
+  private final ProtoConverter<Entities.ObjectId, ObjectId> objectIdConverter =
+      ObjectIdProtoConverter.INSTANCE;
   private final ProtoConverter<Entities.Account_Id, Account.Id> accountIdConverter =
       AccountIdProtoConverter.INSTANCE;
 
@@ -37,7 +39,7 @@ public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet,
         Entities.PatchSet.newBuilder().setId(patchSetIdConverter.toProto(patchSet.getId()));
     RevId revision = patchSet.getRevision();
     if (revision != null) {
-      builder.setRevision(revIdConverter.toProto(revision));
+      builder.setCommitId(objectIdConverter.toProto(ObjectId.fromString(revision.get())));
     }
     Account.Id uploader = patchSet.getUploader();
     if (uploader != null) {
@@ -65,8 +67,8 @@ public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet,
   @Override
   public PatchSet fromProto(Entities.PatchSet proto) {
     PatchSet patchSet = new PatchSet(patchSetIdConverter.fromProto(proto.getId()));
-    if (proto.hasRevision()) {
-      patchSet.setRevision(revIdConverter.fromProto(proto.getRevision()));
+    if (proto.hasCommitId()) {
+      patchSet.setRevision(new RevId(objectIdConverter.fromProto(proto.getCommitId()).name()));
     }
     if (proto.hasUploaderAccountId()) {
       patchSet.setUploader(accountIdConverter.fromProto(proto.getUploaderAccountId()));

java/com/google/gerrit/reviewdb/converter/RevIdProtoConverter.java

@@ -1,38 +0,0 @@
-// Copyright (C) 2018 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.reviewdb.converter;
-
-import com.google.gerrit.proto.Entities;
-import com.google.gerrit.reviewdb.client.RevId;
-import com.google.protobuf.Parser;
-
-public enum RevIdProtoConverter implements ProtoConverter<Entities.RevId, RevId> {
-  INSTANCE;
-
-  @Override
-  public Entities.RevId toProto(RevId revId) {
-    return Entities.RevId.newBuilder().setId(revId.get()).build();
-  }
-
-  @Override
-  public RevId fromProto(Entities.RevId proto) {
-    return new RevId(proto.getId());
-  }
-
-  @Override
-  public Parser<Entities.RevId> getParser() {
-    return Entities.RevId.parser();
-  }
-}

javatests/com/google/gerrit/reviewdb/converter/BUILD

@@ -8,6 +8,7 @@ junit_tests(
         "//java/com/google/gerrit/reviewdb:server",
         "//lib:guava",
         "//lib:protobuf",
+        "//lib/jgit/org.eclipse.jgit:jgit",
         "//lib/truth",
         "//lib/truth:truth-proto-extension",
         "//proto:entities_java_proto",

javatests/com/google/gerrit/reviewdb/converter/ObjectIdProtoConverterTest.java

@@ -1,4 +1,4 @@
-// Copyright (C) 2018 The Android Open Source Project
+// Copyright (C) 2019 The Android Open Source Project
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -15,45 +15,47 @@
 package com.google.gerrit.reviewdb.converter;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
 import static com.google.gerrit.proto.testing.SerializedClassSubject.assertThatSerializedClass;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.gerrit.proto.Entities;
 import com.google.gerrit.proto.testing.SerializedClassSubject;
-import com.google.gerrit.reviewdb.client.RevId;
 import com.google.protobuf.Parser;
+import org.eclipse.jgit.lib.ObjectId;
 import org.junit.Test;
 
-public class RevIdProtoConverterTest {
-  private final RevIdProtoConverter revIdProtoConverter = RevIdProtoConverter.INSTANCE;
+public class ObjectIdProtoConverterTest {
+  private final ObjectIdProtoConverter objectIdProtoConverter = ObjectIdProtoConverter.INSTANCE;
 
   @Test
   public void allValuesConvertedToProto() {
-    RevId revId = new RevId("9903402f303249e");
+    ObjectId objectId = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
 
-    Entities.RevId proto = revIdProtoConverter.toProto(revId);
+    Entities.ObjectId proto = objectIdProtoConverter.toProto(objectId);
 
-    Entities.RevId expectedProto = Entities.RevId.newBuilder().setId("9903402f303249e").build();
+    Entities.ObjectId expectedProto =
+        Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef").build();
     assertThat(proto).isEqualTo(expectedProto);
   }
 
   @Test
   public void allValuesConvertedToProtoAndBackAgain() {
-    RevId revId = new RevId("ff3934a320bb");
+    ObjectId objectId = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
 
-    RevId convertedRevId = revIdProtoConverter.fromProto(revIdProtoConverter.toProto(revId));
+    ObjectId convertedObjectId =
+        objectIdProtoConverter.fromProto(objectIdProtoConverter.toProto(objectId));
 
-    assertThat(convertedRevId).isEqualTo(revId);
+    assertThat(convertedObjectId).isEqualTo(objectId);
   }
 
   @Test
   public void protoCanBeParsedFromBytes() throws Exception {
-    Entities.RevId proto = Entities.RevId.newBuilder().setId("9903402f303249e").build();
+    Entities.ObjectId proto =
+        Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef").build();
     byte[] bytes = proto.toByteArray();
 
-    Parser<Entities.RevId> parser = revIdProtoConverter.getParser();
-    Entities.RevId parsedProto = parser.parseFrom(bytes);
+    Parser<Entities.ObjectId> parser = objectIdProtoConverter.getParser();
+    Entities.ObjectId parsedProto = parser.parseFrom(bytes);
 
     assertThat(parsedProto).isEqualTo(proto);
   }
@@ -61,6 +63,13 @@ public class RevIdProtoConverterTest {
   /** See {@link SerializedClassSubject} for background and what to do if this test fails. */
   @Test
   public void fieldsExistAsExpected() {
-    assertThatSerializedClass(RevId.class).hasFields(ImmutableMap.of("id", String.class));
+    assertThatSerializedClass(ObjectId.class)
+        .hasFields(
+            ImmutableMap.of(
+                "w1", int.class,
+                "w2", int.class,
+                "w3", int.class,
+                "w4", int.class,
+                "w5", int.class));
   }
 }

javatests/com/google/gerrit/reviewdb/converter/PatchSetProtoConverterTest.java

@@ -37,7 +37,7 @@ public class PatchSetProtoConverterTest {
   @Test
   public void allValuesConvertedToProto() {
     PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
-    patchSet.setRevision(new RevId("aabbccddeeff"));
+    patchSet.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
     patchSet.setUploader(Account.id(452));
     patchSet.setCreatedOn(new Timestamp(930349320L));
     patchSet.setGroups(ImmutableList.of("group1, group2"));
@@ -52,7 +52,8 @@ public class PatchSetProtoConverterTest {
                 Entities.PatchSet_Id.newBuilder()
                     .setChangeId(Entities.Change_Id.newBuilder().setId(103))
                     .setId(73))
-            .setRevision(Entities.RevId.newBuilder().setId("aabbccddeeff"))
+            .setCommitId(
+                Entities.ObjectId.newBuilder().setName("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"))
             .setUploaderAccountId(Entities.Account_Id.newBuilder().setId(452))
             .setCreatedOn(930349320L)
             .setGroups("group1, group2")
@@ -81,7 +82,7 @@ public class PatchSetProtoConverterTest {
   @Test
   public void allValuesConvertedToProtoAndBackAgain() {
     PatchSet patchSet = new PatchSet(PatchSet.id(Change.id(103), 73));
-    patchSet.setRevision(new RevId("aabbccddeeff"));
+    patchSet.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
     patchSet.setUploader(Account.id(452));
     patchSet.setCreatedOn(new Timestamp(930349320L));
     patchSet.setGroups(ImmutableList.of("group1, group2"));

proto/entities.proto

@@ -91,7 +91,7 @@ message PatchSet_Id {
 // Next ID: 10
 message PatchSet {
   required PatchSet_Id id = 1;
-  optional RevId revision = 2;
+  optional ObjectId commitId = 2;
   optional Account_Id uploader_account_id = 3;
   optional fixed64 created_on = 4;
   optional string groups = 6;
@@ -151,8 +151,9 @@ message Branch_NameKey {
   optional string branch = 2;
 }
 
-// Serialized form of com.google.gerrit.reviewdb.client.RevId.
+// Serialized form of org.eclipse.jgit.lib.ObjectId.
 // Next ID: 2
-message RevId {
-  optional string id = 1;
+message ObjectId {
+  // Hex string representation of the ID.
+  optional string name = 1;
 }