From c144e78d78f851e88e049946c2a14a770ae1f77c Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 17 Apr 2019 12:53:07 -0700 Subject: [PATCH] Convert PatchSetApproval.Key to AutoValue While we're in there, rename the underlying field from categoryId to labelId, including renaming the proto field. This is cleanup following from I5df6f0c5. At that time, we opted to leave the name the same because Megastore had problems with renaming proto fields and hence we couldn't easily rename @Column fields. Now that ReviewDb (including the Megastore implementation) is gone, this argument no longer applies. See I6982fb24 for context. Change-Id: I011e0bfeb74345a20b5eb0bb97ce61de7cb12bd0 --- .../reviewdb/client/PatchSetApproval.java | 53 +++++-------------- .../PatchSetApprovalKeyProtoConverter.java | 10 ++-- .../google/gerrit/server/ApprovalCopier.java | 4 +- .../google/gerrit/server/ApprovalsUtil.java | 4 +- .../com/google/gerrit/server/ReviewerSet.java | 4 +- .../gerrit/server/change/LabelNormalizer.java | 2 +- .../server/notedb/ChangeNotesParser.java | 6 +-- .../gerrit/server/restapi/change/Move.java | 2 +- .../gerrit/common/data/LabelFunctionTest.java | 2 +- .../reviewdb/client/PatchSetApprovalTest.java | 6 +-- ...PatchSetApprovalKeyProtoConverterTest.java | 14 ++--- .../PatchSetApprovalProtoConverterTest.java | 16 +++--- .../server/change/LabelNormalizerTest.java | 4 +- .../server/notedb/ChangeNotesStateTest.java | 8 +-- .../rules/IgnoreSelfApprovalRuleTest.java | 2 +- proto/entities.proto | 2 +- 16 files changed, 55 insertions(+), 84 deletions(-) diff --git a/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java b/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java index 82a720fbc9..cef2c04553 100644 --- a/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java +++ b/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java @@ -14,52 +14,25 @@ package com.google.gerrit.reviewdb.client; +import com.google.auto.value.AutoValue; import com.google.gerrit.common.Nullable; -import com.google.gwtorm.client.CompoundKey; import java.sql.Timestamp; import java.util.Date; import java.util.Objects; /** An approval (or negative approval) on a patch set. */ public final class PatchSetApproval { - public static class Key extends CompoundKey { - private static final long serialVersionUID = 1L; + public static Key key(PatchSet.Id patchSetId, Account.Id accountId, LabelId labelId) { + return new AutoValue_PatchSetApproval_Key(patchSetId, accountId, labelId); + } - protected PatchSet.Id patchSetId; + @AutoValue + public abstract static class Key { + public abstract PatchSet.Id patchSetId(); - protected Account.Id accountId; + public abstract Account.Id accountId(); - protected LabelId categoryId; - - protected Key() { - patchSetId = new PatchSet.Id(); - accountId = new Account.Id(); - categoryId = new LabelId(); - } - - public Key(PatchSet.Id ps, Account.Id a, LabelId c) { - this.patchSetId = ps; - this.accountId = a; - this.categoryId = c; - } - - @Override - public PatchSet.Id getParentKey() { - return patchSetId; - } - - public Account.Id getAccountId() { - return accountId; - } - - public LabelId getLabelId() { - return categoryId; - } - - @Override - public com.google.gwtorm.client.Key[] members() { - return new com.google.gwtorm.client.Key[] {accountId, categoryId}; - } + public abstract LabelId labelId(); } protected Key key; @@ -103,7 +76,7 @@ public final class PatchSetApproval { } public PatchSetApproval(PatchSet.Id psId, PatchSetApproval src) { - key = new PatchSetApproval.Key(psId, src.getAccountId(), src.getLabelId()); + key = key(psId, src.getAccountId(), src.getLabelId()); value = src.getValue(); granted = src.granted; realAccountId = src.realAccountId; @@ -120,11 +93,11 @@ public final class PatchSetApproval { } public PatchSet.Id getPatchSetId() { - return key.patchSetId; + return key.patchSetId(); } public Account.Id getAccountId() { - return key.accountId; + return key.accountId(); } public Account.Id getRealAccountId() { @@ -137,7 +110,7 @@ public final class PatchSetApproval { } public LabelId getLabelId() { - return key.categoryId; + return key.labelId(); } public short getValue() { diff --git a/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java b/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java index 353830195d..43f6295f71 100644 --- a/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java +++ b/java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java @@ -35,18 +35,18 @@ public enum PatchSetApprovalKeyProtoConverter @Override public Entities.PatchSetApproval_Key toProto(PatchSetApproval.Key key) { return Entities.PatchSetApproval_Key.newBuilder() - .setPatchSetId(patchSetIdConverter.toProto(key.getParentKey())) - .setAccountId(accountIdConverter.toProto(key.getAccountId())) - .setCategoryId(labelIdConverter.toProto(key.getLabelId())) + .setPatchSetId(patchSetIdConverter.toProto(key.patchSetId())) + .setAccountId(accountIdConverter.toProto(key.accountId())) + .setLabelId(labelIdConverter.toProto(key.labelId())) .build(); } @Override public PatchSetApproval.Key fromProto(Entities.PatchSetApproval_Key proto) { - return new PatchSetApproval.Key( + return PatchSetApproval.key( patchSetIdConverter.fromProto(proto.getPatchSetId()), accountIdConverter.fromProto(proto.getAccountId()), - labelIdConverter.fromProto(proto.getCategoryId())); + labelIdConverter.fromProto(proto.getLabelId())); } @Override diff --git a/java/com/google/gerrit/server/ApprovalCopier.java b/java/com/google/gerrit/server/ApprovalCopier.java index a1df711d5a..6787859e6f 100644 --- a/java/com/google/gerrit/server/ApprovalCopier.java +++ b/java/com/google/gerrit/server/ApprovalCopier.java @@ -165,7 +165,7 @@ public class ApprovalCopier { private static boolean canCopy( ProjectState project, PatchSetApproval psa, PatchSet.Id psId, ChangeKind kind) { - int n = psa.getKey().getParentKey().get(); + int n = psa.getKey().patchSetId().get(); checkArgument(n != psId.get()); LabelType type = project.getLabelTypes().byLabel(psa.getLabelId()); if (type == null) { @@ -193,7 +193,7 @@ public class ApprovalCopier { } private static PatchSetApproval copy(PatchSetApproval src, PatchSet.Id psId) { - if (src.getKey().getParentKey().equals(psId)) { + if (src.getKey().patchSetId().equals(psId)) { return src; } return new PatchSetApproval(psId, src); diff --git a/java/com/google/gerrit/server/ApprovalsUtil.java b/java/com/google/gerrit/server/ApprovalsUtil.java index 865e33ccf7..cb32066c59 100644 --- a/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/java/com/google/gerrit/server/ApprovalsUtil.java @@ -82,7 +82,7 @@ public class ApprovalsUtil { PatchSet.Id psId, CurrentUser user, LabelId labelId, int value, Date when) { PatchSetApproval psa = new PatchSetApproval( - new PatchSetApproval.Key(psId, user.getAccountId(), labelId), + PatchSetApproval.key(psId, user.getAccountId(), labelId), Shorts.checkedCast(value), when); user.updateRealAccountId(psa::setRealAccountId); @@ -210,7 +210,7 @@ public class ApprovalsUtil { for (Account.Id account : need) { cells.add( new PatchSetApproval( - new PatchSetApproval.Key(psId, account, labelId), (short) 0, update.getWhen())); + PatchSetApproval.key(psId, account, labelId), (short) 0, update.getWhen())); update.putReviewer(account, REVIEWER); } return Collections.unmodifiableList(cells); diff --git a/java/com/google/gerrit/server/ReviewerSet.java b/java/com/google/gerrit/server/ReviewerSet.java index f36e3abe48..a3be19ed94 100644 --- a/java/com/google/gerrit/server/ReviewerSet.java +++ b/java/com/google/gerrit/server/ReviewerSet.java @@ -46,9 +46,9 @@ public class ReviewerSet { checkArgument( first .getKey() + .patchSetId() .getParentKey() - .getParentKey() - .equals(psa.getKey().getParentKey().getParentKey()), + .equals(psa.getKey().patchSetId().getParentKey()), "multiple change IDs: %s, %s", first.getKey(), psa.getKey()); diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java index 1ec17176f3..831095bb02 100644 --- a/java/com/google/gerrit/server/change/LabelNormalizer.java +++ b/java/com/google/gerrit/server/change/LabelNormalizer.java @@ -88,7 +88,7 @@ public class LabelNormalizer { List deleted = Lists.newArrayListWithCapacity(approvals.size()); LabelTypes labelTypes = projectCache.checkedGet(notes.getProjectName()).getLabelTypes(notes); for (PatchSetApproval psa : approvals) { - Change.Id changeId = psa.getKey().getParentKey().getParentKey(); + Change.Id changeId = psa.getKey().patchSetId().getParentKey(); checkArgument( changeId.equals(notes.getChangeId()), "Approval %s does not match change %s", diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java index 1e7005b13f..421c1984eb 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java +++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java @@ -792,9 +792,7 @@ class ChangeNotesParser { PatchSetApproval psa = new PatchSetApproval( - new PatchSetApproval.Key(psId, effectiveAccountId, new LabelId(l.label())), - l.value(), - ts); + PatchSetApproval.key(psId, effectiveAccountId, new LabelId(l.label())), l.value(), ts); psa.setTag(tag); if (!Objects.equals(realAccountId, committerId)) { psa.setRealAccountId(realAccountId); @@ -835,7 +833,7 @@ class ChangeNotesParser { // needs an actual approval in order to block copying an earlier approval over a later delete. PatchSetApproval remove = new PatchSetApproval( - new PatchSetApproval.Key(psId, effectiveAccountId, new LabelId(label)), (short) 0, ts); + PatchSetApproval.key(psId, effectiveAccountId, new LabelId(label)), (short) 0, ts); if (!Objects.equals(realAccountId, committerId)) { remove.setRealAccountId(realAccountId); } diff --git a/java/com/google/gerrit/server/restapi/change/Move.java b/java/com/google/gerrit/server/restapi/change/Move.java index f2335b155d..1fc37eff25 100644 --- a/java/com/google/gerrit/server/restapi/change/Move.java +++ b/java/com/google/gerrit/server/restapi/change/Move.java @@ -274,7 +274,7 @@ public class Move extends RetryingRestModifyViewbuilder() .put("patchSetId", PatchSet.Id.class) .put("accountId", Account.Id.class) - .put("categoryId", LabelId.class) + .put("labelId", LabelId.class) .build()); } } diff --git a/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverterTest.java b/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverterTest.java index 80b2cc2f4d..62217de51a 100644 --- a/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverterTest.java +++ b/javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverterTest.java @@ -40,7 +40,7 @@ public class PatchSetApprovalProtoConverterTest { public void allValuesConvertedToProto() { PatchSetApproval patchSetApproval = new PatchSetApproval( - new PatchSetApproval.Key( + PatchSetApproval.key( new PatchSet.Id(new Change.Id(42), 14), new Account.Id(100013), new LabelId("label-8")), @@ -61,7 +61,7 @@ public class PatchSetApprovalProtoConverterTest { .setChangeId(Entities.Change_Id.newBuilder().setId(42)) .setPatchSetId(14)) .setAccountId(Entities.Account_Id.newBuilder().setId(100013)) - .setCategoryId(Entities.LabelId.newBuilder().setId("label-8"))) + .setLabelId(Entities.LabelId.newBuilder().setId("label-8"))) .setValue(456) .setGranted(987654L) .setTag("tag-21") @@ -75,7 +75,7 @@ public class PatchSetApprovalProtoConverterTest { public void mandatoryValuesConvertedToProto() { PatchSetApproval patchSetApproval = new PatchSetApproval( - new PatchSetApproval.Key( + PatchSetApproval.key( new PatchSet.Id(new Change.Id(42), 14), new Account.Id(100013), new LabelId("label-8")), @@ -93,7 +93,7 @@ public class PatchSetApprovalProtoConverterTest { .setChangeId(Entities.Change_Id.newBuilder().setId(42)) .setPatchSetId(14)) .setAccountId(Entities.Account_Id.newBuilder().setId(100013)) - .setCategoryId(Entities.LabelId.newBuilder().setId("label-8"))) + .setLabelId(Entities.LabelId.newBuilder().setId("label-8"))) .setValue(456) .setGranted(987654L) // This value can't be unset when our entity class is given. @@ -106,7 +106,7 @@ public class PatchSetApprovalProtoConverterTest { public void allValuesConvertedToProtoAndBackAgain() { PatchSetApproval patchSetApproval = new PatchSetApproval( - new PatchSetApproval.Key( + PatchSetApproval.key( new PatchSet.Id(new Change.Id(42), 14), new Account.Id(100013), new LabelId("label-8")), @@ -125,7 +125,7 @@ public class PatchSetApprovalProtoConverterTest { public void mandatoryValuesConvertedToProtoAndBackAgain() { PatchSetApproval patchSetApproval = new PatchSetApproval( - new PatchSetApproval.Key( + PatchSetApproval.key( new PatchSet.Id(new Change.Id(42), 14), new Account.Id(100013), new LabelId("label-8")), @@ -150,7 +150,7 @@ public class PatchSetApprovalProtoConverterTest { .setChangeId(Entities.Change_Id.newBuilder().setId(42)) .setPatchSetId(14)) .setAccountId(Entities.Account_Id.newBuilder().setId(100013)) - .setCategoryId(Entities.LabelId.newBuilder().setId("label-8"))) + .setLabelId(Entities.LabelId.newBuilder().setId("label-8"))) .build(); PatchSetApproval patchSetApproval = protoConverter.fromProto(proto); @@ -174,7 +174,7 @@ public class PatchSetApprovalProtoConverterTest { .setChangeId(Entities.Change_Id.newBuilder().setId(42)) .setPatchSetId(14)) .setAccountId(Entities.Account_Id.newBuilder().setId(100013)) - .setCategoryId(Entities.LabelId.newBuilder().setId("label-8"))) + .setLabelId(Entities.LabelId.newBuilder().setId("label-8"))) .setValue(456) .setGranted(987654L) .build(); diff --git a/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java b/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java index 6e02d61713..ec0702fbec 100644 --- a/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java +++ b/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java @@ -188,13 +188,13 @@ public class LabelNormalizerTest extends GerritBaseTests { private PatchSetApproval psa(Account.Id accountId, String label, int value) { return new PatchSetApproval( - new PatchSetApproval.Key(change.currentPatchSetId(), accountId, new LabelId(label)), + PatchSetApproval.key(change.currentPatchSetId(), accountId, new LabelId(label)), (short) value, TimeUtil.nowTs()); } private PatchSetApproval copy(PatchSetApproval src, int newValue) { - PatchSetApproval result = new PatchSetApproval(src.getKey().getParentKey(), src); + PatchSetApproval result = new PatchSetApproval(src.getKey().patchSetId(), src); result.setValue((short) newValue); return result; } diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java index 93c00d7c50..c8d66c16c2 100644 --- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java +++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java @@ -368,7 +368,7 @@ public class ChangeNotesStateTest extends GerritBaseTests { public void serializeApprovals() throws Exception { PatchSetApproval a1 = new PatchSetApproval( - new PatchSetApproval.Key( + PatchSetApproval.key( new PatchSet.Id(ID, 1), new Account.Id(2001), new LabelId("Code-Review")), (short) 1, new Timestamp(1212L)); @@ -377,7 +377,7 @@ public class ChangeNotesStateTest extends GerritBaseTests { PatchSetApproval a2 = new PatchSetApproval( - new PatchSetApproval.Key( + PatchSetApproval.key( new PatchSet.Id(ID, 1), new Account.Id(2002), new LabelId("Verified")), (short) -1, new Timestamp(3434L)); @@ -792,11 +792,11 @@ public class ChangeNotesStateTest extends GerritBaseTests { @Test public void patchSetApprovalFields() throws Exception { assertThatSerializedClass(PatchSetApproval.Key.class) - .hasFields( + .hasAutoValueMethods( ImmutableMap.builder() .put("patchSetId", PatchSet.Id.class) .put("accountId", Account.Id.class) - .put("categoryId", LabelId.class) + .put("labelId", LabelId.class) .build()); assertThatSerializedClass(PatchSetApproval.class) .hasFields( diff --git a/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java b/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java index 14124fa90c..a2a718a9fa 100644 --- a/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java +++ b/javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java @@ -88,7 +88,7 @@ public class IgnoreSelfApprovalRuleTest extends GerritBaseTests { private static PatchSetApproval.Key makeKey( PatchSet.Id psId, Account.Id accountId, LabelId labelId) { - return new PatchSetApproval.Key(psId, accountId, labelId); + return PatchSetApproval.key(psId, accountId, labelId); } private static Account.Id makeAccount(int account) { diff --git a/proto/entities.proto b/proto/entities.proto index d2851d382f..be0aca713c 100644 --- a/proto/entities.proto +++ b/proto/entities.proto @@ -120,7 +120,7 @@ message LabelId { message PatchSetApproval_Key { required PatchSet_Id patch_set_id = 1; required Account_Id account_id = 2; - required LabelId category_id = 3; + required LabelId label_id = 3; } // Serialized form of com.google.gerrit.reviewdb.client.PatchSetApproval.