Consistently normalize PatchSetApprovals when persisting

Comparing an unnormalized psa with a normalized psa can create
unexpected differences, leading to error messages during the
copy-approvals command like:
java.lang.IllegalArgumentException: Approval that should be copied is not copied.

Fix those situations by applying normalization to the current approvals
before comparing them to the inferred approvals (which already have
normalization applied).

Release-Notes: Fix copy-approvals for cases where current approvals have different values when normalized
Change-Id: I17cccf537943654f89a4d02af13ec2dda768f0df

1 files changed, 8 insertions, 2 deletions

diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index 870485a5a3..7744f49a66 100644
--- a/java/com/google/gerrit/server/approval/ApprovalsUtil.java
+++ b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
@@ -45,6 +45,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
+import com.google.gerrit.server.change.LabelNormalizer;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.ReviewerStateInternal;
@@ -102,6 +103,7 @@ public class ApprovalsUtil {
   private final PermissionBackend permissionBackend;
   private final ProjectCache projectCache;
   private final ApprovalCache approvalCache;
+  private final LabelNormalizer labelNormalizer;
 
   @VisibleForTesting
   @Inject
@@ -109,11 +111,13 @@ public class ApprovalsUtil {
       ApprovalInference approvalInference,
       PermissionBackend permissionBackend,
       ProjectCache projectCache,
-      ApprovalCache approvalCache) {
+      ApprovalCache approvalCache,
+      LabelNormalizer labelNormalizer) {
     this.approvalInference = approvalInference;
     this.permissionBackend = permissionBackend;
     this.projectCache = projectCache;
     this.approvalCache = approvalCache;
+    this.labelNormalizer = labelNormalizer;
   }
 
   /**
@@ -375,13 +379,15 @@ public class ApprovalsUtil {
       ChangeUpdate changeUpdate) {
     Set<PatchSetApproval> current =
         ImmutableSet.copyOf(notes.getApprovalsWithCopied().get(notes.getCurrentPatchSet().id()));
+    Iterable<PatchSetApproval> currentNormalized =
+        labelNormalizer.normalize(notes, current).getNormalized();
     Set<PatchSetApproval> inferred =
         ImmutableSet.copyOf(approvalInference.forPatchSet(notes, patchSet, revWalk, repoConfig));
 
     // Exempt granted timestamp from comparisson, otherwise, we would persist the copied
     // labels every time this method is called.
     Table<LabelId, Account.Id, Short> approvalTable = HashBasedTable.create();
-    for (PatchSetApproval psa : current) {
+    for (PatchSetApproval psa : currentNormalized) {
       Account.Id id = psa.accountId();
       approvalTable.put(psa.labelId(), id, psa.value());
     }