Convert PatchSetApproval to AutoValue

The public API of PatchSetApproval remains the same with respect to
nullability: getTag is a non-nullable Optional, and getRealAccountId
falls back to getAccountId if setRealAccountId was never called. In the
old implementation, these fields were nullable internally, due to
ReviewDb constraints; AutoValue has no such constraints, so they are
plain non-nullable fields. Instead, populating the default values is
done via logic in the builder.

The no-arg defensive copy method is no longer required, since
PatchSetApproval is immutable. copyWithPatchSet is still useful as a
shortcut, because swapping out just the PatchSet.Id nested in the
PatchSetApproval.Key is annoying.

Leave method names the same so that substantive code changes are easier
to review.

Change-Id: I342f87e9a7cd209afa6e4a6d0158a1f3a5d11360

java/com/google/gerrit/reviewdb/client/PatchSetApproval.java

@@ -16,14 +16,13 @@ package com.google.gerrit.reviewdb.client;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.primitives.Shorts;
-import com.google.gerrit.common.Nullable;
 import java.sql.Timestamp;
 import java.util.Date;
-import java.util.Objects;
 import java.util.Optional;
 
 /** An approval (or negative approval) on a patch set. */
-public final class PatchSetApproval {
+@AutoValue
+public abstract class PatchSetApproval {
   public static Key key(PatchSet.Id patchSetId, Account.Id accountId, LabelId labelId) {
     return new AutoValue_PatchSetApproval_Key(patchSetId, accountId, labelId);
   }
@@ -35,9 +34,55 @@ public final class PatchSetApproval {
     public abstract Account.Id accountId();
 
     public abstract LabelId labelId();
+
+    public boolean isLegacySubmit() {
+      return LabelId.LEGACY_SUBMIT_NAME.equals(labelId().get());
+    }
   }
 
-  protected Key key;
+  public static Builder builder() {
+    return new AutoValue_PatchSetApproval.Builder().postSubmit(false);
+  }
+
+  @AutoValue.Builder
+  public abstract static class Builder {
+    public abstract Builder key(Key key);
+
+    public abstract Key getKey();
+
+    public abstract Builder value(short value);
+
+    public Builder value(int value) {
+      return value(Shorts.checkedCast(value));
+    }
+
+    public abstract Builder granted(Timestamp granted);
+
+    public Builder granted(Date granted) {
+      return granted(new Timestamp(granted.getTime()));
+    }
+
+    public abstract Builder tag(String tag);
+
+    public abstract Builder tag(Optional<String> tag);
+
+    public abstract Builder realAccountId(Account.Id realAccountId);
+
+    abstract Optional<Account.Id> getRealAccountId();
+
+    public abstract Builder postSubmit(boolean isPostSubmit);
+
+    abstract PatchSetApproval autoBuild();
+
+    public PatchSetApproval build() {
+      if (!getRealAccountId().isPresent()) {
+        realAccountId(getKey().accountId());
+      }
+      return autoBuild();
+    }
+  }
+
+  public abstract Key getKey();
 
   /**
    * Value assigned by the user.
@@ -55,96 +100,33 @@ public final class PatchSetApproval {
    * and in the negative and positive direction a magnitude can be assumed.The further from 0 the
    * more assertive the approval.
    */
-  protected short value;
+  public abstract short getValue();
 
-  protected Timestamp granted;
+  public abstract Timestamp getGranted();
 
-  @Nullable protected String tag;
+  public abstract Optional<String> getTag();
 
   /** Real user that made this approval on behalf of the user recorded in {@link Key#accountId}. */
-  @Nullable protected Account.Id realAccountId;
+  public abstract Account.Id getRealAccountId();
 
-  protected boolean postSubmit;
+  public abstract boolean isPostSubmit();
 
-  // DELETED: id = 4 (changeOpen)
+  public abstract Builder toBuilder();
-  // DELETED: id = 5 (changeSortKey)
-
-  protected PatchSetApproval() {}
-
-  public PatchSetApproval(PatchSetApproval.Key k, short v, Date ts) {
-    key = k;
-    setValue(v);
-    setGranted(ts);
-  }
-
-  private PatchSetApproval(PatchSet.Id psId, PatchSetApproval src) {
-    key = key(psId, src.getAccountId(), src.getLabelId());
-    value = src.getValue();
-    granted = src.granted;
-    realAccountId = src.realAccountId;
-    tag = src.tag;
-    postSubmit = src.postSubmit;
-  }
 
   public PatchSetApproval copyWithPatchSet(PatchSet.Id psId) {
-    return new PatchSetApproval(psId, this);
+    return toBuilder().key(key(psId, getKey().accountId(), getKey().labelId())).build();
-  }
-
-  public PatchSetApproval copy() {
-    return copyWithPatchSet(getPatchSetId());
-  }
-
-  public PatchSetApproval.Key getKey() {
-    return key;
   }
 
   public PatchSet.Id getPatchSetId() {
-    return key.patchSetId();
+    return getKey().patchSetId();
   }
 
   public Account.Id getAccountId() {
-    return key.accountId();
+    return getKey().accountId();
-  }
-
-  public Account.Id getRealAccountId() {
-    return realAccountId != null ? realAccountId : getAccountId();
-  }
-
-  public void setRealAccountId(Account.Id id) {
-    // Use null for same real author, as before the column was added.
-    realAccountId = Objects.equals(getAccountId(), id) ? null : id;
   }
 
   public LabelId getLabelId() {
-    return key.labelId();
+    return getKey().labelId();
-  }
-
-  public short getValue() {
-    return value;
-  }
-
-  public void setValue(short v) {
-    value = v;
-  }
-
-  public void setValue(int v) {
-    setValue(Shorts.checkedCast(v));
-  }
-
-  public Timestamp getGranted() {
-    return granted;
-  }
-
-  public void setGranted(Date when) {
-    if (when instanceof Timestamp) {
-      granted = (Timestamp) when;
-    } else {
-      granted = new Timestamp(when.getTime());
-    }
-  }
-
-  public void setTag(String t) {
-    tag = t;
   }
 
   public String getLabel() {
@@ -152,54 +134,6 @@ public final class PatchSetApproval {
   }
 
   public boolean isLegacySubmit() {
-    return LabelId.LEGACY_SUBMIT_NAME.equals(getLabel());
+    return getKey().isLegacySubmit();
-  }
-
-  public Optional<String> getTag() {
-    return Optional.ofNullable(tag);
-  }
-
-  public void setPostSubmit(boolean postSubmit) {
-    this.postSubmit = postSubmit;
-  }
-
-  public boolean isPostSubmit() {
-    return postSubmit;
-  }
-
-  @Override
-  public String toString() {
-    StringBuilder sb =
-        new StringBuilder("[")
-            .append(key)
-            .append(": ")
-            .append(value)
-            .append(",tag:")
-            .append(tag)
-            .append(",realAccountId:")
-            .append(realAccountId);
-    if (postSubmit) {
-      sb.append(",postSubmit");
-    }
-    return sb.append(']').toString();
-  }
-
-  @Override
-  public boolean equals(Object o) {
-    if (o instanceof PatchSetApproval) {
-      PatchSetApproval p = (PatchSetApproval) o;
-      return Objects.equals(key, p.key)
-          && Objects.equals(value, p.value)
-          && Objects.equals(granted, p.granted)
-          && Objects.equals(tag, p.tag)
-          && Objects.equals(realAccountId, p.realAccountId)
-          && postSubmit == p.postSubmit;
-    }
-    return false;
-  }
-
-  @Override
-  public int hashCode() {
-    return Objects.hash(key, value, granted, tag, realAccountId, postSubmit);
   }
 }

java/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverter.java

@@ -54,21 +54,19 @@ public enum PatchSetApprovalProtoConverter
 
   @Override
   public PatchSetApproval fromProto(Entities.PatchSetApproval proto) {
-    PatchSetApproval patchSetApproval =
+    PatchSetApproval.Builder builder =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
-            patchSetApprovalKeyProtoConverter.fromProto(proto.getKey()),
+            .key(patchSetApprovalKeyProtoConverter.fromProto(proto.getKey()))
-            (short) proto.getValue(),
+            .value(proto.getValue())
-            new Timestamp(proto.getGranted()));
+            .granted(new Timestamp(proto.getGranted()))
+            .postSubmit(proto.getPostSubmit());
     if (proto.hasTag()) {
-      patchSetApproval.setTag(proto.getTag());
+      builder.tag(proto.getTag());
     }
     if (proto.hasRealAccountId()) {
-      patchSetApproval.setRealAccountId(accountIdConverter.fromProto(proto.getRealAccountId()));
+      builder.realAccountId(accountIdConverter.fromProto(proto.getRealAccountId()));
     }
-    if (proto.hasPostSubmit()) {
+    return builder.build();
-      patchSetApproval.setPostSubmit(proto.getPostSubmit());
-    }
-    return patchSetApproval;
   }
 
   @Override

java/com/google/gerrit/server/ApprovalsUtil.java

@@ -25,7 +25,6 @@ import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
 import com.google.common.flogger.FluentLogger;
-import com.google.common.primitives.Shorts;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
@@ -76,15 +75,15 @@ import org.eclipse.jgit.revwalk.RevWalk;
 public class ApprovalsUtil {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
-  public static PatchSetApproval newApproval(
+  public static PatchSetApproval.Builder newApproval(
       PatchSet.Id psId, CurrentUser user, LabelId labelId, int value, Date when) {
-    PatchSetApproval psa =
+    PatchSetApproval.Builder b =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
-            PatchSetApproval.key(psId, user.getAccountId(), labelId),
+            .key(PatchSetApproval.key(psId, user.getAccountId(), labelId))
-            Shorts.checkedCast(value),
+            .value(value)
-            when);
+            .granted(when);
-    user.updateRealAccountId(psa::setRealAccountId);
+    user.updateRealAccountId(b::realAccountId);
-    return psa;
+    return b;
   }
 
   private static Iterable<PatchSetApproval> filterApprovals(
@@ -207,8 +206,11 @@ public class ApprovalsUtil {
     LabelId labelId = Iterables.getLast(allTypes).getLabelId();
     for (Account.Id account : need) {
       cells.add(
-          new PatchSetApproval(
+          PatchSetApproval.builder()
-              PatchSetApproval.key(psId, account, labelId), (short) 0, update.getWhen()));
+              .key(PatchSetApproval.key(psId, account, labelId))
+              .value(0)
+              .granted(update.getWhen())
+              .build());
       update.putReviewer(account, REVIEWER);
     }
     return Collections.unmodifiableList(cells);
@@ -286,7 +288,7 @@ public class ApprovalsUtil {
     Date ts = update.getWhen();
     for (Map.Entry<String, Short> vote : approvals.entrySet()) {
       LabelType lt = labelTypes.byLabel(vote.getKey());
-      cells.add(newApproval(ps.id(), user, lt.getLabelId(), vote.getValue(), ts));
+      cells.add(newApproval(ps.id(), user, lt.getLabelId(), vote.getValue(), ts).build());
     }
     for (PatchSetApproval psa : cells) {
       update.putApproval(psa.getLabel(), psa.getValue());

java/com/google/gerrit/server/change/LabelNormalizer.java

@@ -103,8 +103,7 @@ public class LabelNormalizer {
         deleted.add(psa);
         continue;
       }
-      PatchSetApproval copy = psa.copy();
+      PatchSetApproval copy = applyTypeFloor(label, psa);
-      applyTypeFloor(label, copy);
       if (copy.getValue() != psa.getValue()) {
         updated.add(copy);
       } else {
@@ -114,14 +113,16 @@ public class LabelNormalizer {
     return Result.create(unchanged, updated, deleted);
   }
 
-  private void applyTypeFloor(LabelType lt, PatchSetApproval a) {
+  private PatchSetApproval applyTypeFloor(LabelType lt, PatchSetApproval a) {
+    PatchSetApproval.Builder b = a.toBuilder();
     LabelValue atMin = lt.getMin();
     if (atMin != null && a.getValue() < atMin.getValue()) {
-      a.setValue(atMin.getValue());
+      b.value(atMin.getValue());
     }
     LabelValue atMax = lt.getMax();
     if (atMax != null && a.getValue() > atMax.getValue()) {
-      a.setValue(atMax.getValue());
+      b.value(atMax.getValue());
-    }
+    }
+    return b.build();
   }
 }

java/com/google/gerrit/server/git/MergedByPushOp.java

@@ -21,9 +21,7 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.LabelId;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
-import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.config.SendEmailExecutor;
@@ -148,12 +146,7 @@ public class MergedByPushOp implements BatchUpdateOp {
         ChangeMessagesUtil.newMessage(
             psId, ctx.getUser(), ctx.getWhen(), msgBuf.toString(), ChangeMessagesUtil.TAG_MERGED);
     cmUtil.addChangeMessage(update, msg);
-
+    update.putApproval(LabelId.legacySubmit().get(), (short) 1);
-    PatchSetApproval submitter =
-        ApprovalsUtil.newApproval(
-            change.currentPatchSetId(), ctx.getUser(), LabelId.legacySubmit(), 1, ctx.getWhen());
-    update.putApproval(submitter.getLabel(), submitter.getValue());
-
     return true;
   }
 

java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -62,7 +62,6 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Predicate;
@@ -338,12 +337,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {
     if (approvals == null) {
-      ImmutableListMultimap.Builder<PatchSet.Id, PatchSetApproval> b =
+      approvals = ImmutableListMultimap.copyOf(state.approvals());
-          ImmutableListMultimap.builder();
-      for (Map.Entry<PatchSet.Id, PatchSetApproval> e : state.approvals()) {
-        b.put(e.getKey(), e.getValue().copy());
-      }
-      approvals = b.build();
     }
     return approvals;
   }

java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -121,8 +121,8 @@ class ChangeNotesParser {
   private final Set<PatchSet.Id> deletedPatchSets;
   private final Map<PatchSet.Id, PatchSetState> patchSetStates;
   private final List<PatchSet.Id> currentPatchSets;
-  private final Map<PatchSetApproval.Key, PatchSetApproval> approvals;
+  private final Map<PatchSetApproval.Key, PatchSetApproval.Builder> approvals;
-  private final List<PatchSetApproval> bufferedApprovals;
+  private final List<PatchSetApproval.Builder> bufferedApprovals;
   private final List<ChangeMessage> allChangeMessages;
 
   // Non-final private members filled in during the parsing process.
@@ -280,14 +280,14 @@ class ChangeNotesParser {
   private ListMultimap<PatchSet.Id, PatchSetApproval> buildApprovals() {
     ListMultimap<PatchSet.Id, PatchSetApproval> result =
         MultimapBuilder.hashKeys().arrayListValues().build();
-    for (PatchSetApproval a : approvals.values()) {
+    for (PatchSetApproval.Builder a : approvals.values()) {
-      if (!patchSetCommitParsed(a.getPatchSetId())) {
+      if (!patchSetCommitParsed(a.getKey().patchSetId())) {
         continue; // Patch set deleted or missing.
-      } else if (allPastReviewers.contains(a.getAccountId())
+      } else if (allPastReviewers.contains(a.getKey().accountId())
-          && !reviewers.containsRow(a.getAccountId())) {
+          && !reviewers.containsRow(a.getKey().accountId())) {
         continue; // Reviewer was explicitly removed.
       }
-      result.put(a.getPatchSetId(), a);
+      result.put(a.getKey().patchSetId(), a.build());
     }
     result.keySet().forEach(k -> result.get(k).sort(ChangeNotes.PSA_BY_TIME));
     return result;
@@ -612,9 +612,9 @@ class ChangeNotesParser {
     // exception is the legacy SUBM approval, which is never considered post-submit, but might end
     // up sorted after the submit during rebuilding.
     if (status == Change.Status.MERGED) {
-      for (PatchSetApproval psa : bufferedApprovals) {
+      for (PatchSetApproval.Builder psa : bufferedApprovals) {
-        if (!psa.isLegacySubmit()) {
+        if (!psa.getKey().isLegacySubmit()) {
-          psa.setPostSubmit(true);
+          psa.postSubmit(true);
         }
       }
     }
@@ -743,7 +743,7 @@ class ChangeNotesParser {
     if (accountId == null) {
       throw parseException("patch set %s requires an identified user as uploader", psId.get());
     }
-    PatchSetApproval psa;
+    PatchSetApproval.Builder psa;
     if (line.startsWith("-")) {
       psa = parseRemoveApproval(psId, accountId, realAccountId, ts, line);
     } else {
@@ -752,7 +752,7 @@ class ChangeNotesParser {
     bufferedApprovals.add(psa);
   }
 
-  private PatchSetApproval parseAddApproval(
+  private PatchSetApproval.Builder parseAddApproval(
       PatchSet.Id psId, Account.Id committerId, Account.Id realAccountId, Timestamp ts, String line)
       throws ConfigInvalidException {
     // There are potentially 3 accounts involved here:
@@ -789,24 +789,20 @@ class ChangeNotesParser {
       throw pe;
     }
 
-    PatchSetApproval psa =
+    PatchSetApproval.Builder psa =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
-            PatchSetApproval.key(psId, effectiveAccountId, LabelId.create(l.label())),
+            .key(PatchSetApproval.key(psId, effectiveAccountId, LabelId.create(l.label())))
-            l.value(),
+            .value(l.value())
-            ts);
+            .granted(ts)
-    psa.setTag(tag);
+            .tag(Optional.ofNullable(tag));
     if (!Objects.equals(realAccountId, committerId)) {
-      psa.setRealAccountId(realAccountId);
+      psa.realAccountId(realAccountId);
-    }
-    PatchSetApproval.Key k =
-        PatchSetApproval.key(psId, effectiveAccountId, LabelId.create(l.label()));
-    if (!approvals.containsKey(k)) {
-      approvals.put(k, psa);
     }
+    approvals.putIfAbsent(psa.getKey(), psa);
     return psa;
   }
 
-  private PatchSetApproval parseRemoveApproval(
+  private PatchSetApproval.Builder parseRemoveApproval(
       PatchSet.Id psId, Account.Id committerId, Account.Id realAccountId, Timestamp ts, String line)
       throws ConfigInvalidException {
     // See comments in parseAddApproval about the various users involved.
@@ -833,16 +829,15 @@ class ChangeNotesParser {
 
     // Store an actual 0-vote approval in the map for a removed approval, because ApprovalCopier
     // needs an actual approval in order to block copying an earlier approval over a later delete.
-    PatchSetApproval remove =
+    PatchSetApproval.Builder remove =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
-            PatchSetApproval.key(psId, effectiveAccountId, LabelId.create(label)), (short) 0, ts);
+            .key(PatchSetApproval.key(psId, effectiveAccountId, LabelId.create(label)))
+            .value(0)
+            .granted(ts);
     if (!Objects.equals(realAccountId, committerId)) {
-      remove.setRealAccountId(realAccountId);
+      remove.realAccountId(realAccountId);
-    }
-    PatchSetApproval.Key k = PatchSetApproval.key(psId, effectiveAccountId, LabelId.create(label));
-    if (!approvals.containsKey(k)) {
-      approvals.put(k, remove);
     }
+    approvals.putIfAbsent(remove.getKey(), remove);
     return remove;
   }
 
@@ -1028,7 +1023,7 @@ class ChangeNotesParser {
             comments.values(), c -> PatchSet.id(id, c.key.patchSetId), missing);
     pruned +=
         pruneEntitiesForMissingPatchSets(
-            approvals.values(), PatchSetApproval::getPatchSetId, missing);
+            approvals.values(), psa -> psa.getKey().patchSetId(), missing);
 
     if (!missing.isEmpty()) {
       logger.atWarning().log(

java/com/google/gerrit/server/restapi/change/Move.java

@@ -272,10 +272,11 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
         // Remove votes from NoteDb.
         update.removeApprovalFor(psa.getAccountId(), psa.getLabel());
         approvals.add(
-            new PatchSetApproval(
+            PatchSetApproval.builder()
-                PatchSetApproval.key(psId, psa.getAccountId(), LabelId.create(psa.getLabel())),
+                .key(PatchSetApproval.key(psId, psa.getAccountId(), LabelId.create(psa.getLabel())))
-                (short) 0,
+                .value(0)
-                ctx.getWhen()));
+                .granted(ctx.getWhen())
+                .build());
       }
     }
   }

java/com/google/gerrit/server/restapi/change/PostReview.java

@@ -133,6 +133,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.Set;
 import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -1149,10 +1150,13 @@ public class PostReview
             update.putApproval(normName, (short) 0);
           }
         } else if (c != null && c.getValue() != ent.getValue()) {
-          c.setValue(ent.getValue());
+          PatchSetApproval.Builder b =
-          c.setGranted(ctx.getWhen());
+              c.toBuilder()
-          c.setTag(in.tag);
+                  .value(ent.getValue())
-          ctx.getUser().updateRealAccountId(c::setRealAccountId);
+                  .granted(ctx.getWhen())
+                  .tag(Optional.ofNullable(in.tag));
+          ctx.getUser().updateRealAccountId(b::realAccountId);
+          c = b.build();
           ups.add(c);
           addLabelDelta(normName, c.getValue());
           oldApprovals.put(normName, previous.get(normName));
@@ -1163,9 +1167,11 @@ public class PostReview
           oldApprovals.put(normName, null);
           approvals.put(normName, c.getValue());
         } else if (c == null) {
-          c = ApprovalsUtil.newApproval(psId, user, lt.getLabelId(), ent.getValue(), ctx.getWhen());
+          c =
-          c.setTag(in.tag);
+              ApprovalsUtil.newApproval(psId, user, lt.getLabelId(), ent.getValue(), ctx.getWhen())
-          c.setGranted(ctx.getWhen());
+                  .tag(Optional.ofNullable(in.tag))
+                  .granted(ctx.getWhen())
+                  .build();
           ups.add(c);
           addLabelDelta(normName, c.getValue());
           oldApprovals.put(normName, previous.get(normName));
@@ -1275,18 +1281,16 @@ public class PostReview
           }
 
           LabelId labelId = labelTypes.get(0).getLabelId();
-          PatchSetApproval c = ApprovalsUtil.newApproval(psId, user, labelId, 0, ctx.getWhen());
+          ups.add(
-          c.setTag(in.tag);
+              ApprovalsUtil.newApproval(psId, user, labelId, 0, ctx.getWhen())
-          c.setGranted(ctx.getWhen());
+                  .tag(Optional.ofNullable(in.tag))
-          ups.add(c);
+                  .granted(ctx.getWhen())
+                  .build());
         } else {
           // Pick a random label that is about to be deleted and keep it.
           Iterator<PatchSetApproval> i = del.iterator();
-          PatchSetApproval c = i.next();
+          ups.add(i.next().toBuilder().value(0).granted(ctx.getWhen()).build());
-          c.setValue(0);
-          c.setGranted(ctx.getWhen());
           i.remove();
-          ups.add(c);
         }
       }
       ctx.getUpdate(ctx.getChange().currentPatchSetId()).putReviewer(user.getAccountId(), REVIEWER);

java/com/google/gerrit/server/submit/SubmitStrategyOp.java

@@ -348,7 +348,8 @@ abstract class SubmitStrategyOp implements BatchUpdateOp {
     }
 
     submitter =
-        ApprovalsUtil.newApproval(psId, ctx.getUser(), LabelId.legacySubmit(), 1, ctx.getWhen());
+        ApprovalsUtil.newApproval(psId, ctx.getUser(), LabelId.legacySubmit(), 1, ctx.getWhen())
+            .build();
     byKey.put(submitter.getKey(), submitter);
 
     // Flatten out existing approvals for this patch set based upon the current

javatests/com/google/gerrit/common/data/LabelFunctionTest.java

@@ -22,9 +22,9 @@ 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 java.sql.Date;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Date;
 import java.util.List;
 import org.junit.Test;
 
@@ -96,14 +96,11 @@ public class LabelFunctionTest {
   }
 
   private static PatchSetApproval makeApproval(int value) {
-    Account.Id accountId = Account.id(10000 + value);
+    return PatchSetApproval.builder()
-    PatchSetApproval.Key key = makeKey(PS_ID, accountId, LABEL_ID);
+        .key(PatchSetApproval.key(PS_ID, Account.id(10000 + value), LABEL_ID))
-    return new PatchSetApproval(key, (short) value, Date.from(Instant.now()));
+        .value(value)
-  }
+        .granted(Date.from(Instant.now()))
-
+        .build();
-  private static PatchSetApproval.Key makeKey(
-      PatchSet.Id psId, Account.Id accountId, LabelId labelId) {
-    return PatchSetApproval.key(psId, accountId, labelId);
   }
 
   private static void checkBlockWorks(LabelFunction function) {

javatests/com/google/gerrit/reviewdb/converter/PatchSetApprovalProtoConverterTest.java

@@ -26,10 +26,12 @@ 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.inject.TypeLiteral;
 import com.google.protobuf.Parser;
 import java.lang.reflect.Type;
 import java.sql.Timestamp;
 import java.util.Date;
+import java.util.Optional;
 import org.junit.Test;
 
 public class PatchSetApprovalProtoConverterTest {
@@ -39,14 +41,16 @@ public class PatchSetApprovalProtoConverterTest {
   @Test
   public void allValuesConvertedToProto() {
     PatchSetApproval patchSetApproval =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
+            .key(
                 PatchSetApproval.key(
-                PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")),
+                    PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")))
-            (short) 456,
+            .value(456)
-            new Date(987654L));
+            .granted(new Date(987654L))
-    patchSetApproval.setTag("tag-21");
+            .tag("tag-21")
-    patchSetApproval.setRealAccountId(Account.id(612));
+            .realAccountId(Account.id(612))
-    patchSetApproval.setPostSubmit(true);
+            .postSubmit(true)
+            .build();
 
     Entities.PatchSetApproval proto = protoConverter.toProto(patchSetApproval);
 
@@ -72,11 +76,13 @@ public class PatchSetApprovalProtoConverterTest {
   @Test
   public void mandatoryValuesConvertedToProto() {
     PatchSetApproval patchSetApproval =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
+            .key(
                 PatchSetApproval.key(
-                PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")),
+                    PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")))
-            (short) 456,
+            .value(456)
-            new Date(987654L));
+            .granted(new Date(987654L))
+            .build();
 
     Entities.PatchSetApproval proto = protoConverter.toProto(patchSetApproval);
 
@@ -101,14 +107,16 @@ public class PatchSetApprovalProtoConverterTest {
   @Test
   public void allValuesConvertedToProtoAndBackAgain() {
     PatchSetApproval patchSetApproval =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
+            .key(
                 PatchSetApproval.key(
-                PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")),
+                    PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")))
-            (short) 456,
+            .value(456)
-            new Date(987654L));
+            .granted(new Date(987654L))
-    patchSetApproval.setTag("tag-21");
+            .tag("tag-21")
-    patchSetApproval.setRealAccountId(Account.id(612));
+            .realAccountId(Account.id(612))
-    patchSetApproval.setPostSubmit(true);
+            .postSubmit(true)
+            .build();
 
     PatchSetApproval convertedPatchSetApproval =
         protoConverter.fromProto(protoConverter.toProto(patchSetApproval));
@@ -118,11 +126,13 @@ public class PatchSetApprovalProtoConverterTest {
   @Test
   public void mandatoryValuesConvertedToProtoAndBackAgain() {
     PatchSetApproval patchSetApproval =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
+            .key(
                 PatchSetApproval.key(
-                PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")),
+                    PatchSet.id(Change.id(42), 14), Account.id(100013), LabelId.create("label-8")))
-            (short) 456,
+            .value(456)
-            new Date(987654L));
+            .granted(new Date(987654L))
+            .build();
 
     PatchSetApproval convertedPatchSetApproval =
         protoConverter.fromProto(protoConverter.toProto(patchSetApproval));
@@ -182,14 +192,15 @@ public class PatchSetApprovalProtoConverterTest {
   @Test
   public void fieldsExistAsExpected() {
     assertThatSerializedClass(PatchSetApproval.class)
-        .hasFields(
+        .hasAutoValueMethods(
             ImmutableMap.<String, Type>builder()
-                .put("key", PatchSetApproval.Key.class)
+                .put("getKey", PatchSetApproval.Key.class)
-                .put("value", short.class)
+                .put("getValue", short.class)
-                .put("granted", Timestamp.class)
+                .put("getGranted", Timestamp.class)
-                .put("tag", String.class)
+                .put("getTag", new TypeLiteral<Optional<String>>() {}.getType())
-                .put("realAccountId", Account.Id.class)
+                .put("getRealAccountId", Account.Id.class)
-                .put("postSubmit", boolean.class)
+                .put("isPostSubmit", boolean.class)
+                .put("toBuilder", PatchSetApproval.Builder.class)
                 .build());
   }
 }

javatests/com/google/gerrit/server/change/LabelNormalizerTest.java

@@ -186,16 +186,15 @@ public class LabelNormalizerTest {
   }
 
   private PatchSetApproval psa(Account.Id accountId, String label, int value) {
-    return new PatchSetApproval(
+    return PatchSetApproval.builder()
-        PatchSetApproval.key(change.currentPatchSetId(), accountId, LabelId.create(label)),
+        .key(PatchSetApproval.key(change.currentPatchSetId(), accountId, LabelId.create(label)))
-        (short) value,
+        .value(value)
-        TimeUtil.nowTs());
+        .granted(TimeUtil.nowTs())
+        .build();
   }
 
   private PatchSetApproval copy(PatchSetApproval src, int newValue) {
-    PatchSetApproval result = src.copy();
+    return src.toBuilder().value(newValue).build();
-    result.setValue(newValue);
-    return result;
   }
 
   private static List<PatchSetApproval> list(PatchSetApproval... psas) {

javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java

@@ -368,19 +368,24 @@ public class ChangeNotesStateTest {
   @Test
   public void serializeApprovals() throws Exception {
     PatchSetApproval a1 =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
+            .key(
                 PatchSetApproval.key(
-                PatchSet.id(ID, 1), Account.id(2001), LabelId.create("Code-Review")),
+                    PatchSet.id(ID, 1), Account.id(2001), LabelId.create("Code-Review")))
-            (short) 1,
+            .value(1)
-            new Timestamp(1212L));
+            .granted(new Timestamp(1212L))
+            .build();
     ByteString a1Bytes = toByteString(a1, PatchSetApprovalProtoConverter.INSTANCE);
     assertThat(a1Bytes.size()).isEqualTo(43);
 
     PatchSetApproval a2 =
-        new PatchSetApproval(
+        PatchSetApproval.builder()
-            PatchSetApproval.key(PatchSet.id(ID, 1), Account.id(2002), LabelId.create("Verified")),
+            .key(
-            (short) -1,
+                PatchSetApproval.key(
-            new Timestamp(3434L));
+                    PatchSet.id(ID, 1), Account.id(2002), LabelId.create("Verified")))
+            .value(-1)
+            .granted(new Timestamp(3434L))
+            .build();
     ByteString a2Bytes = toByteString(a2, PatchSetApprovalProtoConverter.INSTANCE);
     assertThat(a2Bytes.size()).isEqualTo(49);
     assertThat(a2Bytes).isNotEqualTo(a1Bytes);
@@ -790,14 +795,15 @@ public class ChangeNotesStateTest {
                 .put("labelId", LabelId.class)
                 .build());
     assertThatSerializedClass(PatchSetApproval.class)
-        .hasFields(
+        .hasAutoValueMethods(
             ImmutableMap.<String, Type>builder()
-                .put("key", PatchSetApproval.Key.class)
+                .put("getKey", PatchSetApproval.Key.class)
-                .put("value", short.class)
+                .put("getValue", short.class)
-                .put("granted", Timestamp.class)
+                .put("getGranted", Timestamp.class)
-                .put("tag", String.class)
+                .put("getTag", new TypeLiteral<Optional<String>>() {}.getType())
-                .put("realAccountId", Account.Id.class)
+                .put("getRealAccountId", Account.Id.class)
-                .put("postSubmit", boolean.class)
+                .put("isPostSubmit", boolean.class)
+                .put("toBuilder", PatchSetApproval.Builder.class)
                 .build());
   }
 

javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -358,7 +358,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         .containsExactlyEntriesIn(
             ImmutableListMultimap.of(
                 psa.getPatchSetId(),
-                new PatchSetApproval(psa.getKey(), (short) 0, update.getWhen())));
+                PatchSetApproval.builder()
+                    .key(psa.getKey())
+                    .value(0)
+                    .granted(update.getWhen())
+                    .build()));
   }
 
   @Test
@@ -384,7 +388,11 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
         .containsExactlyEntriesIn(
             ImmutableListMultimap.of(
                 psa.getPatchSetId(),
-                new PatchSetApproval(psa.getKey(), (short) 0, update.getWhen())));
+                PatchSetApproval.builder()
+                    .key(psa.getKey())
+                    .value(0)
+                    .granted(update.getWhen())
+                    .build()));
 
     // Add back approval on same label.
     update = newUpdate(c, otherUser);

javatests/com/google/gerrit/server/rules/IgnoreSelfApprovalRuleTest.java

@@ -81,13 +81,11 @@ public class IgnoreSelfApprovalRuleTest {
   }
 
   private static PatchSetApproval makeApproval(LabelId labelId, Account.Id accountId, int value) {
-    PatchSetApproval.Key key = makeKey(PS_ID, accountId, labelId);
+    return PatchSetApproval.builder()
-    return new PatchSetApproval(key, (short) value, Date.from(Instant.now()));
+        .key(PatchSetApproval.key(PS_ID, accountId, labelId))
-  }
+        .value(value)
-
+        .granted(Date.from(Instant.now()))
-  private static PatchSetApproval.Key makeKey(
+        .build();
-      PatchSet.Id psId, Account.Id accountId, LabelId labelId) {
-    return PatchSetApproval.key(psId, accountId, labelId);
   }
 
   private static Account.Id makeAccount(int account) {