From 921d9d67f96e875c3c57e03e85e238c85356bfb4 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Dec 2018 15:41:19 +0100 Subject: [PATCH 1/6] Add converter for Change protobuf messages The use of ProtobufCodec requires that value classes need to be annotated with @Column, which won't be possible as soon as we have removed gwtorm. Hence, provide a hand-written converter for Change protobuf messages. As protobuf Changes are currently used in indices and we don't want to invalidate those, we have to ensure binary compatibility. Prove that the new converter generates binary compatible results via dedicated tests. Those tests will be removed after this change. Change-Id: Ie1dcd3dd28c628cb8c20d0f95147417e8b2fd260 --- .../elasticsearch/AbstractElasticIndex.java | 2 +- .../elasticsearch/ElasticChangeIndex.java | 5 +- .../gerrit/lucene/LuceneChangeIndex.java | 4 +- .../BranchNameKeyProtoConverter.java | 47 +++ .../ChangeKeyProtoConverter.java} | 26 +- .../converter/ChangeProtoConverter.java | 126 ++++++ .../ProjectNameKeyProtoConverter.java | 39 ++ .../server/index/change/ChangeField.java | 15 +- .../BranchNameKeyProtoConverterTest.java | 83 ++++ .../ChangeConverterCompatibilityTest.java | 123 ++++++ .../ChangeKeyProtoConverterTest.java | 67 ++++ .../converter/ChangeProtoConverterTest.java | 363 ++++++++++++++++++ .../ProjectNameKeyProtoConverterTest.java | 71 ++++ 13 files changed, 952 insertions(+), 19 deletions(-) create mode 100644 java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java rename java/com/google/gerrit/reviewdb/{server/ReviewDbCodecs.java => converter/ChangeKeyProtoConverter.java} (52%) create mode 100644 java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java create mode 100644 java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java create mode 100644 javatests/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverterTest.java create mode 100644 javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java create mode 100644 javatests/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverterTest.java create mode 100644 javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java create mode 100644 javatests/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverterTest.java diff --git a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java index cbfd1cad14..0379423ab1 100644 --- a/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java +++ b/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java @@ -99,7 +99,7 @@ abstract class AbstractElasticIndex implements Index { .collect(toImmutableList()); } - private static

T parseProtoFrom( + protected static

T parseProtoFrom( byte[] bytes, ProtoConverter converter) { P message = Protos.parseUnchecked(converter.getParser(), bytes); return converter.fromProto(message); diff --git a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index eda9d547b5..cf4022bae5 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java @@ -14,7 +14,6 @@ package com.google.gerrit.elasticsearch; -import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.CHANGE_CODEC; import static com.google.gerrit.server.index.change.ChangeIndexRewriter.CLOSED_STATUSES; import static com.google.gerrit.server.index.change.ChangeIndexRewriter.OPEN_STATUSES; import static java.nio.charset.StandardCharsets.UTF_8; @@ -43,6 +42,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Id; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.converter.ChangeProtoConverter; import com.google.gerrit.reviewdb.converter.PatchSetApprovalProtoConverter; import com.google.gerrit.reviewdb.converter.PatchSetProtoConverter; import com.google.gerrit.server.ReviewerByEmailSet; @@ -218,7 +218,8 @@ class ElasticChangeIndex extends AbstractElasticIndex } ChangeData cd = - changeDataFactory.create(CHANGE_CODEC.decode(Base64.decodeBase64(c.getAsString()))); + changeDataFactory.create( + parseProtoFrom(Base64.decodeBase64(c.getAsString()), ChangeProtoConverter.INSTANCE)); // Any decoding that is done here must also be done in {@link LuceneChangeIndex}. diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 056ecb1f43..938665692d 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -16,7 +16,6 @@ package com.google.gerrit.lucene; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.gerrit.lucene.AbstractLuceneIndex.sortFieldName; -import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.CHANGE_CODEC; import static com.google.gerrit.server.git.QueueProvider.QueueType.INTERACTIVE; import static com.google.gerrit.server.index.change.ChangeField.LEGACY_ID; import static com.google.gerrit.server.index.change.ChangeField.PROJECT; @@ -46,6 +45,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.converter.ChangeProtoConverter; import com.google.gerrit.reviewdb.converter.PatchSetApprovalProtoConverter; import com.google.gerrit.reviewdb.converter.PatchSetProtoConverter; import com.google.gerrit.reviewdb.converter.ProtoConverter; @@ -445,7 +445,7 @@ public class LuceneChangeIndex implements ChangeIndex { IndexableField cb = Iterables.getFirst(doc.get(CHANGE_FIELD), null); if (cb != null) { BytesRef proto = cb.binaryValue(); - cd = changeDataFactory.create(CHANGE_CODEC.decode(proto.bytes, proto.offset, proto.length)); + cd = changeDataFactory.create(parseProtoFrom(proto, ChangeProtoConverter.INSTANCE)); } else { IndexableField f = Iterables.getFirst(doc.get(idFieldName), null); Change.Id id = new Change.Id(f.numericValue().intValue()); diff --git a/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java new file mode 100644 index 0000000000..cec7511a14 --- /dev/null +++ b/java/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverter.java @@ -0,0 +1,47 @@ +// 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.reviewdb.Reviewdb; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Project; +import com.google.protobuf.Parser; + +public enum BranchNameKeyProtoConverter + implements ProtoConverter { + INSTANCE; + + private final ProtoConverter projectNameConverter = + ProjectNameKeyProtoConverter.INSTANCE; + + @Override + public Reviewdb.Branch_NameKey toProto(Branch.NameKey nameKey) { + return Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(projectNameConverter.toProto(nameKey.getParentKey())) + .setBranchName(nameKey.get()) + .build(); + } + + @Override + public Branch.NameKey fromProto(Reviewdb.Branch_NameKey proto) { + return new Branch.NameKey( + projectNameConverter.fromProto(proto.getProjectName()), proto.getBranchName()); + } + + @Override + public Parser getParser() { + return Reviewdb.Branch_NameKey.parser(); + } +} diff --git a/java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java b/java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java similarity index 52% rename from java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java rename to java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java index 7ff22848f3..704badf06f 100644 --- a/java/com/google/gerrit/reviewdb/server/ReviewDbCodecs.java +++ b/java/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverter.java @@ -12,15 +12,27 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.gerrit.reviewdb.server; +package com.google.gerrit.reviewdb.converter; +import com.google.gerrit.proto.reviewdb.Reviewdb; import com.google.gerrit.reviewdb.client.Change; -import com.google.gwtorm.protobuf.CodecFactory; -import com.google.gwtorm.protobuf.ProtobufCodec; +import com.google.protobuf.Parser; -/** {@link ProtobufCodec} instances for ReviewDb types. */ -public class ReviewDbCodecs { - public static final ProtobufCodec CHANGE_CODEC = CodecFactory.encoder(Change.class); +public enum ChangeKeyProtoConverter implements ProtoConverter { + INSTANCE; - private ReviewDbCodecs() {} + @Override + public Reviewdb.Change_Key toProto(Change.Key key) { + return Reviewdb.Change_Key.newBuilder().setId(key.get()).build(); + } + + @Override + public Change.Key fromProto(Reviewdb.Change_Key proto) { + return new Change.Key(proto.getId()); + } + + @Override + public Parser getParser() { + return Reviewdb.Change_Key.parser(); + } } diff --git a/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java new file mode 100644 index 0000000000..3d7b093d4f --- /dev/null +++ b/java/com/google/gerrit/reviewdb/converter/ChangeProtoConverter.java @@ -0,0 +1,126 @@ +// 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.reviewdb.Reviewdb; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.protobuf.Parser; +import java.sql.Timestamp; + +public enum ChangeProtoConverter implements ProtoConverter { + INSTANCE; + + private final ProtoConverter changeIdConverter = + ChangeIdProtoConverter.INSTANCE; + private final ProtoConverter changeKeyConverter = + ChangeKeyProtoConverter.INSTANCE; + private final ProtoConverter accountIdConverter = + AccountIdProtoConverter.INSTANCE; + private final ProtoConverter branchNameConverter = + BranchNameKeyProtoConverter.INSTANCE; + + @Override + public Reviewdb.Change toProto(Change change) { + Reviewdb.Change.Builder builder = + Reviewdb.Change.newBuilder() + .setChangeId(changeIdConverter.toProto(change.getId())) + .setRowVersion(change.getRowVersion()) + .setChangeKey(changeKeyConverter.toProto(change.getKey())) + .setCreatedOn(change.getCreatedOn().getTime()) + .setLastUpdatedOn(change.getLastUpdatedOn().getTime()) + .setOwnerAccountId(accountIdConverter.toProto(change.getOwner())) + .setDest(branchNameConverter.toProto(change.getDest())) + .setStatus(change.getStatus().getCode()) + .setIsPrivate(change.isPrivate()) + .setWorkInProgress(change.isWorkInProgress()) + .setReviewStarted(change.hasReviewStarted()); + PatchSet.Id currentPatchSetId = change.currentPatchSetId(); + // Special behavior necessary to ensure binary compatibility. + builder.setCurrentPatchSetId(currentPatchSetId == null ? 0 : currentPatchSetId.get()); + String subject = change.getSubject(); + if (subject != null) { + builder.setSubject(subject); + } + String topic = change.getTopic(); + if (topic != null) { + builder.setTopic(topic); + } + String originalSubject = change.getOriginalSubjectOrNull(); + if (originalSubject != null) { + builder.setOriginalSubject(originalSubject); + } + String submissionId = change.getSubmissionId(); + if (submissionId != null) { + builder.setSubmissionId(submissionId); + } + Account.Id assignee = change.getAssignee(); + if (assignee != null) { + builder.setAssignee(accountIdConverter.toProto(assignee)); + } + Change.Id revertOf = change.getRevertOf(); + if (revertOf != null) { + builder.setRevertOf(changeIdConverter.toProto(revertOf)); + } + return builder.build(); + } + + @Override + public Change fromProto(Reviewdb.Change proto) { + Change.Id changeId = changeIdConverter.fromProto(proto.getChangeId()); + Change.Key key = + proto.hasChangeKey() ? changeKeyConverter.fromProto(proto.getChangeKey()) : null; + Account.Id owner = + proto.hasOwnerAccountId() ? accountIdConverter.fromProto(proto.getOwnerAccountId()) : null; + Branch.NameKey destination = + proto.hasDest() ? branchNameConverter.fromProto(proto.getDest()) : null; + Change change = + new Change(key, changeId, owner, destination, new Timestamp(proto.getCreatedOn())); + if (proto.hasLastUpdatedOn()) { + change.setLastUpdatedOn(new Timestamp(proto.getLastUpdatedOn())); + } + Change.Status status = Change.Status.forCode((char) proto.getStatus()); + if (status != null) { + change.setStatus(status); + } + String subject = proto.hasSubject() ? proto.getSubject() : null; + String originalSubject = proto.hasOriginalSubject() ? proto.getOriginalSubject() : null; + change.setCurrentPatchSet( + new PatchSet.Id(changeId, proto.getCurrentPatchSetId()), subject, originalSubject); + if (proto.hasTopic()) { + change.setTopic(proto.getTopic()); + } + if (proto.hasSubmissionId()) { + change.setSubmissionId(proto.getSubmissionId()); + } + if (proto.hasAssignee()) { + change.setAssignee(accountIdConverter.fromProto(proto.getAssignee())); + } + change.setPrivate(proto.getIsPrivate()); + change.setWorkInProgress(proto.getWorkInProgress()); + change.setReviewStarted(proto.getReviewStarted()); + if (proto.hasRevertOf()) { + change.setRevertOf(changeIdConverter.fromProto(proto.getRevertOf())); + } + return change; + } + + @Override + public Parser getParser() { + return Reviewdb.Change.parser(); + } +} diff --git a/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java new file mode 100644 index 0000000000..ff8dba240d --- /dev/null +++ b/java/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverter.java @@ -0,0 +1,39 @@ +// 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.reviewdb.Reviewdb; +import com.google.gerrit.reviewdb.client.Project; +import com.google.protobuf.Parser; + +public enum ProjectNameKeyProtoConverter + implements ProtoConverter { + INSTANCE; + + @Override + public Reviewdb.Project_NameKey toProto(Project.NameKey nameKey) { + return Reviewdb.Project_NameKey.newBuilder().setName(nameKey.get()).build(); + } + + @Override + public Project.NameKey fromProto(Reviewdb.Project_NameKey proto) { + return new Project.NameKey(proto.getName()); + } + + @Override + public Parser getParser() { + return Reviewdb.Project_NameKey.parser(); + } +} diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java index af44ea7a97..d8abf80ff7 100644 --- a/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/java/com/google/gerrit/server/index/change/ChangeField.java @@ -23,7 +23,6 @@ import static com.google.gerrit.index.FieldDef.integer; import static com.google.gerrit.index.FieldDef.prefix; import static com.google.gerrit.index.FieldDef.storedOnly; import static com.google.gerrit.index.FieldDef.timestamp; -import static com.google.gerrit.reviewdb.server.ReviewDbCodecs.CHANGE_CODEC; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toSet; @@ -53,6 +52,7 @@ 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.RefNames; +import com.google.gerrit.reviewdb.converter.ChangeProtoConverter; import com.google.gerrit.reviewdb.converter.PatchSetApprovalProtoConverter; import com.google.gerrit.reviewdb.converter.PatchSetProtoConverter; import com.google.gerrit.reviewdb.converter.ProtoConverter; @@ -468,7 +468,8 @@ public class ChangeField { /** Serialized change object, used for pre-populating results. */ public static final FieldDef CHANGE = - storedOnly("_change").build(changeGetter(CHANGE_CODEC::encodeToByteArray)); + storedOnly("_change") + .build(changeGetter(change -> toProto(ChangeProtoConverter.INSTANCE, change))); /** Serialized approvals for the current patch set, used for pre-populating results. */ public static final FieldDef> APPROVAL = @@ -854,11 +855,11 @@ public class ChangeField { } private static List toProtos(ProtoConverter converter, Collection objects) { - return objects - .stream() - .map(converter::toProto) - .map(Protos::toByteArray) - .collect(toImmutableList()); + return objects.stream().map(object -> toProto(converter, object)).collect(toImmutableList()); + } + + private static byte[] toProto(ProtoConverter converter, T object) { + return Protos.toByteArray(converter.toProto(object)); } private static FieldDef.Getter changeGetter(Function func) { diff --git a/javatests/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverterTest.java new file mode 100644 index 0000000000..9b4b49476b --- /dev/null +++ b/javatests/com/google/gerrit/reviewdb/converter/BranchNameKeyProtoConverterTest.java @@ -0,0 +1,83 @@ +// 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 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.reviewdb.Reviewdb; +import com.google.gerrit.proto.testing.SerializedClassSubject; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Project; +import com.google.protobuf.Parser; +import java.lang.reflect.Type; +import org.junit.Test; + +public class BranchNameKeyProtoConverterTest { + private final BranchNameKeyProtoConverter branchNameKeyProtoConverter = + BranchNameKeyProtoConverter.INSTANCE; + + @Test + public void allValuesConvertedToProto() { + Branch.NameKey nameKey = new Branch.NameKey(new Project.NameKey("project-13"), "branch-72"); + + Reviewdb.Branch_NameKey proto = branchNameKeyProtoConverter.toProto(nameKey); + + Reviewdb.Branch_NameKey expectedProto = + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project-13")) + .setBranchName("refs/heads/branch-72") + .build(); + assertThat(proto).isEqualTo(expectedProto); + } + + @Test + public void allValuesConvertedToProtoAndBackAgain() { + Branch.NameKey nameKey = new Branch.NameKey(new Project.NameKey("project-52"), "branch 14"); + + Branch.NameKey convertedNameKey = + branchNameKeyProtoConverter.fromProto(branchNameKeyProtoConverter.toProto(nameKey)); + + assertThat(convertedNameKey).isEqualTo(nameKey); + } + + @Test + public void protoCanBeParsedFromBytes() throws Exception { + Reviewdb.Branch_NameKey proto = + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 1")) + .setBranchName("branch 36") + .build(); + byte[] bytes = proto.toByteArray(); + + Parser parser = branchNameKeyProtoConverter.getParser(); + Reviewdb.Branch_NameKey parsedProto = parser.parseFrom(bytes); + + assertThat(parsedProto).isEqualTo(proto); + } + + /** See {@link SerializedClassSubject} for background and what to do if this test fails. */ + @Test + public void fieldsExistAsExpected() { + assertThatSerializedClass(Branch.NameKey.class) + .hasFields( + ImmutableMap.builder() + .put("projectName", Project.NameKey.class) + .put("branchName", String.class) + .build()); + } +} diff --git a/javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java b/javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java new file mode 100644 index 0000000000..1b2cf1cf4b --- /dev/null +++ b/javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java @@ -0,0 +1,123 @@ +// 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 static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.proto.Protos; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gwtorm.protobuf.CodecFactory; +import com.google.gwtorm.protobuf.ProtobufCodec; +import java.sql.Timestamp; +import org.junit.Test; + +// TODO(aliceks): Delete after proving binary compatibility. +public class ChangeConverterCompatibilityTest { + + private final ProtobufCodec changeCodec = CodecFactory.encoder(Change.class); + private final ChangeProtoConverter changeProtoConverter = ChangeProtoConverter.INSTANCE; + + @Test + public void changeIndexFieldWithAllValuesIsBinaryCompatible() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + change.setLastUpdatedOn(new Timestamp(1234567L)); + change.setStatus(Change.Status.MERGED); + change.setCurrentPatchSet( + new PatchSet.Id(new Change.Id(14), 23), "subject XYZ", "original subject ABC"); + change.setTopic("my topic"); + change.setSubmissionId("submission ID 234"); + change.setAssignee(new Account.Id(100001)); + change.setPrivate(true); + change.setWorkInProgress(true); + change.setReviewStarted(true); + change.setRevertOf(new Change.Id(180)); + + byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); + byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); + + assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); + } + + @Test + public void changeIndexFieldWithMandatoryValuesIsBinaryCompatible() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + + byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); + byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); + + assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); + } + + @Test + public void changeIndexFieldWithSubjectButNotOriginalSubjectIsBinaryCompatible() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + change.setCurrentPatchSet(new PatchSet.Id(new Change.Id(14), 23), "subject XYZ", null); + + byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); + byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); + + assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); + } + + @Test + public void changeIndexFieldWithoutPatchSetIsBinaryCompatible() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + // O for patch set ID means that there isn't any current patch set. + change.setCurrentPatchSet(new PatchSet.Id(new Change.Id(14), 0), null, null); + + byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); + byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); + + assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); + } + + // Copied from ChangeField. + private static byte[] convertToProto_old(ProtobufCodec codec, T object) { + return codec.encodeToByteArray(object); + } + + // Copied from ChangeField. + private static byte[] convertToProto_new(ProtoConverter converter, T object) { + return Protos.toByteArray(converter.toProto(object)); + } +} diff --git a/javatests/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverterTest.java new file mode 100644 index 0000000000..b52844906a --- /dev/null +++ b/javatests/com/google/gerrit/reviewdb/converter/ChangeKeyProtoConverterTest.java @@ -0,0 +1,67 @@ +// 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 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.reviewdb.Reviewdb; +import com.google.gerrit.proto.testing.SerializedClassSubject; +import com.google.gerrit.reviewdb.client.Change; +import com.google.protobuf.Parser; +import org.junit.Test; + +public class ChangeKeyProtoConverterTest { + private final ChangeKeyProtoConverter changeKeyProtoConverter = ChangeKeyProtoConverter.INSTANCE; + + @Test + public void allValuesConvertedToProto() { + Change.Key changeKey = new Change.Key("change-1"); + + Reviewdb.Change_Key proto = changeKeyProtoConverter.toProto(changeKey); + + Reviewdb.Change_Key expectedProto = Reviewdb.Change_Key.newBuilder().setId("change-1").build(); + assertThat(proto).isEqualTo(expectedProto); + } + + @Test + public void allValuesConvertedToProtoAndBackAgain() { + Change.Key changeKey = new Change.Key("change-52"); + + Change.Key convertedChangeKey = + changeKeyProtoConverter.fromProto(changeKeyProtoConverter.toProto(changeKey)); + + assertThat(convertedChangeKey).isEqualTo(changeKey); + } + + @Test + public void protoCanBeParsedFromBytes() throws Exception { + Reviewdb.Change_Key proto = Reviewdb.Change_Key.newBuilder().setId("change 36").build(); + byte[] bytes = proto.toByteArray(); + + Parser parser = changeKeyProtoConverter.getParser(); + Reviewdb.Change_Key parsedProto = parser.parseFrom(bytes); + + assertThat(parsedProto).isEqualTo(proto); + } + + /** See {@link SerializedClassSubject} for background and what to do if this test fails. */ + @Test + public void fieldsExistAsExpected() { + assertThatSerializedClass(Change.Key.class).hasFields(ImmutableMap.of("id", String.class)); + } +} diff --git a/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java new file mode 100644 index 0000000000..f897b0fe91 --- /dev/null +++ b/javatests/com/google/gerrit/reviewdb/converter/ChangeProtoConverterTest.java @@ -0,0 +1,363 @@ +// 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 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.reviewdb.Reviewdb; +import com.google.gerrit.proto.testing.SerializedClassSubject; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.reviewdb.client.Project; +import com.google.protobuf.Parser; +import java.lang.reflect.Type; +import java.sql.Timestamp; +import org.junit.Test; + +public class ChangeProtoConverterTest { + private final ChangeProtoConverter changeProtoConverter = ChangeProtoConverter.INSTANCE; + + @Test + public void allValuesConvertedToProto() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch 74"), + new Timestamp(987654L)); + change.setLastUpdatedOn(new Timestamp(1234567L)); + change.setStatus(Change.Status.MERGED); + change.setCurrentPatchSet( + new PatchSet.Id(new Change.Id(14), 23), "subject XYZ", "original subject ABC"); + change.setTopic("my topic"); + change.setSubmissionId("submission ID 234"); + change.setAssignee(new Account.Id(100001)); + change.setPrivate(true); + change.setWorkInProgress(true); + change.setReviewStarted(true); + change.setRevertOf(new Change.Id(180)); + + Reviewdb.Change proto = changeProtoConverter.toProto(change); + + Reviewdb.Change expectedProto = + Reviewdb.Change.newBuilder() + .setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)) + .setChangeKey(Reviewdb.Change_Key.newBuilder().setId("change 1")) + .setRowVersion(0) + .setCreatedOn(987654L) + .setLastUpdatedOn(1234567L) + .setOwnerAccountId(Reviewdb.Account_Id.newBuilder().setId(35)) + .setDest( + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 67")) + .setBranchName("refs/heads/branch 74")) + .setStatus(Change.STATUS_MERGED) + .setCurrentPatchSetId(23) + .setSubject("subject XYZ") + .setTopic("my topic") + .setOriginalSubject("original subject ABC") + .setSubmissionId("submission ID 234") + .setAssignee(Reviewdb.Account_Id.newBuilder().setId(100001)) + .setIsPrivate(true) + .setWorkInProgress(true) + .setReviewStarted(true) + .setRevertOf(Reviewdb.Change_Id.newBuilder().setId(180)) + .build(); + assertThat(proto).isEqualTo(expectedProto); + } + + @Test + public void mandatoryValuesConvertedToProto() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + + Reviewdb.Change proto = changeProtoConverter.toProto(change); + + Reviewdb.Change expectedProto = + Reviewdb.Change.newBuilder() + .setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)) + .setChangeKey(Reviewdb.Change_Key.newBuilder().setId("change 1")) + .setCreatedOn(987654L) + // Defaults to createdOn if not set. + .setLastUpdatedOn(987654L) + .setOwnerAccountId(Reviewdb.Account_Id.newBuilder().setId(35)) + .setDest( + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 67")) + .setBranchName("refs/heads/branch-74")) + // Default values which can't be unset. + .setCurrentPatchSetId(0) + .setRowVersion(0) + .setStatus(Change.STATUS_NEW) + .setIsPrivate(false) + .setWorkInProgress(false) + .setReviewStarted(false) + .build(); + assertThat(proto).isEqualTo(expectedProto); + } + + // This test documents a special behavior which is necessary to ensure binary compatibility. + @Test + public void currentPatchSetIsAlwaysSetWhenConvertedToProto() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + // O as ID actually means that no current patch set is present. + change.setCurrentPatchSet(new PatchSet.Id(new Change.Id(14), 0), null, null); + + Reviewdb.Change proto = changeProtoConverter.toProto(change); + + Reviewdb.Change expectedProto = + Reviewdb.Change.newBuilder() + .setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)) + .setChangeKey(Reviewdb.Change_Key.newBuilder().setId("change 1")) + .setCreatedOn(987654L) + // Defaults to createdOn if not set. + .setLastUpdatedOn(987654L) + .setOwnerAccountId(Reviewdb.Account_Id.newBuilder().setId(35)) + .setDest( + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 67")) + .setBranchName("refs/heads/branch-74")) + .setCurrentPatchSetId(0) + // Default values which can't be unset. + .setRowVersion(0) + .setStatus(Change.STATUS_NEW) + .setIsPrivate(false) + .setWorkInProgress(false) + .setReviewStarted(false) + .build(); + assertThat(proto).isEqualTo(expectedProto); + } + + // This test documents a special behavior which is necessary to ensure binary compatibility. + @Test + public void originalSubjectIsNotAutomaticallySetToSubjectWhenConvertedToProto() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + change.setCurrentPatchSet(new PatchSet.Id(new Change.Id(14), 23), "subject ABC", null); + + Reviewdb.Change proto = changeProtoConverter.toProto(change); + + Reviewdb.Change expectedProto = + Reviewdb.Change.newBuilder() + .setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)) + .setChangeKey(Reviewdb.Change_Key.newBuilder().setId("change 1")) + .setCreatedOn(987654L) + // Defaults to createdOn if not set. + .setLastUpdatedOn(987654L) + .setOwnerAccountId(Reviewdb.Account_Id.newBuilder().setId(35)) + .setDest( + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 67")) + .setBranchName("refs/heads/branch-74")) + .setCurrentPatchSetId(23) + .setSubject("subject ABC") + // Default values which can't be unset. + .setRowVersion(0) + .setStatus(Change.STATUS_NEW) + .setIsPrivate(false) + .setWorkInProgress(false) + .setReviewStarted(false) + .build(); + assertThat(proto).isEqualTo(expectedProto); + } + + @Test + public void allValuesConvertedToProtoAndBackAgain() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + change.setLastUpdatedOn(new Timestamp(1234567L)); + change.setStatus(Change.Status.MERGED); + change.setCurrentPatchSet( + new PatchSet.Id(new Change.Id(14), 23), "subject XYZ", "original subject ABC"); + change.setTopic("my topic"); + change.setSubmissionId("submission ID 234"); + change.setAssignee(new Account.Id(100001)); + change.setPrivate(true); + change.setWorkInProgress(true); + change.setReviewStarted(true); + change.setRevertOf(new Change.Id(180)); + + Change convertedChange = changeProtoConverter.fromProto(changeProtoConverter.toProto(change)); + assertEqualChange(convertedChange, change); + } + + @Test + public void mandatoryValuesConvertedToProtoAndBackAgain() { + Change change = + new Change( + new Change.Key("change 1"), + new Change.Id(14), + new Account.Id(35), + new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), + new Timestamp(987654L)); + + Change convertedChange = changeProtoConverter.fromProto(changeProtoConverter.toProto(change)); + assertEqualChange(convertedChange, change); + } + + // We need this special test as some values are only optional in the protobuf definition but can + // never be unset in our entity object. + @Test + public void protoWithOnlyRequiredValuesCanBeConvertedBack() { + Reviewdb.Change proto = + Reviewdb.Change.newBuilder().setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)).build(); + Change change = changeProtoConverter.fromProto(proto); + + assertThat(change.getChangeId()).isEqualTo(14); + // Values which can't be null according to ReviewDb's column definition but which are optional. + assertThat(change.getKey()).isNull(); + assertThat(change.getOwner()).isNull(); + assertThat(change.getDest()).isNull(); + assertThat(change.getCreatedOn()).isEqualTo(new Timestamp(0)); + assertThat(change.getLastUpdatedOn()).isEqualTo(new Timestamp(0)); + assertThat(change.getSubject()).isNull(); + assertThat(change.currentPatchSetId()).isNull(); + // Default values for unset protobuf fields which can't be unset in the entity object. + assertThat(change.getRowVersion()).isEqualTo(0); + assertThat(change.getStatus()).isEqualTo(Change.Status.NEW); + assertThat(change.isPrivate()).isFalse(); + assertThat(change.isWorkInProgress()).isFalse(); + assertThat(change.hasReviewStarted()).isFalse(); + } + + @Test + public void unsetLastUpdatedOnIsAutomaticallySetToCreatedOnWhenConvertedBack() { + Reviewdb.Change proto = + Reviewdb.Change.newBuilder() + .setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)) + .setChangeKey(Reviewdb.Change_Key.newBuilder().setId("change 1")) + .setCreatedOn(987654L) + .setOwnerAccountId(Reviewdb.Account_Id.newBuilder().setId(35)) + .setDest( + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 67")) + .setBranchName("branch 74")) + .build(); + Change change = changeProtoConverter.fromProto(proto); + + assertThat(change.getLastUpdatedOn()).isEqualTo(new Timestamp(987654L)); + } + + @Test + public void protoCanBeParsedFromBytes() throws Exception { + Reviewdb.Change proto = + Reviewdb.Change.newBuilder() + .setChangeId(Reviewdb.Change_Id.newBuilder().setId(14)) + .setChangeKey(Reviewdb.Change_Key.newBuilder().setId("change 1")) + .setRowVersion(0) + .setCreatedOn(987654L) + .setLastUpdatedOn(1234567L) + .setOwnerAccountId(Reviewdb.Account_Id.newBuilder().setId(35)) + .setDest( + Reviewdb.Branch_NameKey.newBuilder() + .setProjectName(Reviewdb.Project_NameKey.newBuilder().setName("project 67")) + .setBranchName("branch 74")) + .setStatus(Change.STATUS_MERGED) + .setCurrentPatchSetId(23) + .setSubject("subject XYZ") + .setTopic("my topic") + .setOriginalSubject("original subject ABC") + .setSubmissionId("submission ID 234") + .setAssignee(Reviewdb.Account_Id.newBuilder().setId(100001)) + .setIsPrivate(true) + .setWorkInProgress(true) + .setReviewStarted(true) + .setRevertOf(Reviewdb.Change_Id.newBuilder().setId(180)) + .build(); + byte[] bytes = proto.toByteArray(); + + Parser parser = changeProtoConverter.getParser(); + Reviewdb.Change parsedProto = parser.parseFrom(bytes); + + assertThat(parsedProto).isEqualTo(proto); + } + + /** See {@link SerializedClassSubject} for background and what to do if this test fails. */ + @Test + public void fieldsExistAsExpected() { + assertThatSerializedClass(Change.class) + .hasFields( + ImmutableMap.builder() + .put("changeId", Change.Id.class) + .put("changeKey", Change.Key.class) + .put("rowVersion", int.class) + .put("createdOn", Timestamp.class) + .put("lastUpdatedOn", Timestamp.class) + .put("owner", Account.Id.class) + .put("dest", Branch.NameKey.class) + .put("status", char.class) + .put("currentPatchSetId", int.class) + .put("subject", String.class) + .put("topic", String.class) + .put("originalSubject", String.class) + .put("submissionId", String.class) + .put("assignee", Account.Id.class) + .put("isPrivate", boolean.class) + .put("workInProgress", boolean.class) + .put("reviewStarted", boolean.class) + .put("revertOf", Change.Id.class) + .build()); + } + + // Unfortunately, Change doesn't implement equals(). Remove this method when we switch Change to + // an AutoValue. + private static void assertEqualChange(Change change, Change expectedChange) { + assertThat(change.getChangeId()).isEqualTo(expectedChange.getChangeId()); + assertThat(change.getKey()).isEqualTo(expectedChange.getKey()); + assertThat(change.getRowVersion()).isEqualTo(expectedChange.getRowVersion()); + assertThat(change.getCreatedOn()).isEqualTo(expectedChange.getCreatedOn()); + assertThat(change.getLastUpdatedOn()).isEqualTo(expectedChange.getLastUpdatedOn()); + assertThat(change.getOwner()).isEqualTo(expectedChange.getOwner()); + assertThat(change.getDest()).isEqualTo(expectedChange.getDest()); + assertThat(change.getStatus()).isEqualTo(expectedChange.getStatus()); + assertThat(change.currentPatchSetId()).isEqualTo(expectedChange.currentPatchSetId()); + assertThat(change.getSubject()).isEqualTo(expectedChange.getSubject()); + assertThat(change.getTopic()).isEqualTo(expectedChange.getTopic()); + assertThat(change.getOriginalSubject()).isEqualTo(expectedChange.getOriginalSubject()); + assertThat(change.getSubmissionId()).isEqualTo(expectedChange.getSubmissionId()); + assertThat(change.getAssignee()).isEqualTo(expectedChange.getAssignee()); + assertThat(change.isPrivate()).isEqualTo(expectedChange.isPrivate()); + assertThat(change.isWorkInProgress()).isEqualTo(expectedChange.isWorkInProgress()); + assertThat(change.hasReviewStarted()).isEqualTo(expectedChange.hasReviewStarted()); + assertThat(change.getRevertOf()).isEqualTo(expectedChange.getRevertOf()); + } +} diff --git a/javatests/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverterTest.java new file mode 100644 index 0000000000..acea8f6818 --- /dev/null +++ b/javatests/com/google/gerrit/reviewdb/converter/ProjectNameKeyProtoConverterTest.java @@ -0,0 +1,71 @@ +// 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 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.reviewdb.Reviewdb; +import com.google.gerrit.proto.testing.SerializedClassSubject; +import com.google.gerrit.reviewdb.client.Project; +import com.google.protobuf.Parser; +import org.junit.Test; + +public class ProjectNameKeyProtoConverterTest { + private final ProjectNameKeyProtoConverter projectNameKeyProtoConverter = + ProjectNameKeyProtoConverter.INSTANCE; + + @Test + public void allValuesConvertedToProto() { + Project.NameKey nameKey = new Project.NameKey("project-72"); + + Reviewdb.Project_NameKey proto = projectNameKeyProtoConverter.toProto(nameKey); + + Reviewdb.Project_NameKey expectedProto = + Reviewdb.Project_NameKey.newBuilder().setName("project-72").build(); + assertThat(proto).isEqualTo(expectedProto); + } + + @Test + public void allValuesConvertedToProtoAndBackAgain() { + Project.NameKey nameKey = new Project.NameKey("project-52"); + + Project.NameKey convertedNameKey = + projectNameKeyProtoConverter.fromProto(projectNameKeyProtoConverter.toProto(nameKey)); + + assertThat(convertedNameKey).isEqualTo(nameKey); + } + + @Test + public void protoCanBeParsedFromBytes() throws Exception { + Reviewdb.Project_NameKey proto = + Reviewdb.Project_NameKey.newBuilder().setName("project 36").build(); + byte[] bytes = proto.toByteArray(); + + Parser parser = projectNameKeyProtoConverter.getParser(); + Reviewdb.Project_NameKey parsedProto = parser.parseFrom(bytes); + + assertThat(parsedProto).isEqualTo(proto); + } + + /** See {@link SerializedClassSubject} for background and what to do if this test fails. */ + @Test + public void fieldsExistAsExpected() { + assertThatSerializedClass(Project.NameKey.class) + .hasFields(ImmutableMap.of("name", String.class)); + } +} From 74a97c4a0c31259e919674990a50a23440156ad2 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Dec 2018 15:51:29 +0100 Subject: [PATCH 2/6] Remove tests ensuring generation of reviewdb.proto Since we don't generate the reviewdb.proto file anymore, we also don't need tests which ensure that the generation worked correctly. The hand-written reviewdb.proto file is covered via other tests. Change-Id: I398eab062f9d12f0d85345df38cc653114afa46c --- javatests/com/google/gerrit/proto/BUILD | 2 -- .../gerrit/proto/ReviewDbProtoTest.java | 32 ------------------- 2 files changed, 34 deletions(-) delete mode 100644 javatests/com/google/gerrit/proto/ReviewDbProtoTest.java diff --git a/javatests/com/google/gerrit/proto/BUILD b/javatests/com/google/gerrit/proto/BUILD index c7d3acaf66..6b98b72a50 100644 --- a/javatests/com/google/gerrit/proto/BUILD +++ b/javatests/com/google/gerrit/proto/BUILD @@ -6,11 +6,9 @@ junit_tests( deps = [ "//java/com/google/gerrit/proto", "//java/com/google/gerrit/testing:gerrit-test-util", - "//lib:guava", "//lib:protobuf", "//lib/truth", "//lib/truth:truth-proto-extension", "//proto:cache_java_proto", - "//proto:reviewdb_java_proto", ], ) diff --git a/javatests/com/google/gerrit/proto/ReviewDbProtoTest.java b/javatests/com/google/gerrit/proto/ReviewDbProtoTest.java deleted file mode 100644 index d6ffee51a0..0000000000 --- a/javatests/com/google/gerrit/proto/ReviewDbProtoTest.java +++ /dev/null @@ -1,32 +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.proto; - -import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat; - -import com.google.gerrit.proto.reviewdb.Reviewdb.Change; -import com.google.gerrit.proto.reviewdb.Reviewdb.Change_Id; -import com.google.gerrit.testing.GerritBaseTests; -import org.junit.Test; - -public class ReviewDbProtoTest extends GerritBaseTests { - @Test - public void generatedProtoApi() { - Change c1 = Change.newBuilder().setChangeId(Change_Id.newBuilder().setId(1234).build()).build(); - Change c2 = Change.newBuilder().setChangeId(Change_Id.newBuilder().setId(5678).build()).build(); - assertThat(c1).isEqualTo(c1); - assertThat(c1).isNotEqualTo(c2); - } -} From 1c92aa6f8d6c3fec0274d6bc43ea1a5bed8849b0 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Dec 2018 15:53:45 +0100 Subject: [PATCH 3/6] Remove ProtoGen We don't generate reviewdb.proto anymore. Hence, we can remove the generator. Change-Id: I7dd9d6b82824823e140be90f186a563ff1d17443 --- java/com/google/gerrit/proto/BUILD | 15 ---- java/com/google/gerrit/proto/ProtoGen.java | 84 ---------------------- 2 files changed, 99 deletions(-) delete mode 100644 java/com/google/gerrit/proto/ProtoGen.java diff --git a/java/com/google/gerrit/proto/BUILD b/java/com/google/gerrit/proto/BUILD index b831e92721..703aa4e075 100644 --- a/java/com/google/gerrit/proto/BUILD +++ b/java/com/google/gerrit/proto/BUILD @@ -1,18 +1,3 @@ -java_binary( - name = "ProtoGen", - srcs = ["ProtoGen.java"], - resource_strip_prefix = "resources", - resources = ["//resources/com/google/gerrit/proto"], - visibility = ["//proto:__pkg__"], - deps = [ - "//java/com/google/gerrit/reviewdb:server", - "//lib:args4j", - "//lib:guava", - "//lib:gwtorm", - "//lib/jgit/org.eclipse.jgit:jgit", - ], -) - java_library( name = "proto", srcs = ["Protos.java"], diff --git a/java/com/google/gerrit/proto/ProtoGen.java b/java/com/google/gerrit/proto/ProtoGen.java deleted file mode 100644 index c1302416e3..0000000000 --- a/java/com/google/gerrit/proto/ProtoGen.java +++ /dev/null @@ -1,84 +0,0 @@ -// Copyright (C) 2010 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.proto; - -import static com.google.common.base.Preconditions.checkState; -import static java.nio.charset.StandardCharsets.UTF_8; - -import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gwtorm.schema.java.JavaSchemaModel; -import java.io.BufferedWriter; -import java.io.File; -import java.io.InputStream; -import java.io.OutputStream; -import java.io.OutputStreamWriter; -import java.io.PrintWriter; -import java.nio.ByteBuffer; -import org.eclipse.jgit.internal.storage.file.LockFile; -import org.eclipse.jgit.util.IO; -import org.kohsuke.args4j.CmdLineException; -import org.kohsuke.args4j.CmdLineParser; -import org.kohsuke.args4j.Option; -import org.kohsuke.args4j.ParserProperties; - -public class ProtoGen { - @Option( - name = "--output", - aliases = {"-o"}, - required = true, - metaVar = "FILE", - usage = "File to write .proto into") - private File file; - - public static void main(String[] argv) throws Exception { - System.exit(new ProtoGen().run(argv)); - } - - private int run(String[] argv) throws Exception { - CmdLineParser parser = new CmdLineParser(this, ParserProperties.defaults().withAtSyntax(false)); - try { - parser.parseArgument(argv); - } catch (CmdLineException e) { - System.err.println(e.getMessage()); - System.err.println(getClass().getSimpleName() + " -o output.proto"); - parser.printUsage(System.err); - return 1; - } - - LockFile lock = new LockFile(file.getAbsoluteFile()); - checkState(lock.lock(), "cannot lock %s", file); - try { - JavaSchemaModel jsm = new JavaSchemaModel(ReviewDb.class); - try (OutputStream o = lock.getOutputStream(); - PrintWriter out = new PrintWriter(new BufferedWriter(new OutputStreamWriter(o, UTF_8)))) { - String header; - try (InputStream in = getClass().getResourceAsStream("ProtoGenHeader.txt")) { - ByteBuffer buf = IO.readWholeStream(in, 1024); - int ptr = buf.arrayOffset() + buf.position(); - int len = buf.remaining(); - header = new String(buf.array(), ptr, len, UTF_8); - } - - out.write(header); - jsm.generateProto(out); - out.flush(); - } - checkState(lock.commit(), "Could not write to %s", file); - } finally { - lock.unlock(); - } - return 0; - } -} From ab80c219a464829d094bd4028ffa7d3522e9ae2e Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Dec 2018 15:57:01 +0100 Subject: [PATCH 4/6] Remove binary compatibility tests for proto converters The tests served their purpose. Remove them to cut another dependency on gwtorm. Change-Id: I750756ed050aa97c6a8c576e8090334f1ea87b8d --- .../google/gerrit/reviewdb/converter/BUILD | 20 +- .../ChangeConverterCompatibilityTest.java | 123 ----------- ...angeMessageConverterCompatibilityTest.java | 195 ------------------ ...SetApprovalConverterCompatibilityTest.java | 166 --------------- .../PatchSetConverterCompatibilityTest.java | 137 ------------ 5 files changed, 1 insertion(+), 640 deletions(-) delete mode 100644 javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java delete mode 100644 javatests/com/google/gerrit/reviewdb/converter/ChangeMessageConverterCompatibilityTest.java delete mode 100644 javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalConverterCompatibilityTest.java delete mode 100644 javatests/com/google/gerrit/reviewdb/converter/PatchSetConverterCompatibilityTest.java diff --git a/javatests/com/google/gerrit/reviewdb/converter/BUILD b/javatests/com/google/gerrit/reviewdb/converter/BUILD index 7c159100ba..72939e3a2a 100644 --- a/javatests/com/google/gerrit/reviewdb/converter/BUILD +++ b/javatests/com/google/gerrit/reviewdb/converter/BUILD @@ -1,13 +1,8 @@ load("//tools/bzl:junit.bzl", "junit_tests") -COMPATIBLITY_TEST_SRCS = glob(["*CompatibilityTest.java"]) - junit_tests( name = "proto_converter_tests", - srcs = glob( - ["*.java"], - exclude = COMPATIBLITY_TEST_SRCS, - ), + srcs = glob(["*.java"]), deps = [ "//java/com/google/gerrit/proto/testing", "//java/com/google/gerrit/reviewdb:server", @@ -18,16 +13,3 @@ junit_tests( "//proto:reviewdb_java_proto", ], ) - -junit_tests( - name = "compatibility_tests", - srcs = COMPATIBLITY_TEST_SRCS, - deps = [ - "//java/com/google/gerrit/proto", - "//java/com/google/gerrit/reviewdb:server", - "//lib:guava", - "//lib:gwtorm-client", - "//lib:protobuf", - "//lib/truth", - ], -) diff --git a/javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java b/javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java deleted file mode 100644 index 1b2cf1cf4b..0000000000 --- a/javatests/com/google/gerrit/reviewdb/converter/ChangeConverterCompatibilityTest.java +++ /dev/null @@ -1,123 +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 static com.google.common.truth.Truth.assertThat; - -import com.google.gerrit.proto.Protos; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Branch; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.Project; -import com.google.gwtorm.protobuf.CodecFactory; -import com.google.gwtorm.protobuf.ProtobufCodec; -import java.sql.Timestamp; -import org.junit.Test; - -// TODO(aliceks): Delete after proving binary compatibility. -public class ChangeConverterCompatibilityTest { - - private final ProtobufCodec changeCodec = CodecFactory.encoder(Change.class); - private final ChangeProtoConverter changeProtoConverter = ChangeProtoConverter.INSTANCE; - - @Test - public void changeIndexFieldWithAllValuesIsBinaryCompatible() { - Change change = - new Change( - new Change.Key("change 1"), - new Change.Id(14), - new Account.Id(35), - new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), - new Timestamp(987654L)); - change.setLastUpdatedOn(new Timestamp(1234567L)); - change.setStatus(Change.Status.MERGED); - change.setCurrentPatchSet( - new PatchSet.Id(new Change.Id(14), 23), "subject XYZ", "original subject ABC"); - change.setTopic("my topic"); - change.setSubmissionId("submission ID 234"); - change.setAssignee(new Account.Id(100001)); - change.setPrivate(true); - change.setWorkInProgress(true); - change.setReviewStarted(true); - change.setRevertOf(new Change.Id(180)); - - byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); - byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeIndexFieldWithMandatoryValuesIsBinaryCompatible() { - Change change = - new Change( - new Change.Key("change 1"), - new Change.Id(14), - new Account.Id(35), - new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), - new Timestamp(987654L)); - - byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); - byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeIndexFieldWithSubjectButNotOriginalSubjectIsBinaryCompatible() { - Change change = - new Change( - new Change.Key("change 1"), - new Change.Id(14), - new Account.Id(35), - new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), - new Timestamp(987654L)); - change.setCurrentPatchSet(new PatchSet.Id(new Change.Id(14), 23), "subject XYZ", null); - - byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); - byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeIndexFieldWithoutPatchSetIsBinaryCompatible() { - Change change = - new Change( - new Change.Key("change 1"), - new Change.Id(14), - new Account.Id(35), - new Branch.NameKey(new Project.NameKey("project 67"), "branch-74"), - new Timestamp(987654L)); - // O for patch set ID means that there isn't any current patch set. - change.setCurrentPatchSet(new PatchSet.Id(new Change.Id(14), 0), null, null); - - byte[] resultOfOldConverter = convertToProto_old(changeCodec, change); - byte[] resultOfNewConverter = convertToProto_new(changeProtoConverter, change); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - // Copied from ChangeField. - private static byte[] convertToProto_old(ProtobufCodec codec, T object) { - return codec.encodeToByteArray(object); - } - - // Copied from ChangeField. - private static byte[] convertToProto_new(ProtoConverter converter, T object) { - return Protos.toByteArray(converter.toProto(object)); - } -} diff --git a/javatests/com/google/gerrit/reviewdb/converter/ChangeMessageConverterCompatibilityTest.java b/javatests/com/google/gerrit/reviewdb/converter/ChangeMessageConverterCompatibilityTest.java deleted file mode 100644 index a194ec6e57..0000000000 --- a/javatests/com/google/gerrit/reviewdb/converter/ChangeMessageConverterCompatibilityTest.java +++ /dev/null @@ -1,195 +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 static com.google.common.collect.ImmutableList.toImmutableList; -import static com.google.common.collect.Iterables.getOnlyElement; -import static com.google.common.truth.Truth.assertThat; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Lists; -import com.google.gerrit.proto.Protos; -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.PatchSet; -import com.google.gwtorm.protobuf.CodecFactory; -import com.google.gwtorm.protobuf.ProtobufCodec; -import com.google.gwtorm.server.OrmException; -import com.google.protobuf.ByteString; -import com.google.protobuf.CodedOutputStream; -import com.google.protobuf.MessageLite; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.sql.Timestamp; -import java.util.Collection; -import java.util.List; -import org.junit.Test; - -// TODO(aliceks): Delete after proving binary compatibility. -public class ChangeMessageConverterCompatibilityTest { - - private final ProtobufCodec changeMessageCodec = - CodecFactory.encoder(ChangeMessage.class); - private final ChangeMessageProtoConverter changeMessageProtoConverter = - ChangeMessageProtoConverter.INSTANCE; - - @Test - public void changeIndexFieldWithAllValuesIsBinaryCompatible() throws Exception { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), - new Account.Id(63), - new Timestamp(9876543), - new PatchSet.Id(new Change.Id(34), 13)); - changeMessage.setMessage("This is a change message."); - changeMessage.setTag("An arbitrary tag."); - changeMessage.setRealAuthor(new Account.Id(10003)); - ImmutableList changeMessages = ImmutableList.of(changeMessage); - - byte[] resultOfOldConverter = - getOnlyElement(convertToProtos_old(changeMessageCodec, changeMessages)); - byte[] resultOfNewConverter = - getOnlyElement(convertToProtos_new(changeMessageProtoConverter, changeMessages)); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeIndexFieldWithMandatoryValuesIsBinaryCompatible() throws Exception { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), null, null, null); - ImmutableList changeMessages = ImmutableList.of(changeMessage); - - byte[] resultOfOldConverter = - getOnlyElement(convertToProtos_old(changeMessageCodec, changeMessages)); - byte[] resultOfNewConverter = - getOnlyElement(convertToProtos_new(changeMessageProtoConverter, changeMessages)); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithAllValuesIsBinaryCompatible() { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), - new Account.Id(63), - new Timestamp(9876543), - new PatchSet.Id(new Change.Id(34), 13)); - changeMessage.setMessage("This is a change message."); - changeMessage.setTag("An arbitrary tag."); - changeMessage.setRealAuthor(new Account.Id(10003)); - - ByteString resultOfOldConverter = Protos.toByteString(changeMessage, changeMessageCodec); - ByteString resultOfNewConverter = toByteString(changeMessage, changeMessageProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithMainValuesIsBinaryCompatible() { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), - new Account.Id(63), - new Timestamp(9876543), - new PatchSet.Id(new Change.Id(34), 13)); - - ByteString resultOfOldConverter = Protos.toByteString(changeMessage, changeMessageCodec); - ByteString resultOfNewConverter = toByteString(changeMessage, changeMessageProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithoutRealAuthorButAuthorIsBinaryCompatible() { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), - new Account.Id(63), - null, - null); - - ByteString resultOfOldConverter = Protos.toByteString(changeMessage, changeMessageCodec); - ByteString resultOfNewConverter = toByteString(changeMessage, changeMessageProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithoutSameRealAuthorAndAuthorIsBinaryCompatible() { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), - new Account.Id(63), - null, - null); - changeMessage.setRealAuthor(new Account.Id(63)); - - ByteString resultOfOldConverter = Protos.toByteString(changeMessage, changeMessageCodec); - ByteString resultOfNewConverter = toByteString(changeMessage, changeMessageProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithMandatoryValuesIsBinaryCompatible() { - ChangeMessage changeMessage = - new ChangeMessage( - new ChangeMessage.Key(new Change.Id(543), "change-message-21"), null, null, null); - - ByteString resultOfOldConverter = Protos.toByteString(changeMessage, changeMessageCodec); - ByteString resultOfNewConverter = toByteString(changeMessage, changeMessageProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - // Copied from ChangeField. - private static List convertToProtos_old(ProtobufCodec codec, Collection objs) - throws OrmException { - List result = Lists.newArrayListWithCapacity(objs.size()); - ByteArrayOutputStream out = new ByteArrayOutputStream(256); - try { - for (T obj : objs) { - out.reset(); - CodedOutputStream cos = CodedOutputStream.newInstance(out); - codec.encode(obj, cos); - cos.flush(); - result.add(out.toByteArray()); - } - } catch (IOException e) { - throw new OrmException(e); - } - return result; - } - - // Copied from ChangeField. - private static List convertToProtos_new( - ProtoConverter converter, Collection objects) { - return objects - .stream() - .map(converter::toProto) - .map(Protos::toByteArray) - .collect(toImmutableList()); - } - - // Copied from ChangeNotesState.Serializer. - private static ByteString toByteString(T object, ProtoConverter converter) { - MessageLite message = converter.toProto(object); - return Protos.toByteString(message); - } -} diff --git a/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalConverterCompatibilityTest.java b/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalConverterCompatibilityTest.java deleted file mode 100644 index 9da37dace4..0000000000 --- a/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalConverterCompatibilityTest.java +++ /dev/null @@ -1,166 +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 static com.google.common.collect.ImmutableList.toImmutableList; -import static com.google.common.collect.Iterables.getOnlyElement; -import static com.google.common.truth.Truth.assertThat; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Lists; -import com.google.gerrit.proto.Protos; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.LabelId; -import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.PatchSetApproval; -import com.google.gwtorm.protobuf.CodecFactory; -import com.google.gwtorm.protobuf.ProtobufCodec; -import com.google.gwtorm.server.OrmException; -import com.google.protobuf.ByteString; -import com.google.protobuf.CodedOutputStream; -import com.google.protobuf.MessageLite; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.util.Collection; -import java.util.Date; -import java.util.List; -import org.junit.Test; - -// TODO(aliceks): Delete after proving binary compatibility. -public class PatchSetApprovalConverterCompatibilityTest { - - private final ProtobufCodec patchSetApprovalCodec = - CodecFactory.encoder(PatchSetApproval.class); - private final PatchSetApprovalProtoConverter patchSetApprovalProtoConverter = - PatchSetApprovalProtoConverter.INSTANCE; - - @Test - public void changeIndexFieldWithAllValuesIsBinaryCompatible() throws Exception { - PatchSetApproval patchSetApproval = - new PatchSetApproval( - new PatchSetApproval.Key( - new PatchSet.Id(new Change.Id(42), 14), - new Account.Id(100013), - new LabelId("label-8")), - (short) 456, - new Date(987654L)); - patchSetApproval.setTag("tag-21"); - patchSetApproval.setRealAccountId(new Account.Id(612)); - patchSetApproval.setPostSubmit(true); - ImmutableList patchSetApprovals = ImmutableList.of(patchSetApproval); - - byte[] resultOfOldConverter = - getOnlyElement(convertToProtos_old(patchSetApprovalCodec, patchSetApprovals)); - byte[] resultOfNewConverter = - getOnlyElement(convertToProtos_new(patchSetApprovalProtoConverter, patchSetApprovals)); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeIndexFieldWithMandatoryValuesIsBinaryCompatible() throws Exception { - PatchSetApproval patchSetApproval = - new PatchSetApproval( - new PatchSetApproval.Key( - new PatchSet.Id(new Change.Id(42), 14), - new Account.Id(100013), - new LabelId("label-8")), - (short) 456, - new Date(987654L)); - ImmutableList patchSetApprovals = ImmutableList.of(patchSetApproval); - - byte[] resultOfOldConverter = - getOnlyElement(convertToProtos_old(patchSetApprovalCodec, patchSetApprovals)); - byte[] resultOfNewConverter = - getOnlyElement(convertToProtos_new(patchSetApprovalProtoConverter, patchSetApprovals)); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithAllValuesIsBinaryCompatible() { - PatchSetApproval patchSetApproval = - new PatchSetApproval( - new PatchSetApproval.Key( - new PatchSet.Id(new Change.Id(42), 14), - new Account.Id(100013), - new LabelId("label-8")), - (short) 456, - new Date(987654L)); - patchSetApproval.setTag("tag-21"); - patchSetApproval.setRealAccountId(new Account.Id(612)); - patchSetApproval.setPostSubmit(true); - - ByteString resultOfOldConverter = Protos.toByteString(patchSetApproval, patchSetApprovalCodec); - ByteString resultOfNewConverter = - toByteString(patchSetApproval, patchSetApprovalProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithMandatoryValuesIsBinaryCompatible() { - PatchSetApproval patchSetApproval = - new PatchSetApproval( - new PatchSetApproval.Key( - new PatchSet.Id(new Change.Id(42), 14), - new Account.Id(100013), - new LabelId("label-8")), - (short) 456, - new Date(987654L)); - - ByteString resultOfOldConverter = Protos.toByteString(patchSetApproval, patchSetApprovalCodec); - ByteString resultOfNewConverter = - toByteString(patchSetApproval, patchSetApprovalProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - // Copied from ChangeField. - private static List convertToProtos_old(ProtobufCodec codec, Collection objs) - throws OrmException { - List result = Lists.newArrayListWithCapacity(objs.size()); - ByteArrayOutputStream out = new ByteArrayOutputStream(256); - try { - for (T obj : objs) { - out.reset(); - CodedOutputStream cos = CodedOutputStream.newInstance(out); - codec.encode(obj, cos); - cos.flush(); - result.add(out.toByteArray()); - } - } catch (IOException e) { - throw new OrmException(e); - } - return result; - } - - // Copied from ChangeField. - private static List convertToProtos_new( - ProtoConverter converter, Collection objects) { - return objects - .stream() - .map(converter::toProto) - .map(Protos::toByteArray) - .collect(toImmutableList()); - } - - // Copied from ChangeNotesState.Serializer. - private static ByteString toByteString(T object, ProtoConverter converter) { - MessageLite message = converter.toProto(object); - return Protos.toByteString(message); - } -} diff --git a/javatests/com/google/gerrit/reviewdb/converter/PatchSetConverterCompatibilityTest.java b/javatests/com/google/gerrit/reviewdb/converter/PatchSetConverterCompatibilityTest.java deleted file mode 100644 index 8d8960cb97..0000000000 --- a/javatests/com/google/gerrit/reviewdb/converter/PatchSetConverterCompatibilityTest.java +++ /dev/null @@ -1,137 +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 static com.google.common.collect.ImmutableList.toImmutableList; -import static com.google.common.collect.Iterables.getOnlyElement; -import static com.google.common.truth.Truth.assertThat; - -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Lists; -import com.google.gerrit.proto.Protos; -import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.RevId; -import com.google.gwtorm.protobuf.CodecFactory; -import com.google.gwtorm.protobuf.ProtobufCodec; -import com.google.gwtorm.server.OrmException; -import com.google.protobuf.ByteString; -import com.google.protobuf.CodedOutputStream; -import com.google.protobuf.MessageLite; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.sql.Timestamp; -import java.util.Collection; -import java.util.List; -import org.junit.Test; - -// TODO(aliceks): Delete after proving binary compatibility. -public class PatchSetConverterCompatibilityTest { - - private final ProtobufCodec patchSetCodec = CodecFactory.encoder(PatchSet.class); - private final PatchSetProtoConverter patchSetProtoConverter = PatchSetProtoConverter.INSTANCE; - - @Test - public void changeIndexFieldWithAllValuesIsBinaryCompatible() throws Exception { - PatchSet patchSet = new PatchSet(new PatchSet.Id(new Change.Id(103), 73)); - patchSet.setRevision(new RevId("aabbccddeeff")); - patchSet.setUploader(new Account.Id(452)); - patchSet.setCreatedOn(new Timestamp(930349320L)); - patchSet.setGroups(ImmutableList.of("group1, group2")); - patchSet.setPushCertificate("my push certificate"); - patchSet.setDescription("This is a patch set description."); - ImmutableList patchSets = ImmutableList.of(patchSet); - - byte[] resultOfOldConverter = getOnlyElement(convertToProtos_old(patchSetCodec, patchSets)); - byte[] resultOfNewConverter = - getOnlyElement(convertToProtos_new(patchSetProtoConverter, patchSets)); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeIndexFieldWithMandatoryValuesIsBinaryCompatible() throws Exception { - PatchSet patchSet = new PatchSet(new PatchSet.Id(new Change.Id(103), 73)); - ImmutableList patchSets = ImmutableList.of(patchSet); - - byte[] resultOfOldConverter = getOnlyElement(convertToProtos_old(patchSetCodec, patchSets)); - byte[] resultOfNewConverter = - getOnlyElement(convertToProtos_new(patchSetProtoConverter, patchSets)); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithAllValuesIsBinaryCompatible() { - PatchSet patchSet = new PatchSet(new PatchSet.Id(new Change.Id(103), 73)); - patchSet.setRevision(new RevId("aabbccddeeff")); - patchSet.setUploader(new Account.Id(452)); - patchSet.setCreatedOn(new Timestamp(930349320L)); - patchSet.setGroups(ImmutableList.of("group1, group2")); - patchSet.setPushCertificate("my push certificate"); - patchSet.setDescription("This is a patch set description."); - - ByteString resultOfOldConverter = Protos.toByteString(patchSet, patchSetCodec); - ByteString resultOfNewConverter = toByteString(patchSet, patchSetProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - @Test - public void changeNotesFieldWithMandatoryValuesIsBinaryCompatible() { - PatchSet patchSet = new PatchSet(new PatchSet.Id(new Change.Id(103), 73)); - - ByteString resultOfOldConverter = Protos.toByteString(patchSet, patchSetCodec); - ByteString resultOfNewConverter = toByteString(patchSet, patchSetProtoConverter); - - assertThat(resultOfNewConverter).isEqualTo(resultOfOldConverter); - } - - // Copied from ChangeField. - private static List convertToProtos_old(ProtobufCodec codec, Collection objs) - throws OrmException { - List result = Lists.newArrayListWithCapacity(objs.size()); - ByteArrayOutputStream out = new ByteArrayOutputStream(256); - try { - for (T obj : objs) { - out.reset(); - CodedOutputStream cos = CodedOutputStream.newInstance(out); - codec.encode(obj, cos); - cos.flush(); - result.add(out.toByteArray()); - } - } catch (IOException e) { - throw new OrmException(e); - } - return result; - } - - // Copied from ChangeField. - private static List convertToProtos_new( - ProtoConverter converter, Collection objects) { - return objects - .stream() - .map(converter::toProto) - .map(Protos::toByteArray) - .collect(toImmutableList()); - } - - // Copied from ChangeNotesState.Serializer. - private static ByteString toByteString(T object, ProtoConverter converter) { - MessageLite message = converter.toProto(object); - return Protos.toByteString(message); - } -} From 9a2b06ed900490b48c7ba585ae84de12e7f399b7 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Dec 2018 16:02:16 +0100 Subject: [PATCH 5/6] Protos: Remove unused method and thus cut dependency on gwtorm We got rid of all usages of this method and can now safely remove it. Change-Id: Ia6a886084e2f57bc9c240a9ce317733408606566 --- java/com/google/gerrit/proto/BUILD | 1 - java/com/google/gerrit/proto/Protos.java | 24 ------------------------ 2 files changed, 25 deletions(-) diff --git a/java/com/google/gerrit/proto/BUILD b/java/com/google/gerrit/proto/BUILD index 703aa4e075..4f05bf61f3 100644 --- a/java/com/google/gerrit/proto/BUILD +++ b/java/com/google/gerrit/proto/BUILD @@ -3,7 +3,6 @@ java_library( srcs = ["Protos.java"], visibility = ["//visibility:public"], deps = [ - "//lib:gwtorm", "//lib:protobuf", ], ) diff --git a/java/com/google/gerrit/proto/Protos.java b/java/com/google/gerrit/proto/Protos.java index f8c63a31b9..b4f0b55741 100644 --- a/java/com/google/gerrit/proto/Protos.java +++ b/java/com/google/gerrit/proto/Protos.java @@ -14,7 +14,6 @@ package com.google.gerrit.proto; -import com.google.gwtorm.protobuf.ProtobufCodec; import com.google.protobuf.ByteString; import com.google.protobuf.CodedOutputStream; import com.google.protobuf.MessageLite; @@ -68,29 +67,6 @@ public class Protos { } } - /** - * Serializes an object to a {@link ByteString} using a protobuf codec. - * - *

Guarantees deterministic serialization. No matter whether the use case cares about - * determinism or not, always use this method 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); - } - } - /** * Parses a byte array to a protobuf message. * From 14db531b94f10ee9813d59a01bc1cf00b5c553c5 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Fri, 14 Dec 2018 16:06:41 +0100 Subject: [PATCH 6/6] cache.proto: Remove reference to ProtobufCodec in comments Change-Id: I6875181e743bd8a6eca491e4216a99db9cab612b --- proto/cache.proto | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/proto/cache.proto b/proto/cache.proto index 52f5e1c406..c978069e32 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -61,10 +61,10 @@ message ChangeNotesKeyProto { // 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. +// type. This includes types that can be serialized to proto using +// ProtoConverters as well as NoteDb and indexed types that are serialized using +// JSON. We can always revisit this decision later; 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, @@ -134,10 +134,10 @@ message ChangeNotesStateProto { repeated string hashtag = 5; - // Raw PatchSet proto as produced by ProtobufCodec. + // Raw PatchSet proto as produced by PatchSetProtoConverter. repeated bytes patch_set = 6; - // Raw PatchSetApproval proto as produced by ProtobufCodec. + // Raw PatchSetApproval proto as produced by PatchSetApprovalProtoConverter. repeated bytes approval = 7; // Next ID: 4 @@ -175,7 +175,7 @@ message ChangeNotesStateProto { // com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord. repeated string submit_record = 14; - // Raw ChangeMessage proto as produced by ProtobufCodec. + // Raw ChangeMessage proto as produced by ChangeMessageProtoConverter. repeated bytes change_message = 15; // JSON produced from com.google.gerrit.reviewdb.client.Comment.