From bab45861b7b9d62c3a98fdad76973550f0d0f9d9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 1 May 2018 12:31:48 -0400 Subject: [PATCH] Optionally persist ChangeNotesCache Loading ChangeNotes into the ChangeNotesCache should generally be pretty fast when the underlying git repository storage is fast, but there are some situations where that is not the case: * The repo hasn't been GC'ed in a while, so may contain a lot of loose objects. * On googlesource.com using the JGit DFS backend, when GC has happened recently and the DFS block cache is cold. These problems are particularly noticeable on a cold server start. As an optional optimization, allow persisting the ChangeNotesCache. For installations where cache loading latency hasn't proven to be a problem, it may not be worth the disk space, but we think it will make a difference for googlesource.com. Writing the necessary protos was a bit of work, but actually the marginal cost of tweaking fields should be relatively low, and any change should cause a small test to fail, so we should be able to detect any changes as they arise. I explicitly chose to reuse existing serialization mechanisms where possible (ProtobufCodecs, JSON), to limit the size of this change. This is just cache data, so it's not like it has to be particularly pretty or long-lasting. This change is not intended to indicate we are giving up on optimizing loading ChangeNotes from storage, but is more of a bandaid for fixing performance problems in production today. Change-Id: I1ffe15fe56b6822b7f9af55635b063793e66d6fd --- Documentation/config-gerrit.txt | 1 + WORKSPACE | 12 + .../reviewdb/server/ReviewDbCodecs.java | 4 + .../server/cache/ProtoCacheSerializers.java | 25 + .../cache/testing/SerializedClassSubject.java | 22 + .../server/index/change/ChangeField.java | 6 +- .../server/notedb/ChangeNotesCache.java | 50 +- .../server/notedb/ChangeNotesState.java | 307 +++++- javatests/com/google/gerrit/server/BUILD | 1 + .../server/notedb/ChangeNotesCacheTest.java | 60 ++ .../server/notedb/ChangeNotesStateTest.java | 957 ++++++++++++++++++ lib/truth/BUILD | 28 + proto/cache.proto | 139 +++ 13 files changed, 1604 insertions(+), 8 deletions(-) create mode 100644 javatests/com/google/gerrit/server/notedb/ChangeNotesCacheTest.java create mode 100644 javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 174043d76a..7ed0e17b76 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -773,6 +773,7 @@ performed once every 24 hours. + Default is 128 MiB per cache, except: + +* `"change_notes"`: disk storage is disabled by default * `"diff_summary"`: default is `1g` (1 GiB of disk space) + diff --git a/WORKSPACE b/WORKSPACE index 15d86516b9..94138e4085 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -711,6 +711,18 @@ maven_jar( sha1 = "636e49d675bc28e0b3ae0edd077d6acbbb159166", ) +maven_jar( + name = "truth-liteproto-extension", + artifact = "com.google.truth.extensions:truth-liteproto-extension:" + TRUTH_VERS, + sha1 = "21210ac07e5cfbe83f04733f806224a6c0ae4d2d", +) + +maven_jar( + name = "truth-proto-extension", + artifact = "com.google.truth.extensions:truth-proto-extension:" + TRUTH_VERS, + sha1 = "5a2b504143a5fec2b6be8bce292b3b7577a81789", +) + # When bumping the easymock version number, make sure to also move powermock to a compatible version maven_jar( name = "easymock", diff --git a/java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java b/java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java index 631e7f51d2..29584647f9 100644 --- a/java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java +++ b/java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java @@ -15,6 +15,7 @@ package com.google.gerrit.reviewdb.server; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gwtorm.protobuf.CodecFactory; @@ -27,6 +28,9 @@ public class ReviewDbCodecs { public static final ProtobufCodec CHANGE_CODEC = CodecFactory.encoder(Change.class); + public static final ProtobufCodec MESSAGE_CODEC = + CodecFactory.encoder(ChangeMessage.class); + public static final ProtobufCodec PATCH_SET_CODEC = CodecFactory.encoder(PatchSet.class); diff --git a/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java b/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java index a8e7e5f279..9fe6b833a2 100644 --- a/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java +++ b/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.cache; +import com.google.gwtorm.protobuf.ProtobufCodec; +import com.google.protobuf.ByteString; import com.google.protobuf.CodedOutputStream; import com.google.protobuf.MessageLite; import java.io.IOException; @@ -43,5 +45,28 @@ public class ProtoCacheSerializers { } } + /** + * Serializes an object to a {@link ByteString} using a protobuf codec. + * + *

Guarantees deterministic serialization and thus is suitable for use in persistent caches. + * Should be used in preference to {@link ProtobufCodec#encodeToByteString(Object)}, which is not + * guaranteed deterministic. + * + * @param object the object to serialize. + * @param codec codec for serializing. + * @return a {@code ByteString} with the message contents. + */ + public static ByteString toByteString(T object, ProtobufCodec codec) { + try (ByteString.Output bout = ByteString.newOutput()) { + CodedOutputStream cout = CodedOutputStream.newInstance(bout); + cout.useDeterministicSerialization(); + codec.encode(object, cout); + cout.flush(); + return bout.toByteString(); + } catch (IOException e) { + throw new IllegalStateException("exception writing to ByteString", e); + } + } + private ProtoCacheSerializers() {} } diff --git a/java/com/google/gerrit/server/cache/testing/SerializedClassSubject.java b/java/com/google/gerrit/server/cache/testing/SerializedClassSubject.java index 78900cbc51..19c5b67b11 100644 --- a/java/com/google/gerrit/server/cache/testing/SerializedClassSubject.java +++ b/java/com/google/gerrit/server/cache/testing/SerializedClassSubject.java @@ -22,8 +22,10 @@ import static com.google.common.truth.Truth.assertWithMessage; import com.google.common.truth.FailureMetadata; import com.google.common.truth.Subject; import java.lang.reflect.Field; +import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.lang.reflect.Type; +import java.util.Arrays; import java.util.Map; import org.apache.commons.lang3.reflect.FieldUtils; @@ -62,6 +64,13 @@ public class SerializedClassSubject extends Subject expectedMethods) { + // Would be nice if we could check clazz is an @AutoValue, but the retention is not RUNTIME. + isAbstract(); + assertThat( + Arrays.stream(actual().getDeclaredMethods()) + .filter(m -> !Modifier.isStatic(m.getModifiers())) + .filter(m -> Modifier.isAbstract(m.getModifiers())) + .filter(m -> m.getParameters().length == 0) + .collect(toImmutableMap(Method::getName, Method::getGenericReturnType))) + .named("no-argument abstract methods on %s", actual().getName()) + .isEqualTo(expectedMethods); + } } diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java index 5db347ece1..82253f28e2 100644 --- a/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/java/com/google/gerrit/server/index/change/ChangeField.java @@ -643,7 +643,7 @@ public class ChangeField { *

Stored fields need to use a stable format over a long period; this type insulates the index * from implementation changes in SubmitRecord itself. */ - static class StoredSubmitRecord { + public static class StoredSubmitRecord { static class StoredLabel { String label; SubmitRecord.Label.Status status; @@ -661,7 +661,7 @@ public class ChangeField { List requirements; String errorMessage; - StoredSubmitRecord(SubmitRecord rec) { + public StoredSubmitRecord(SubmitRecord rec) { this.status = rec.status; this.errorMessage = rec.errorMessage; if (rec.labels != null) { @@ -686,7 +686,7 @@ public class ChangeField { } } - private SubmitRecord toSubmitRecord() { + public SubmitRecord toSubmitRecord() { SubmitRecord rec = new SubmitRecord(); rec.status = status; rec.errorMessage = errorMessage; diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java index 853ed69cb6..d1c28c419c 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java @@ -25,12 +25,16 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.cache.CacheModule; +import com.google.gerrit.server.cache.CacheSerializer; +import com.google.gerrit.server.cache.ProtoCacheSerializers; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesKeyProto; import com.google.gerrit.server.notedb.AbstractChangeNotes.Args; import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk; import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.Singleton; import com.google.inject.name.Named; +import com.google.protobuf.ByteString; import java.io.IOException; import java.util.List; import java.util.Map; @@ -38,6 +42,7 @@ import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @Singleton @@ -49,20 +54,59 @@ public class ChangeNotesCache { @Override protected void configure() { bind(ChangeNotesCache.class); - cache(CACHE_NAME, Key.class, ChangeNotesState.class) + persist(CACHE_NAME, Key.class, ChangeNotesState.class) .weigher(Weigher.class) - .maximumWeight(10 << 20); + .maximumWeight(10 << 20) + .diskLimit(-1) + .version(1) + .keySerializer(Key.Serializer.INSTANCE) + .valueSerializer(ChangeNotesState.Serializer.INSTANCE); } }; } @AutoValue public abstract static class Key { + static Key create(Project.NameKey project, Change.Id changeId, ObjectId id) { + return new AutoValue_ChangeNotesCache_Key(project, changeId, id.copy()); + } + abstract Project.NameKey project(); abstract Change.Id changeId(); abstract ObjectId id(); + + @VisibleForTesting + static enum Serializer implements CacheSerializer { + INSTANCE; + + @Override + public byte[] serialize(Key object) { + byte[] buf = new byte[Constants.OBJECT_ID_LENGTH]; + object.id().copyRawTo(buf, 0); + return ProtoCacheSerializers.toByteArray( + ChangeNotesKeyProto.newBuilder() + .setProject(object.project().get()) + .setChangeId(object.changeId().get()) + .setId(ByteString.copyFrom(buf)) + .build()); + } + + @Override + public Key deserialize(byte[] in) { + ChangeNotesKeyProto proto; + try { + proto = ChangeNotesKeyProto.parseFrom(in); + } catch (IOException e) { + throw new IllegalArgumentException("Failed to deserialize " + Key.class.getName()); + } + return Key.create( + new Project.NameKey(proto.getProject()), + new Change.Id(proto.getChangeId()), + ObjectId.fromRaw(proto.getId().toByteArray())); + } + } } public static class Weigher implements com.google.common.cache.Weigher { @@ -330,7 +374,7 @@ public class ChangeNotesCache { Value get(Project.NameKey project, Change.Id changeId, ObjectId metaId, ChangeNotesRevWalk rw) throws IOException { try { - Key key = new AutoValue_ChangeNotesCache_Key(project, changeId, metaId.copy()); + Key key = Key.create(project, changeId, metaId); Loader loader = new Loader(key, rw); ChangeNotesState s = cache.get(key, loader); return new AutoValue_ChangeNotesCache_Value(s, loader.revisionNoteMap); diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java index ab7e580065..1b09494531 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java @@ -14,15 +14,29 @@ package com.google.gerrit.server.notedb; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap; +import static com.google.common.collect.ImmutableSet.toImmutableSet; +import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.APPROVAL_CODEC; +import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.MESSAGE_CODEC; +import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.PATCH_SET_CODEC; +import static com.google.gerrit.server.cache.ProtoCacheSerializers.toByteString; 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.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableTable; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Maps; +import com.google.common.collect.Table; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Account; @@ -34,15 +48,28 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.OutputFormat; import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.ReviewerStatusUpdate; +import com.google.gerrit.server.cache.CacheSerializer; +import com.google.gerrit.server.cache.ProtoCacheSerializers; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ChangeColumnsProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerByEmailSetEntryProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerSetEntryProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerStatusUpdateProto; +import com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord; +import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; +import com.google.gson.Gson; +import com.google.protobuf.ByteString; import java.io.IOException; import java.sql.Timestamp; import java.util.List; import java.util.Map; import java.util.Set; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; /** @@ -106,7 +133,7 @@ public abstract class ChangeNotesState { .metaId(metaId) .changeId(changeId) .columns( - new AutoValue_ChangeNotesState_ChangeColumns.Builder() + ChangeColumns.builder() .changeKey(changeKey) .createdOn(createdOn) .lastUpdatedOn(lastUpdatedOn) @@ -151,6 +178,10 @@ public abstract class ChangeNotesState { */ @AutoValue abstract static class ChangeColumns { + static Builder builder() { + return new AutoValue_ChangeNotesState_ChangeColumns.Builder(); + } + abstract Change.Key changeKey(); abstract Timestamp createdOn(); @@ -192,6 +223,8 @@ public abstract class ChangeNotesState { @Nullable abstract Change.Id revertOf(); + abstract Builder toBuilder(); + @AutoValue.Builder abstract static class Builder { abstract Builder changeKey(Change.Key changeKey); @@ -369,7 +402,7 @@ public abstract class ChangeNotesState { abstract Builder pastAssignees(Set pastAssignees); - abstract Builder hashtags(Set hashtags); + abstract Builder hashtags(Iterable hashtags); abstract Builder patchSets(Iterable> patchSets); @@ -397,4 +430,274 @@ public abstract class ChangeNotesState { abstract ChangeNotesState build(); } + + static enum Serializer implements CacheSerializer { + INSTANCE; + + @VisibleForTesting static final Gson GSON = OutputFormat.JSON_COMPACT.newGson(); + + private static final Converter STATUS_CONVERTER = + Enums.stringConverter(Change.Status.class); + private static final Converter REVIEWER_STATE_CONVERTER = + Enums.stringConverter(ReviewerStateInternal.class); + + @Override + public byte[] serialize(ChangeNotesState object) { + checkArgument(object.metaId() != null, "meta ID is required in: %s", object); + checkArgument(object.columns() != null, "ChangeColumns is required in: %s", object); + ChangeNotesStateProto.Builder b = ChangeNotesStateProto.newBuilder(); + + byte[] idBuf = new byte[Constants.OBJECT_ID_LENGTH]; + object.metaId().copyRawTo(idBuf, 0); + b.setMetaId(ByteString.copyFrom(idBuf)) + .setChangeId(object.changeId().get()) + .setColumns(toChangeColumnsProto(object.columns())); + + object.pastAssignees().forEach(a -> b.addPastAssignee(a.get())); + object.hashtags().forEach(b::addHashtag); + object.patchSets().forEach(e -> b.addPatchSet(toByteString(e.getValue(), PATCH_SET_CODEC))); + object.approvals().forEach(e -> b.addApproval(toByteString(e.getValue(), APPROVAL_CODEC))); + + object.reviewers().asTable().cellSet().forEach(c -> b.addReviewer(toReviewerSetEntry(c))); + object + .reviewersByEmail() + .asTable() + .cellSet() + .forEach(c -> b.addReviewerByEmail(toReviewerByEmailSetEntry(c))); + object + .pendingReviewers() + .asTable() + .cellSet() + .forEach(c -> b.addPendingReviewer(toReviewerSetEntry(c))); + object + .pendingReviewersByEmail() + .asTable() + .cellSet() + .forEach(c -> b.addPendingReviewerByEmail(toReviewerByEmailSetEntry(c))); + + object.allPastReviewers().forEach(a -> b.addPastReviewer(a.get())); + object.reviewerUpdates().forEach(u -> b.addReviewerUpdate(toReviewerStatusUpdateProto(u))); + object + .submitRecords() + .forEach(r -> b.addSubmitRecord(GSON.toJson(new StoredSubmitRecord(r)))); + object.changeMessages().forEach(m -> b.addChangeMessage(toByteString(m, MESSAGE_CODEC))); + object.publishedComments().values().forEach(c -> b.addPublishedComment(GSON.toJson(c))); + + if (object.readOnlyUntil() != null) { + b.setReadOnlyUntil(object.readOnlyUntil().getTime()).setHasReadOnlyUntil(true); + } + + return ProtoCacheSerializers.toByteArray(b.build()); + } + + private static ChangeColumnsProto toChangeColumnsProto(ChangeColumns cols) { + ChangeColumnsProto.Builder b = + ChangeColumnsProto.newBuilder() + .setChangeKey(cols.changeKey().get()) + .setCreatedOn(cols.createdOn().getTime()) + .setLastUpdatedOn(cols.lastUpdatedOn().getTime()) + .setOwner(cols.owner().get()) + .setBranch(cols.branch()); + if (cols.currentPatchSetId() != null) { + b.setCurrentPatchSetId(cols.currentPatchSetId().get()).setHasCurrentPatchSetId(true); + } + b.setSubject(cols.subject()); + if (cols.topic() != null) { + b.setTopic(cols.topic()).setHasTopic(true); + } + if (cols.originalSubject() != null) { + b.setOriginalSubject(cols.originalSubject()).setHasOriginalSubject(true); + } + if (cols.submissionId() != null) { + b.setSubmissionId(cols.submissionId()).setHasSubmissionId(true); + } + if (cols.assignee() != null) { + b.setAssignee(cols.assignee().get()).setHasAssignee(true); + } + if (cols.status() != null) { + b.setStatus(STATUS_CONVERTER.reverse().convert(cols.status())).setHasStatus(true); + } + b.setIsPrivate(cols.isPrivate()) + .setWorkInProgress(cols.workInProgress()) + .setReviewStarted(cols.reviewStarted()); + if (cols.revertOf() != null) { + b.setRevertOf(cols.revertOf().get()).setHasRevertOf(true); + } + return b.build(); + } + + private static ReviewerSetEntryProto toReviewerSetEntry( + Table.Cell c) { + return ReviewerSetEntryProto.newBuilder() + .setState(REVIEWER_STATE_CONVERTER.reverse().convert(c.getRowKey())) + .setAccountId(c.getColumnKey().get()) + .setTimestamp(c.getValue().getTime()) + .build(); + } + + private static ReviewerByEmailSetEntryProto toReviewerByEmailSetEntry( + Table.Cell c) { + return ReviewerByEmailSetEntryProto.newBuilder() + .setState(REVIEWER_STATE_CONVERTER.reverse().convert(c.getRowKey())) + .setAddress(c.getColumnKey().toHeaderString()) + .setTimestamp(c.getValue().getTime()) + .build(); + } + + private static ReviewerStatusUpdateProto toReviewerStatusUpdateProto(ReviewerStatusUpdate u) { + return ReviewerStatusUpdateProto.newBuilder() + .setDate(u.date().getTime()) + .setUpdatedBy(u.updatedBy().get()) + .setReviewer(u.reviewer().get()) + .setState(REVIEWER_STATE_CONVERTER.reverse().convert(u.state())) + .build(); + } + + @Override + public ChangeNotesState deserialize(byte[] in) { + ChangeNotesStateProto proto; + try { + proto = ChangeNotesStateProto.parseFrom(in); + } catch (IOException e) { + throw new IllegalArgumentException( + "Failed to deserialize " + ChangeNotesState.class.getName()); + } + Change.Id changeId = new Change.Id(proto.getChangeId()); + + ChangeNotesState.Builder b = + builder() + .metaId(ObjectId.fromRaw(proto.getMetaId().toByteArray())) + .changeId(changeId) + .columns(toChangeColumns(changeId, proto.getColumns())) + .pastAssignees( + proto + .getPastAssigneeList() + .stream() + .map(Account.Id::new) + .collect(toImmutableSet())) + .hashtags(proto.getHashtagList()) + .patchSets( + proto + .getPatchSetList() + .stream() + .map(PATCH_SET_CODEC::decode) + .map(ps -> Maps.immutableEntry(ps.getId(), ps)) + .collect(toImmutableList())) + .approvals( + proto + .getApprovalList() + .stream() + .map(APPROVAL_CODEC::decode) + .map(a -> Maps.immutableEntry(a.getPatchSetId(), a)) + .collect(toImmutableList())) + .reviewers(toReviewerSet(proto.getReviewerList())) + .reviewersByEmail(toReviewerByEmailSet(proto.getReviewerByEmailList())) + .pendingReviewers(toReviewerSet(proto.getPendingReviewerList())) + .pendingReviewersByEmail(toReviewerByEmailSet(proto.getPendingReviewerByEmailList())) + .allPastReviewers( + proto + .getPastReviewerList() + .stream() + .map(Account.Id::new) + .collect(toImmutableList())) + .reviewerUpdates(toReviewerStatusUpdateList(proto.getReviewerUpdateList())) + .submitRecords( + proto + .getSubmitRecordList() + .stream() + .map(r -> GSON.fromJson(r, StoredSubmitRecord.class).toSubmitRecord()) + .collect(toImmutableList())) + .changeMessages( + proto + .getChangeMessageList() + .stream() + .map(MESSAGE_CODEC::decode) + .collect(toImmutableList())) + .publishedComments( + proto + .getPublishedCommentList() + .stream() + .map(r -> GSON.fromJson(r, Comment.class)) + .collect(toImmutableListMultimap(c -> new RevId(c.revId), c -> c))); + if (proto.getHasReadOnlyUntil()) { + b.readOnlyUntil(new Timestamp(proto.getReadOnlyUntil())); + } + return b.build(); + } + + private static ChangeColumns toChangeColumns(Change.Id changeId, ChangeColumnsProto proto) { + ChangeColumns.Builder b = + ChangeColumns.builder() + .changeKey(new Change.Key(proto.getChangeKey())) + .createdOn(new Timestamp(proto.getCreatedOn())) + .lastUpdatedOn(new Timestamp(proto.getLastUpdatedOn())) + .owner(new Account.Id(proto.getOwner())) + .branch(proto.getBranch()); + if (proto.getHasCurrentPatchSetId()) { + b.currentPatchSetId(new PatchSet.Id(changeId, proto.getCurrentPatchSetId())); + } + b.subject(proto.getSubject()); + if (proto.getHasTopic()) { + b.topic(proto.getTopic()); + } + if (proto.getHasOriginalSubject()) { + b.originalSubject(proto.getOriginalSubject()); + } + if (proto.getHasSubmissionId()) { + b.submissionId(proto.getSubmissionId()); + } + if (proto.getHasAssignee()) { + b.assignee(new Account.Id(proto.getAssignee())); + } + if (proto.getHasStatus()) { + b.status(STATUS_CONVERTER.convert(proto.getStatus())); + } + b.isPrivate(proto.getIsPrivate()) + .workInProgress(proto.getWorkInProgress()) + .reviewStarted(proto.getReviewStarted()); + if (proto.getHasRevertOf()) { + b.revertOf(new Change.Id(proto.getRevertOf())); + } + return b.build(); + } + + private static ReviewerSet toReviewerSet(List protos) { + ImmutableTable.Builder b = + ImmutableTable.builder(); + for (ReviewerSetEntryProto e : protos) { + b.put( + REVIEWER_STATE_CONVERTER.convert(e.getState()), + new Account.Id(e.getAccountId()), + new Timestamp(e.getTimestamp())); + } + return ReviewerSet.fromTable(b.build()); + } + + private static ReviewerByEmailSet toReviewerByEmailSet( + List protos) { + ImmutableTable.Builder b = + ImmutableTable.builder(); + for (ReviewerByEmailSetEntryProto e : protos) { + b.put( + REVIEWER_STATE_CONVERTER.convert(e.getState()), + Address.parse(e.getAddress()), + new Timestamp(e.getTimestamp())); + } + return ReviewerByEmailSet.fromTable(b.build()); + } + + private static ImmutableList toReviewerStatusUpdateList( + List protos) { + ImmutableList.Builder b = ImmutableList.builder(); + for (ReviewerStatusUpdateProto proto : protos) { + b.add( + ReviewerStatusUpdate.create( + new Timestamp(proto.getDate()), + new Account.Id(proto.getUpdatedBy()), + new Account.Id(proto.getReviewer()), + REVIEWER_STATE_CONVERTER.convert(proto.getState()))); + } + return b.build(); + } + } } diff --git a/javatests/com/google/gerrit/server/BUILD b/javatests/com/google/gerrit/server/BUILD index 1b11dd65ba..3113a8afb6 100644 --- a/javatests/com/google/gerrit/server/BUILD +++ b/javatests/com/google/gerrit/server/BUILD @@ -65,6 +65,7 @@ junit_tests( "//lib/jgit/org.eclipse.jgit.junit:junit", "//lib/truth", "//lib/truth:truth-java8-extension", + "//lib/truth:truth-proto-extension", "//proto:cache_java_proto", ], ) diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesCacheTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesCacheTest.java new file mode 100644 index 0000000000..5a7d812e05 --- /dev/null +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesCacheTest.java @@ -0,0 +1,60 @@ +// 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.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.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.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesKeyProto; +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Test; + +public final class ChangeNotesCacheTest { + @Test + public void keySerializer() throws Exception { + ChangeNotesCache.Key key = + ChangeNotesCache.Key.create( + new Project.NameKey("project"), + new Change.Id(1234), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")); + byte[] serialized = ChangeNotesCache.Key.Serializer.INSTANCE.serialize(key); + assertThat(ChangeNotesKeyProto.parseFrom(serialized)) + .isEqualTo( + ChangeNotesKeyProto.newBuilder() + .setProject("project") + .setChangeId(1234) + .setId( + bytes( + 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, + 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef)) + .build()); + assertThat(ChangeNotesCache.Key.Serializer.INSTANCE.deserialize(serialized)).isEqualTo(key); + } + + @Test + public void keyMethods() throws Exception { + assertThatSerializedClass(ChangeNotesCache.Key.class) + .hasAutoValueMethods( + ImmutableMap.of( + "project", Project.NameKey.class, + "changeId", Change.Id.class, + "id", ObjectId.class)); + } +} diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java new file mode 100644 index 0000000000..c0f2c4380f --- /dev/null +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -0,0 +1,957 @@ +// 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.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.reviewdb.server.ReviewDbCodecs.APPROVAL_CODEC; +import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.MESSAGE_CODEC; +import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.PATCH_SET_CODEC; +import static com.google.gerrit.server.cache.testing.SerializedClassSubject.assertThatSerializedClass; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableTable; +import com.google.common.collect.Iterables; +import com.google.gerrit.common.data.SubmitRecord; +import com.google.gerrit.common.data.SubmitRequirement; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.ChangeMessage; +import com.google.gerrit.reviewdb.client.Comment; +import com.google.gerrit.reviewdb.client.LabelId; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.ReviewerByEmailSet; +import com.google.gerrit.server.ReviewerSet; +import com.google.gerrit.server.ReviewerStatusUpdate; +import com.google.gerrit.server.cache.ProtoCacheSerializers; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ChangeColumnsProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerByEmailSetEntryProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerSetEntryProto; +import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerStatusUpdateProto; +import com.google.gerrit.server.mail.Address; +import com.google.gerrit.server.notedb.ChangeNotesState.ChangeColumns; +import com.google.gerrit.server.notedb.ChangeNotesState.Serializer; +import com.google.gwtorm.client.KeyUtil; +import com.google.gwtorm.protobuf.ProtobufCodec; +import com.google.gwtorm.server.StandardKeyEncoder; +import com.google.inject.TypeLiteral; +import com.google.protobuf.ByteString; +import java.lang.reflect.Type; +import java.sql.Timestamp; +import java.util.List; +import java.util.Map; +import org.eclipse.jgit.lib.Constants; +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Before; +import org.junit.Test; + +public class ChangeNotesStateTest { + static { + KeyUtil.setEncoderImpl(new StandardKeyEncoder()); + } + + private static final Change.Id ID = new Change.Id(123); + private static final ObjectId SHA = + ObjectId.fromString("1234567812345678123456781234567812345678"); + private static final ByteString SHA_BYTES = toByteString(SHA); + private static final String CHANGE_KEY = "Iabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd"; + + private ChangeColumns cols; + private ChangeColumnsProto colsProto; + + @Before + public void setUp() throws Exception { + cols = + ChangeColumns.builder() + .changeKey(new Change.Key(CHANGE_KEY)) + .createdOn(new Timestamp(123456L)) + .lastUpdatedOn(new Timestamp(234567L)) + .owner(new Account.Id(1000)) + .branch("refs/heads/master") + .subject("Test change") + .isPrivate(false) + .workInProgress(false) + .reviewStarted(true) + .build(); + colsProto = toProto(newBuilder().build()).getColumns(); + } + + private ChangeNotesState.Builder newBuilder() { + return ChangeNotesState.Builder.empty(ID).metaId(SHA).columns(cols); + } + + @Test + public void serializeChangeKey() throws Exception { + assertRoundTrip( + newBuilder() + .columns( + cols.toBuilder() + .changeKey(new Change.Key("Ieeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee")) + .build()) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns( + colsProto.toBuilder().setChangeKey("Ieeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee")) + .build()); + } + + @Test + public void serializeCreatedOn() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().createdOn(new Timestamp(98765L)).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setCreatedOn(98765L)) + .build()); + } + + @Test + public void serializeLastUpdatedOn() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().lastUpdatedOn(new Timestamp(98765L)).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setLastUpdatedOn(98765L)) + .build()); + } + + @Test + public void serializeOwner() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().owner(new Account.Id(7777)).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setOwner(7777)) + .build()); + } + + @Test + public void serializeBranch() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().branch("refs/heads/bar").build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setBranch("refs/heads/bar")) + .build()); + } + + @Test + public void serializeSubject() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().subject("A different test change").build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setSubject("A different test change")) + .build()); + } + + @Test + public void serializeCurrentPatchSetId() throws Exception { + assertRoundTrip( + newBuilder() + .columns(cols.toBuilder().currentPatchSetId(new PatchSet.Id(ID, 2)).build()) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setCurrentPatchSetId(2).setHasCurrentPatchSetId(true)) + .build()); + } + + @Test + public void serializeNullTopic() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().topic(null).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .build()); + } + + @Test + public void serializeEmptyTopic() throws Exception { + ChangeNotesState state = newBuilder().columns(cols.toBuilder().topic("").build()).build(); + assertRoundTrip( + state, + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setTopic("").setHasTopic(true)) + .build()); + } + + @Test + public void serializeNonEmptyTopic() throws Exception { + ChangeNotesState state = newBuilder().columns(cols.toBuilder().topic("topic").build()).build(); + assertRoundTrip( + state, + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setTopic("topic").setHasTopic(true)) + .build()); + } + + @Test + public void serializeOriginalSubject() throws Exception { + assertRoundTrip( + newBuilder() + .columns(cols.toBuilder().originalSubject("The first patch set").build()) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns( + colsProto + .toBuilder() + .setOriginalSubject("The first patch set") + .setHasOriginalSubject(true)) + .build()); + } + + @Test + public void serializeSubmissionId() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().submissionId("xyz").build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setSubmissionId("xyz").setHasSubmissionId(true)) + .build()); + } + + @Test + public void serializeAssignee() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().assignee(new Account.Id(2000)).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setAssignee(2000).setHasAssignee(true)) + .build()); + } + + @Test + public void serializeStatus() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().status(Change.Status.MERGED).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setStatus("MERGED").setHasStatus(true)) + .build()); + } + + @Test + public void serializeIsPrivate() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().isPrivate(true).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setIsPrivate(true)) + .build()); + } + + @Test + public void serializeIsWorkInProgress() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().workInProgress(true).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setWorkInProgress(true)) + .build()); + } + + @Test + public void serializeHasReviewStarted() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().reviewStarted(true).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setReviewStarted(true)) + .build()); + } + + @Test + public void serializeRevertOf() throws Exception { + assertRoundTrip( + newBuilder().columns(cols.toBuilder().revertOf(new Change.Id(999)).build()).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto.toBuilder().setRevertOf(999).setHasRevertOf(true)) + .build()); + } + + @Test + public void serializePastAssignees() throws Exception { + assertRoundTrip( + newBuilder() + .pastAssignees(ImmutableSet.of(new Account.Id(2002), new Account.Id(2001))) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addPastAssignee(2002) + .addPastAssignee(2001) + .build()); + } + + @Test + public void serializeHashtags() throws Exception { + assertRoundTrip( + newBuilder().hashtags(ImmutableSet.of("tag2", "tag1")).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addHashtag("tag2") + .addHashtag("tag1") + .build()); + } + + @Test + public void serializePatchSets() throws Exception { + PatchSet ps1 = new PatchSet(new PatchSet.Id(ID, 1)); + ps1.setUploader(new Account.Id(2000)); + ps1.setRevision(new RevId("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); + ps1.setCreatedOn(cols.createdOn()); + ByteString ps1Bytes = toByteString(ps1, PATCH_SET_CODEC); + assertThat(ps1Bytes.size()).isEqualTo(66); + + PatchSet ps2 = new PatchSet(new PatchSet.Id(ID, 2)); + ps2.setUploader(new Account.Id(3000)); + ps2.setRevision(new RevId("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")); + ps2.setCreatedOn(cols.lastUpdatedOn()); + ByteString ps2Bytes = toByteString(ps2, PATCH_SET_CODEC); + assertThat(ps2Bytes.size()).isEqualTo(66); + assertThat(ps2Bytes).isNotEqualTo(ps1Bytes); + + assertRoundTrip( + newBuilder() + .patchSets(ImmutableMap.of(ps2.getId(), ps2, ps1.getId(), ps1).entrySet()) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addPatchSet(ps2Bytes) + .addPatchSet(ps1Bytes) + .build()); + } + + @Test + public void serializeApprovals() throws Exception { + PatchSetApproval a1 = + new PatchSetApproval( + new PatchSetApproval.Key( + new PatchSet.Id(ID, 1), new Account.Id(2001), new LabelId("Code-Review")), + (short) 1, + new Timestamp(1212L)); + ByteString a1Bytes = toByteString(a1, APPROVAL_CODEC); + assertThat(a1Bytes.size()).isEqualTo(43); + + PatchSetApproval a2 = + new PatchSetApproval( + new PatchSetApproval.Key( + new PatchSet.Id(ID, 1), new Account.Id(2002), new LabelId("Verified")), + (short) -1, + new Timestamp(3434L)); + ByteString a2Bytes = toByteString(a2, APPROVAL_CODEC); + assertThat(a2Bytes.size()).isEqualTo(49); + assertThat(a2Bytes).isNotEqualTo(a1Bytes); + + assertRoundTrip( + newBuilder() + .approvals( + ImmutableListMultimap.of(a2.getPatchSetId(), a2, a1.getPatchSetId(), a1).entries()) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addApproval(a2Bytes) + .addApproval(a1Bytes) + .build()); + } + + @Test + public void serializeReviewers() throws Exception { + assertRoundTrip( + newBuilder() + .reviewers( + ReviewerSet.fromTable( + ImmutableTable.builder() + .put(ReviewerStateInternal.CC, new Account.Id(2001), new Timestamp(1212L)) + .put( + ReviewerStateInternal.REVIEWER, + new Account.Id(2002), + new Timestamp(3434L)) + .build())) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addReviewer( + ReviewerSetEntryProto.newBuilder() + .setState("CC") + .setAccountId(2001) + .setTimestamp(1212L)) + .addReviewer( + ReviewerSetEntryProto.newBuilder() + .setState("REVIEWER") + .setAccountId(2002) + .setTimestamp(3434L)) + .build()); + } + + @Test + public void serializeReviewersByEmail() throws Exception { + assertRoundTrip( + newBuilder() + .reviewersByEmail( + ReviewerByEmailSet.fromTable( + ImmutableTable.builder() + .put( + ReviewerStateInternal.CC, + new Address("Name1", "email1@example.com"), + new Timestamp(1212L)) + .put( + ReviewerStateInternal.REVIEWER, + new Address("Name2", "email2@example.com"), + new Timestamp(3434L)) + .build())) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addReviewerByEmail( + ReviewerByEmailSetEntryProto.newBuilder() + .setState("CC") + .setAddress("Name1 ") + .setTimestamp(1212L)) + .addReviewerByEmail( + ReviewerByEmailSetEntryProto.newBuilder() + .setState("REVIEWER") + .setAddress("Name2 ") + .setTimestamp(3434L)) + .build()); + } + + @Test + public void serializeReviewersByEmailWithNullName() throws Exception { + ChangeNotesState actual = + assertRoundTrip( + newBuilder() + .reviewersByEmail( + ReviewerByEmailSet.fromTable( + ImmutableTable.of( + ReviewerStateInternal.CC, + new Address("emailonly@example.com"), + new Timestamp(1212L)))) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addReviewerByEmail( + ReviewerByEmailSetEntryProto.newBuilder() + .setState("CC") + .setAddress("emailonly@example.com") + .setTimestamp(1212L)) + .build()); + + // Address doesn't consider the name field in equals, so we have to check it manually. + // TODO(dborowitz): Fix Address#equals. + ImmutableSet

ccs = actual.reviewersByEmail().byState(ReviewerStateInternal.CC); + assertThat(ccs).hasSize(1); + Address address = Iterables.getOnlyElement(ccs); + assertThat(address.getName()).isNull(); + assertThat(address.getEmail()).isEqualTo("emailonly@example.com"); + } + + @Test + public void serializePendingReviewers() throws Exception { + assertRoundTrip( + newBuilder() + .pendingReviewers( + ReviewerSet.fromTable( + ImmutableTable.builder() + .put(ReviewerStateInternal.CC, new Account.Id(2001), new Timestamp(1212L)) + .put( + ReviewerStateInternal.REVIEWER, + new Account.Id(2002), + new Timestamp(3434L)) + .build())) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addPendingReviewer( + ReviewerSetEntryProto.newBuilder() + .setState("CC") + .setAccountId(2001) + .setTimestamp(1212L)) + .addPendingReviewer( + ReviewerSetEntryProto.newBuilder() + .setState("REVIEWER") + .setAccountId(2002) + .setTimestamp(3434L)) + .build()); + } + + @Test + public void serializePendingReviewersByEmail() throws Exception { + assertRoundTrip( + newBuilder() + .pendingReviewersByEmail( + ReviewerByEmailSet.fromTable( + ImmutableTable.builder() + .put( + ReviewerStateInternal.CC, + new Address("Name1", "email1@example.com"), + new Timestamp(1212L)) + .put( + ReviewerStateInternal.REVIEWER, + new Address("Name2", "email2@example.com"), + new Timestamp(3434L)) + .build())) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addPendingReviewerByEmail( + ReviewerByEmailSetEntryProto.newBuilder() + .setState("CC") + .setAddress("Name1 ") + .setTimestamp(1212L)) + .addPendingReviewerByEmail( + ReviewerByEmailSetEntryProto.newBuilder() + .setState("REVIEWER") + .setAddress("Name2 ") + .setTimestamp(3434L)) + .build()); + } + + @Test + public void serializeAllPastReviewers() throws Exception { + assertRoundTrip( + newBuilder() + .allPastReviewers(ImmutableList.of(new Account.Id(2002), new Account.Id(2001))) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addPastReviewer(2002) + .addPastReviewer(2001) + .build()); + } + + @Test + public void serializeReviewerUpdates() throws Exception { + assertRoundTrip( + newBuilder() + .reviewerUpdates( + ImmutableList.of( + ReviewerStatusUpdate.create( + new Timestamp(1212L), + new Account.Id(1000), + new Account.Id(2002), + ReviewerStateInternal.CC), + ReviewerStatusUpdate.create( + new Timestamp(3434L), + new Account.Id(1000), + new Account.Id(2001), + ReviewerStateInternal.REVIEWER))) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addReviewerUpdate( + ReviewerStatusUpdateProto.newBuilder() + .setDate(1212L) + .setUpdatedBy(1000) + .setReviewer(2002) + .setState("CC")) + .addReviewerUpdate( + ReviewerStatusUpdateProto.newBuilder() + .setDate(3434L) + .setUpdatedBy(1000) + .setReviewer(2001) + .setState("REVIEWER")) + .build()); + } + + @Test + public void serializeSubmitRecords() throws Exception { + SubmitRecord sr1 = new SubmitRecord(); + sr1.status = SubmitRecord.Status.OK; + + SubmitRecord sr2 = new SubmitRecord(); + sr2.status = SubmitRecord.Status.FORCED; + + assertRoundTrip( + newBuilder().submitRecords(ImmutableList.of(sr2, sr1)).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addSubmitRecord("{\"status\":\"FORCED\"}") + .addSubmitRecord("{\"status\":\"OK\"}") + .build()); + } + + @Test + public void serializeChangeMessages() throws Exception { + ChangeMessage m1 = + new ChangeMessage( + new ChangeMessage.Key(ID, "uuid1"), + new Account.Id(1000), + new Timestamp(1212L), + new PatchSet.Id(ID, 1)); + ByteString m1Bytes = toByteString(m1, MESSAGE_CODEC); + assertThat(m1Bytes.size()).isEqualTo(35); + + ChangeMessage m2 = + new ChangeMessage( + new ChangeMessage.Key(ID, "uuid2"), + new Account.Id(2000), + new Timestamp(3434L), + new PatchSet.Id(ID, 2)); + ByteString m2Bytes = toByteString(m2, MESSAGE_CODEC); + assertThat(m2Bytes.size()).isEqualTo(35); + assertThat(m2Bytes).isNotEqualTo(m1Bytes); + + assertRoundTrip( + newBuilder().changeMessages(ImmutableList.of(m2, m1)).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addChangeMessage(m2Bytes) + .addChangeMessage(m1Bytes) + .build()); + } + + @Test + public void serializePublishedComments() throws Exception { + Comment c1 = + new Comment( + new Comment.Key("uuid1", "file1", 1), + new Account.Id(1001), + new Timestamp(1212L), + (short) 1, + "message 1", + "serverId", + false); + c1.setRevId(new RevId("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); + String c1Json = Serializer.GSON.toJson(c1); + + Comment c2 = + new Comment( + new Comment.Key("uuid2", "file2", 2), + new Account.Id(1002), + new Timestamp(3434L), + (short) 2, + "message 2", + "serverId", + true); + c2.setRevId(new RevId("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")); + String c2Json = Serializer.GSON.toJson(c2); + + assertRoundTrip( + newBuilder() + .publishedComments( + ImmutableListMultimap.of(new RevId(c2.revId), c2, new RevId(c1.revId), c1)) + .build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .addPublishedComment(c2Json) + .addPublishedComment(c1Json) + .build()); + } + + @Test + public void serializeReadOnlyUntil() throws Exception { + assertRoundTrip( + newBuilder().readOnlyUntil(new Timestamp(1212L)).build(), + ChangeNotesStateProto.newBuilder() + .setMetaId(SHA_BYTES) + .setChangeId(ID.get()) + .setColumns(colsProto) + .setReadOnlyUntil(1212L) + .setHasReadOnlyUntil(true) + .build()); + } + + @Test + public void changeNotesStateMethods() throws Exception { + assertThatSerializedClass(ChangeNotesState.class) + .hasAutoValueMethods( + ImmutableMap.builder() + .put("metaId", ObjectId.class) + .put("changeId", Change.Id.class) + .put("columns", ChangeColumns.class) + .put("pastAssignees", new TypeLiteral>() {}.getType()) + .put("hashtags", new TypeLiteral>() {}.getType()) + .put( + "patchSets", + new TypeLiteral>>() {}.getType()) + .put( + "approvals", + new TypeLiteral< + ImmutableList>>() {}.getType()) + .put("reviewers", ReviewerSet.class) + .put("reviewersByEmail", ReviewerByEmailSet.class) + .put("pendingReviewers", ReviewerSet.class) + .put("pendingReviewersByEmail", ReviewerByEmailSet.class) + .put("allPastReviewers", new TypeLiteral>() {}.getType()) + .put( + "reviewerUpdates", + new TypeLiteral>() {}.getType()) + .put("submitRecords", new TypeLiteral>() {}.getType()) + .put("changeMessages", new TypeLiteral>() {}.getType()) + .put( + "publishedComments", + new TypeLiteral>() {}.getType()) + .put("readOnlyUntil", Timestamp.class) + .build()); + } + + @Test + public void changeColumnsMethods() throws Exception { + assertThatSerializedClass(ChangeColumns.class) + .hasAutoValueMethods( + ImmutableMap.builder() + .put("changeKey", Change.Key.class) + .put("createdOn", Timestamp.class) + .put("lastUpdatedOn", Timestamp.class) + .put("owner", Account.Id.class) + .put("branch", String.class) + .put("currentPatchSetId", PatchSet.Id.class) + .put("subject", String.class) + .put("topic", String.class) + .put("originalSubject", String.class) + .put("submissionId", String.class) + .put("assignee", Account.Id.class) + .put("status", Change.Status.class) + .put("isPrivate", boolean.class) + .put("workInProgress", boolean.class) + .put("reviewStarted", boolean.class) + .put("revertOf", Change.Id.class) + .put("toBuilder", ChangeNotesState.ChangeColumns.Builder.class) + .build()); + } + + @Test + public void patchSetFields() throws Exception { + assertThatSerializedClass(PatchSet.class) + .hasFields( + ImmutableMap.builder() + .put("id", PatchSet.Id.class) + .put("revision", RevId.class) + .put("uploader", Account.Id.class) + .put("createdOn", Timestamp.class) + .put("groups", String.class) + .put("pushCertificate", String.class) + .put("description", String.class) + .build()); + } + + @Test + public void patchSetApprovalFields() throws Exception { + assertThatSerializedClass(PatchSetApproval.Key.class) + .hasFields( + ImmutableMap.builder() + .put("patchSetId", PatchSet.Id.class) + .put("accountId", Account.Id.class) + .put("categoryId", LabelId.class) + .build()); + assertThatSerializedClass(PatchSetApproval.class) + .hasFields( + ImmutableMap.builder() + .put("key", PatchSetApproval.Key.class) + .put("value", short.class) + .put("granted", Timestamp.class) + .put("tag", String.class) + .put("realAccountId", Account.Id.class) + .put("postSubmit", boolean.class) + .build()); + } + + @Test + public void reviewerSetFields() throws Exception { + assertThatSerializedClass(ReviewerSet.class) + .hasFields( + ImmutableMap.of( + "table", + new TypeLiteral< + ImmutableTable< + ReviewerStateInternal, Account.Id, Timestamp>>() {}.getType(), + "accounts", new TypeLiteral>() {}.getType())); + } + + @Test + public void reviewerByEmailSetFields() throws Exception { + assertThatSerializedClass(ReviewerByEmailSet.class) + .hasFields( + ImmutableMap.of( + "table", + new TypeLiteral< + ImmutableTable>() {}.getType(), + "users", new TypeLiteral>() {}.getType())); + } + + @Test + public void reviewerStatusUpdateMethods() throws Exception { + assertThatSerializedClass(ReviewerStatusUpdate.class) + .hasAutoValueMethods( + ImmutableMap.of( + "date", Timestamp.class, + "updatedBy", Account.Id.class, + "reviewer", Account.Id.class, + "state", ReviewerStateInternal.class)); + } + + @Test + public void submitRecordFields() throws Exception { + assertThatSerializedClass(SubmitRecord.class) + .hasFields( + ImmutableMap.of( + "status", + SubmitRecord.Status.class, + "labels", + new TypeLiteral>() {}.getType(), + "requirements", + new TypeLiteral>() {}.getType(), + "errorMessage", + String.class)); + assertThatSerializedClass(SubmitRecord.Label.class) + .hasFields( + ImmutableMap.of( + "label", String.class, + "status", SubmitRecord.Label.Status.class, + "appliedBy", Account.Id.class)); + assertThatSerializedClass(SubmitRequirement.class) + .hasAutoValueMethods( + ImmutableMap.of( + "fallbackText", String.class, + "type", String.class, + "data", new TypeLiteral>() {}.getType())); + } + + @Test + public void changeMessageFields() throws Exception { + assertThatSerializedClass(ChangeMessage.Key.class) + .hasFields(ImmutableMap.of("changeId", Change.Id.class, "uuid", String.class)); + assertThatSerializedClass(ChangeMessage.class) + .hasFields( + ImmutableMap.builder() + .put("key", ChangeMessage.Key.class) + .put("author", Account.Id.class) + .put("writtenOn", Timestamp.class) + .put("message", String.class) + .put("patchset", PatchSet.Id.class) + .put("tag", String.class) + .put("realAuthor", Account.Id.class) + .build()); + } + + @Test + public void commentFields() throws Exception { + assertThatSerializedClass(Comment.Key.class) + .hasFields( + ImmutableMap.of( + "uuid", String.class, "filename", String.class, "patchSetId", int.class)); + assertThatSerializedClass(Comment.Identity.class).hasFields(ImmutableMap.of("id", int.class)); + assertThatSerializedClass(Comment.Range.class) + .hasFields( + ImmutableMap.of( + "startLine", int.class, + "startChar", int.class, + "endLine", int.class, + "endChar", int.class)); + assertThatSerializedClass(Comment.class) + .hasFields( + ImmutableMap.builder() + .put("key", Comment.Key.class) + .put("lineNbr", int.class) + .put("author", Comment.Identity.class) + .put("realAuthor", Comment.Identity.class) + .put("writtenOn", Timestamp.class) + .put("side", short.class) + .put("message", String.class) + .put("parentUuid", String.class) + .put("range", Comment.Range.class) + .put("tag", String.class) + .put("revId", String.class) + .put("serverId", String.class) + .put("unresolved", boolean.class) + .put("legacyFormat", boolean.class) + .build()); + } + + private static ChangeNotesStateProto toProto(ChangeNotesState state) throws Exception { + return ChangeNotesStateProto.parseFrom(Serializer.INSTANCE.serialize(state)); + } + + private static ChangeNotesState assertRoundTrip( + ChangeNotesState state, ChangeNotesStateProto expectedProto) throws Exception { + ChangeNotesStateProto actualProto = toProto(state); + assertThat(actualProto).isEqualTo(expectedProto); + ChangeNotesState actual = Serializer.INSTANCE.deserialize(Serializer.INSTANCE.serialize(state)); + assertThat(actual).isEqualTo(state); + // It's possible that ChangeNotesState contains objects which implement equals without taking + // into account all fields. Return the actual deserialized instance so that callers can perform + // additional assertions if necessary. + return actual; + } + + private static ByteString toByteString(ObjectId id) { + byte[] buf = new byte[Constants.OBJECT_ID_LENGTH]; + id.copyRawTo(buf, 0); + return ByteString.copyFrom(buf); + } + + private ByteString toByteString(T object, ProtobufCodec codec) { + return ProtoCacheSerializers.toByteString(object, codec); + } +} diff --git a/lib/truth/BUILD b/lib/truth/BUILD index cb17269b01..82cd98a9a6 100644 --- a/lib/truth/BUILD +++ b/lib/truth/BUILD @@ -19,3 +19,31 @@ java_library( "//lib:guava", ], ) + +java_library( + name = "truth-liteproto-extension", + data = ["//lib:LICENSE-DO_NOT_DISTRIBUTE"], + visibility = ["//visibility:private"], + exports = ["@truth-liteproto-extension//jar"], + runtime_deps = [ + ":truth", + "//lib:guava", + "//lib:protobuf", + ], +) + +java_library( + name = "truth-proto-extension", + data = ["//lib:LICENSE-DO_NOT_DISTRIBUTE"], + visibility = ["//visibility:public"], + exports = [ + ":truth-liteproto-extension", + "@truth-proto-extension//jar", + ], + runtime_deps = [ + ":truth", + ":truth-liteproto-extension", + "//lib:guava", + "//lib:protobuf", + ], +) diff --git a/proto/cache.proto b/proto/cache.proto index 634b595422..7e2e75a801 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -45,3 +45,142 @@ message OAuthTokenProto { int64 expires_at = 4; string provider_id = 5; } + + +// Serialized form of com.google.gerrit.server.notedb.ChangeNotesCache.Key. +// Next ID: 4 +message ChangeNotesKeyProto { + string project = 1; + int32 change_id = 2; + bytes id = 3; +} + +// Serialized from of com.google.gerrit.server.notedb.ChangeNotesState. +// +// Note on embedded protos: this is just for storing in a cache, so some formats +// were chosen ease of coding the initial implementation. In particular, where +// there already exists another serialization mechanism in Gerrit for +// serializing a particular field, we use that rather than defining a new proto +// type. This includes ReviewDb types that can be serialized to proto using +// ProtobufCodec as well as NoteDb and indexed types that are serialized using +// JSON. We can always revisit this decision later, particularly when we +// eliminate the ReviewDb types; it just requires bumping the cache version. +// +// Note on nullability: there are a lot of nullable fields in ChangeNotesState +// and its dependencies. It's likely we could make some of them non-nullable, +// but each one of those would be a potentially significant amount of cleanup, +// and there's no guarantee we'd be able to eliminate all of them. (For a less +// complex class, it's likely the cleanup would be more feasible.) +// +// Instead, we just take the tedious yet simple approach of having a "has_foo" +// field for each nullable field "foo", indicating whether or not foo is null. +// +// Next ID: 19 +message ChangeNotesStateProto { + // Effectively required, even though the corresponding ChangeNotesState field + // is optional, since the field is only absent when NoteDb is disabled, in + // which case attempting to use the ChangeNotesCache is programmer error. + bytes meta_id = 1; + + int32 change_id = 2; + + // Next ID: 24 + message ChangeColumnsProto { + string change_key = 1; + + int64 created_on = 2; + + int64 last_updated_on = 3; + + int32 owner = 4; + + string branch = 5; + + int32 current_patch_set_id = 6; + bool has_current_patch_set_id = 7; + + string subject = 8; + + string topic = 9; + bool has_topic = 10; + + string original_subject = 11; + bool has_original_subject = 12; + + string submission_id = 13; + bool has_submission_id = 14; + + int32 assignee = 15; + bool has_assignee = 16; + + string status = 17; + bool has_status = 18; + + bool is_private = 19; + + bool work_in_progress = 20; + + bool review_started = 21; + + int32 revert_of = 22; + bool has_revert_of = 23; + } + // Effectively required, even though the corresponding ChangeNotesState field + // is optional, since the field is only absent when NoteDb is disabled, in + // which case attempting to use the ChangeNotesCache is programmer error. + ChangeColumnsProto columns = 3; + + repeated int32 past_assignee = 4; + + repeated string hashtag = 5; + + // Raw PatchSet proto as produced by ProtobufCodec. + repeated bytes patch_set = 6; + + // Raw PatchSetApproval proto as produced by ProtobufCodec. + repeated bytes approval = 7; + + // Next ID: 4 + message ReviewerSetEntryProto { + string state = 1; + int32 account_id = 2; + int64 timestamp = 3; + } + repeated ReviewerSetEntryProto reviewer = 8; + + // Next ID: 4 + message ReviewerByEmailSetEntryProto { + string state = 1; + string address = 2; + int64 timestamp = 3; + } + repeated ReviewerByEmailSetEntryProto reviewer_by_email = 9; + + repeated ReviewerSetEntryProto pending_reviewer = 10; + + repeated ReviewerByEmailSetEntryProto pending_reviewer_by_email = 11; + + repeated int32 past_reviewer = 12; + + // Next ID: 5 + message ReviewerStatusUpdateProto { + int64 date = 1; + int32 updated_by = 2; + int32 reviewer = 3; + string state = 4; + } + repeated ReviewerStatusUpdateProto reviewer_update = 13; + + // JSON produced from + // com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord. + repeated string submit_record = 14; + + // Raw ChangeMessage proto as produced by ProtobufCodec. + repeated bytes change_message = 15; + + // JSON produced from com.google.gerrit.reviewdb.client.Comment. + repeated string published_comment = 16; + + int64 read_only_until = 17; + bool has_read_only_until = 18; +}