opendev/gerrit
Code Issues Proposed changes

Convert PatchSetApproval.Key to AutoValue

Browse Source 
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
This commit is contained in:
Dave Borowitz
2019-04-17 12:53:07 -07:00
parent 19101e65cf
commit c144e78d78
16 changed files with 55 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
java/com/google/gerrit/reviewdb/client/PatchSetApproval.java
View File

@@ -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<PatchSet.Id> {
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() {

10
java/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverter.java
View File

@@ -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

4
java/com/google/gerrit/server/ApprovalCopier.java
View File

@@ -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);

4
java/com/google/gerrit/server/ApprovalsUtil.java
View File

@@ -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);

4
java/com/google/gerrit/server/ReviewerSet.java
View File

@@ -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());

2
java/com/google/gerrit/server/change/LabelNormalizer.java
View File

@@ -88,7 +88,7 @@ public class LabelNormalizer {
List<PatchSetApproval> 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",

6
java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -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);
}

2
java/com/google/gerrit/server/restapi/change/Move.java
View File

@@ -274,7 +274,7 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
update.removeApprovalFor(psa.getAccountId(), psa.getLabel());
approvals.add(
new PatchSetApproval(
new PatchSetApproval.Key(psId, psa.getAccountId(), new LabelId(psa.getLabel())),
PatchSetApproval.key(psId, psa.getAccountId(), new LabelId(psa.getLabel())),
(short) 0,
ctx.getWhen()));
}

2
javatests/com/google/gerrit/common/data/LabelFunctionTest.java
View File

@@ -104,7 +104,7 @@ public class LabelFunctionTest 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 void checkBlockWorks(LabelFunction function) {

6
javatests/com/google/gerrit/reviewdb/client/PatchSetApprovalTest.java
View File

@@ -25,13 +25,13 @@ public class PatchSetApprovalTest extends GerritBaseTests {
@Test
public void keyEquality() {
PatchSetApproval.Key k1 =
new PatchSetApproval.Key(
PatchSetApproval.key(
new PatchSet.Id(new Change.Id(1), 2), new Account.Id(3), new LabelId("My-Label"));
PatchSetApproval.Key k2 =
new PatchSetApproval.Key(
PatchSetApproval.key(
new PatchSet.Id(new Change.Id(1), 2), new Account.Id(3), new LabelId("My-Label"));
PatchSetApproval.Key k3 =
new PatchSetApproval.Key(
PatchSetApproval.key(
new PatchSet.Id(new Change.Id(1), 2), new Account.Id(3), new LabelId("Other-Label"));
assertThat(k2).isEqualTo(k1);

14
javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalKeyProtoConverterTest.java
View File

@@ -37,7 +37,7 @@ public class PatchSetApprovalKeyProtoConverterTest {
@Test
public void allValuesConvertedToProto() {
PatchSetApproval.Key key =
new PatchSetApproval.Key(
PatchSetApproval.key(
new PatchSet.Id(new Change.Id(42), 14), new Account.Id(100013), new LabelId("label-8"));
Entities.PatchSetApproval_Key proto = protoConverter.toProto(key);
@@ -49,7 +49,7 @@ public class PatchSetApprovalKeyProtoConverterTest {
.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();
assertThat(proto).isEqualTo(expectedProto);
}
@@ -57,7 +57,7 @@ public class PatchSetApprovalKeyProtoConverterTest {
@Test
public void allValuesConvertedToProtoAndBackAgain() {
PatchSetApproval.Key key =
new PatchSetApproval.Key(
PatchSetApproval.key(
new PatchSet.Id(new Change.Id(42), 14), new Account.Id(100013), new LabelId("label-8"));
PatchSetApproval.Key convertedKey = protoConverter.fromProto(protoConverter.toProto(key));
@@ -74,7 +74,7 @@ public class PatchSetApprovalKeyProtoConverterTest {
.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();
byte[] bytes = proto.toByteArray();
@@ -86,13 +86,13 @@ public class PatchSetApprovalKeyProtoConverterTest {
/** See {@link SerializedClassSubject} for background and what to do if this test fails. */
@Test
public void fieldsExistAsExpected() {
public void methodsExistAsExpected() {
assertThatSerializedClass(PatchSetApproval.Key.class)
.hasFields(
.hasAutoValueMethods(
ImmutableMap.<String, Type>builder()
.put("patchSetId", PatchSet.Id.class)
.put("accountId", Account.Id.class)
.put("categoryId", LabelId.class)
.put("labelId", LabelId.class)
.build());
}
}

16
javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverterTest.java
View File

@@ -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();

4
javatests/com/google/gerrit/server/change/LabelNormalizerTest.java
View File

@@ -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;
}

8
javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
View File

@@ -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.<String, Type>builder()
.put("patchSetId", PatchSet.Id.class)
.put("accountId", Account.Id.class)
.put("categoryId", LabelId.class)
.put("labelId", LabelId.class)
.build());
assertThatSerializedClass(PatchSetApproval.class)
.hasFields(

2
javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java
View File

@@ -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) {

2
proto/entities.proto
View File

@@ -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.