Merge "Inline entity protobufs directly into cache protobufs"

2021-02-09 10:39:26 +00:00
parent df15c40fcc bcd3edf281
commit 45e8f92091
5 changed files with 38 additions and 44 deletions
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -47,7 +47,6 @@ import com.google.gerrit.entities.SubmitRecord;
 import com.google.gerrit.entities.converter.ChangeMessageProtoConverter;
 import com.google.gerrit.entities.converter.PatchSetApprovalProtoConverter;
 import com.google.gerrit.entities.converter.PatchSetProtoConverter;
-import com.google.gerrit.entities.converter.ProtoConverter;
 import com.google.gerrit.json.OutputFormat;
 import com.google.gerrit.proto.Protos;
 import com.google.gerrit.server.AssigneeStatusUpdate;
@@ -65,8 +64,6 @@ import com.google.gerrit.server.cache.serialize.CacheSerializer;
 import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
 import com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord;
 import com.google.gson.Gson;
-import com.google.protobuf.ByteString;
-import com.google.protobuf.MessageLite;
 import java.sql.Timestamp;
 import java.time.Instant;
 import java.util.List;
@@ -455,6 +452,9 @@ public abstract class ChangeNotesState {
    abstract ChangeNotesState build();
  }

+  /**
+   * Convert ChangeNotesState (which is AutoValue based) to byte[] and back, using protocol buffers.
+   */
  enum Serializer implements CacheSerializer<ChangeNotesState> {
    INSTANCE;

@@ -482,13 +482,11 @@ public abstract class ChangeNotesState {
      object.hashtags().forEach(b::addHashtag);
      object
          .patchSets()
-          .forEach(e -> b.addPatchSet(toByteString(e.getValue(), PatchSetProtoConverter.INSTANCE)));
+          .forEach(e -> b.addPatchSet(PatchSetProtoConverter.INSTANCE.toProto(e.getValue())));
      object
          .approvals()
          .forEach(
-              e ->
-                  b.addApproval(
-                      toByteString(e.getValue(), PatchSetApprovalProtoConverter.INSTANCE)));
+              e -> b.addApproval(PatchSetApprovalProtoConverter.INSTANCE.toProto(e.getValue())));

      object.reviewers().asTable().cellSet().forEach(c -> b.addReviewer(toReviewerSetEntry(c)));
      object
@@ -519,7 +517,7 @@ public abstract class ChangeNotesState {
          .forEach(r -> b.addSubmitRecord(GSON.toJson(new StoredSubmitRecord(r))));
      object
          .changeMessages()
-          .forEach(m -> b.addChangeMessage(toByteString(m, ChangeMessageProtoConverter.INSTANCE)));
+          .forEach(m -> b.addChangeMessage(ChangeMessageProtoConverter.INSTANCE.toProto(m)));
      object.publishedComments().values().forEach(c -> b.addPublishedComment(GSON.toJson(c)));
      b.setUpdateCount(object.updateCount());
      if (object.mergedOn() != null) {
@@ -530,12 +528,6 @@ public abstract class ChangeNotesState {
      return Protos.toByteArray(b.build());
    }

-    @VisibleForTesting
-    static <T> ByteString toByteString(T object, ProtoConverter<?, T> converter) {
-      MessageLite message = converter.toProto(object);
-      return Protos.toByteString(message);
-    }
-
    private static ChangeColumnsProto toChangeColumnsProto(ChangeColumns cols) {
      ChangeColumnsProto.Builder b =
          ChangeColumnsProto.newBuilder()
@@ -635,12 +627,12 @@ public abstract class ChangeNotesState {
              .hashtags(proto.getHashtagList())
              .patchSets(
                  proto.getPatchSetList().stream()
-                      .map(bytes -> parseProtoFrom(PatchSetProtoConverter.INSTANCE, bytes))
+                      .map(msg -> PatchSetProtoConverter.INSTANCE.fromProto(msg))
                      .map(ps -> Maps.immutableEntry(ps.id(), ps))
                      .collect(toImmutableList()))
              .approvals(
                  proto.getApprovalList().stream()
-                      .map(bytes -> parseProtoFrom(PatchSetApprovalProtoConverter.INSTANCE, bytes))
+                      .map(msg -> PatchSetApprovalProtoConverter.INSTANCE.fromProto(msg))
                      .map(a -> Maps.immutableEntry(a.patchSetId(), a))
                      .collect(toImmutableList()))
              .reviewers(toReviewerSet(proto.getReviewerList()))
@@ -660,7 +652,7 @@ public abstract class ChangeNotesState {
                      .collect(toImmutableList()))
              .changeMessages(
                  proto.getChangeMessageList().stream()
-                      .map(bytes -> parseProtoFrom(ChangeMessageProtoConverter.INSTANCE, bytes))
+                      .map(msg -> ChangeMessageProtoConverter.INSTANCE.fromProto(msg))
                      .collect(toImmutableList()))
              .publishedComments(
                  proto.getPublishedCommentList().stream()
@@ -671,12 +663,6 @@ public abstract class ChangeNotesState {
      return b.build();
    }

-    private static <P extends MessageLite, T> T parseProtoFrom(
-        ProtoConverter<P, T> converter, ByteString byteString) {
-      P message = Protos.parseUnchecked(converter.getParser(), byteString);
-      return converter.fromProto(message);
-    }
-
    private static ChangeColumns toChangeColumns(Change.Id changeId, ChangeColumnsProto proto) {
      ChangeColumns.Builder b =
          ChangeColumns.builder()
--- a/javatests/com/google/gerrit/server/BUILD
+++ b/javatests/com/google/gerrit/server/BUILD
@@ -49,6 +49,7 @@ junit_tests(
        "//java/com/google/gerrit/lifecycle",
        "//java/com/google/gerrit/mail",
        "//java/com/google/gerrit/metrics",
+        "//java/com/google/gerrit/proto",
        "//java/com/google/gerrit/proto/testing",
        "//java/com/google/gerrit/server",
        "//java/com/google/gerrit/server/account/externalids/testing",
@@ -82,5 +83,6 @@ junit_tests(
        "//lib/truth:truth-java8-extension",
        "//lib/truth:truth-proto-extension",
        "//proto:cache_java_proto",
+        "//proto:entities_java_proto",
    ],
 )
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -17,7 +17,6 @@ package com.google.gerrit.server.notedb;
 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 static com.google.gerrit.server.notedb.ChangeNotesState.Serializer.toByteString;

 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
@@ -40,6 +39,8 @@ import com.google.gerrit.entities.SubmitRequirement;
 import com.google.gerrit.entities.converter.ChangeMessageProtoConverter;
 import com.google.gerrit.entities.converter.PatchSetApprovalProtoConverter;
 import com.google.gerrit.entities.converter.PatchSetProtoConverter;
+import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Protos;
 import com.google.gerrit.server.AssigneeStatusUpdate;
 import com.google.gerrit.server.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
@@ -332,7 +333,8 @@ public class ChangeNotesStateTest {
            .uploader(Account.id(2000))
            .createdOn(cols.createdOn())
            .build();
-    ByteString ps1Bytes = toByteString(ps1, PatchSetProtoConverter.INSTANCE);
+    Entities.PatchSet ps1Proto = PatchSetProtoConverter.INSTANCE.toProto(ps1);
+    ByteString ps1Bytes = Protos.toByteString(ps1Proto);
    assertThat(ps1Bytes.size()).isEqualTo(66);

    PatchSet ps2 =
@@ -342,7 +344,8 @@ public class ChangeNotesStateTest {
            .uploader(Account.id(3000))
            .createdOn(cols.lastUpdatedOn())
            .build();
-    ByteString ps2Bytes = toByteString(ps2, PatchSetProtoConverter.INSTANCE);
+    Entities.PatchSet ps2Proto = PatchSetProtoConverter.INSTANCE.toProto(ps2);
+    ByteString ps2Bytes = Protos.toByteString(ps2Proto);
    assertThat(ps2Bytes.size()).isEqualTo(66);
    assertThat(ps2Bytes).isNotEqualTo(ps1Bytes);

@@ -352,8 +355,8 @@ public class ChangeNotesStateTest {
            .setMetaId(SHA_BYTES)
            .setChangeId(ID.get())
            .setColumns(colsProto)
-            .addPatchSet(ps2Bytes)
-            .addPatchSet(ps1Bytes)
+            .addPatchSet(ps2Proto)
+            .addPatchSet(ps1Proto)
            .build());
  }

@@ -367,8 +370,8 @@ public class ChangeNotesStateTest {
            .value(1)
            .granted(new Timestamp(1212L))
            .build();
-    ByteString a1Bytes = toByteString(a1, PatchSetApprovalProtoConverter.INSTANCE);
-    assertThat(a1Bytes.size()).isEqualTo(43);
+    Entities.PatchSetApproval psa1 = PatchSetApprovalProtoConverter.INSTANCE.toProto(a1);
+    ByteString a1Bytes = Protos.toByteString(psa1);

    PatchSetApproval a2 =
        PatchSetApproval.builder()
@@ -378,7 +381,8 @@ public class ChangeNotesStateTest {
            .value(-1)
            .granted(new Timestamp(3434L))
            .build();
-    ByteString a2Bytes = toByteString(a2, PatchSetApprovalProtoConverter.INSTANCE);
+    Entities.PatchSetApproval psa2 = PatchSetApprovalProtoConverter.INSTANCE.toProto(a2);
+    ByteString a2Bytes = Protos.toByteString(psa2);
    assertThat(a2Bytes.size()).isEqualTo(49);
    assertThat(a2Bytes).isNotEqualTo(a1Bytes);

@@ -390,8 +394,8 @@ public class ChangeNotesStateTest {
            .setMetaId(SHA_BYTES)
            .setChangeId(ID.get())
            .setColumns(colsProto)
-            .addApproval(a2Bytes)
-            .addApproval(a1Bytes)
+            .addApproval(psa2)
+            .addApproval(psa1)
            .build());
  }

@@ -722,7 +726,8 @@ public class ChangeNotesStateTest {
            Account.id(1000),
            new Timestamp(1212L),
            PatchSet.id(ID, 1));
-    ByteString m1Bytes = toByteString(m1, ChangeMessageProtoConverter.INSTANCE);
+    Entities.ChangeMessage m1Proto = ChangeMessageProtoConverter.INSTANCE.toProto(m1);
+    ByteString m1Bytes = Protos.toByteString(m1Proto);
    assertThat(m1Bytes.size()).isEqualTo(35);

    ChangeMessage m2 =
@@ -731,7 +736,8 @@ public class ChangeNotesStateTest {
            Account.id(2000),
            new Timestamp(3434L),
            PatchSet.id(ID, 2));
-    ByteString m2Bytes = toByteString(m2, ChangeMessageProtoConverter.INSTANCE);
+    Entities.ChangeMessage m2Proto = ChangeMessageProtoConverter.INSTANCE.toProto(m2);
+    ByteString m2Bytes = Protos.toByteString(m2Proto);
    assertThat(m2Bytes.size()).isEqualTo(35);
    assertThat(m2Bytes).isNotEqualTo(m1Bytes);

@@ -741,8 +747,8 @@ public class ChangeNotesStateTest {
            .setMetaId(SHA_BYTES)
            .setChangeId(ID.get())
            .setColumns(colsProto)
-            .addChangeMessage(m2Bytes)
-            .addChangeMessage(m1Bytes)
+            .addChangeMessage(m2Proto)
+            .addChangeMessage(m1Proto)
            .build());
  }

--- a/proto/BUILD
+++ b/proto/BUILD
@@ -4,6 +4,7 @@ load("@rules_proto//proto:defs.bzl", "proto_library")
 proto_library(
    name = "cache_proto",
    srcs = ["cache.proto"],
+    deps = [":entities_proto"],
 )

 java_proto_library(
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -18,7 +18,9 @@ package gerrit.cache;

 option java_package = "com.google.gerrit.server.cache.proto";

-// Serialized form of com.google.gerrit.server.change.CHangeKindCacheImpl.Key.
+import "proto/entities.proto";
+
+// Serialized form of com.google.gerrit.server.change.ChangeKindCacheImpl.Key.
 // Next ID: 4
 message ChangeKindKeyProto {
  bytes prior = 1;
@@ -140,11 +142,9 @@ message ChangeNotesStateProto {

  repeated string hashtag = 5;

-  // Raw PatchSet proto as produced by PatchSetProtoConverter.
-  repeated bytes patch_set = 6;
+  repeated devtools.gerritcodereview.PatchSet patch_set = 6;

-  // Raw PatchSetApproval proto as produced by PatchSetApprovalProtoConverter.
-  repeated bytes approval = 7;
+  repeated devtools.gerritcodereview.PatchSetApproval approval = 7;

  // Next ID: 4
  message ReviewerSetEntryProto {
@@ -184,8 +184,7 @@ message ChangeNotesStateProto {
  // com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord.
  repeated string submit_record = 14;

-  // Raw ChangeMessage proto as produced by ChangeMessageProtoConverter.
-  repeated bytes change_message = 15;
+  repeated devtools.gerritcodereview.ChangeMessage change_message = 15;

  // JSON produced from com.google.gerrit.entities.Comment.
  repeated string published_comment = 16;