commit 55b95261cfe591ef74ce67f5a4cbb6ca91393a0e
author Nasser Grainawi <nasser.grainawi@linaro.org> Mon Feb 26 16:05:07 2024 -0800
committer Nasser Grainawi <nasser.grainawi@linaro.org> Mon Feb 26 16:05:07 2024 -0800
tree 7b232fc7329f39eab612ed3482770014b8411459
parent 0586478f1ece9f34130a97b08a47b5e441576b8a

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

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

diff --git a/java/com/google/gerrit/server/approval/ApprovalsUtil.java b/java/com/google/gerrit/server/approval/ApprovalsUtil.java
index 870485a..7744f49 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.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 @@
   private final PermissionBackend permissionBackend;
   private final ProjectCache projectCache;
   private final ApprovalCache approvalCache;
+  private final LabelNormalizer labelNormalizer;
 
   @VisibleForTesting
   @Inject
@@ -109,11 +111,13 @@
       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 @@
       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());
     }