Fix parsing legacy labels for users with comma

This change fixes a bug introduced by change 336883 when parsing labels
where a UUID was present together with a user name containing a comma.

Bug: Issue 16529
Release-Notes: Fix parsing legacy labels for users with comma
Forward-Compatible: checked
Change-Id: I32575d11dadc2cbe82ae78ff1f43186ba730d611

2 files changed, 99 insertions, 20 deletions

diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 8aa29cc64d..0fc6324015 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -107,6 +107,8 @@ import org.eclipse.jgit.util.RawParseUtils;
 class ChangeNotesParser {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
+  private static final String LABEL_VOTE_UUID_SEPARATOR = ", ";
+
   // Private final members initialized in the constructor.
   private final ChangeNoteJson changeNoteJson;
   private final NoteDbMetrics metrics;
@@ -801,6 +803,41 @@ class ChangeNotesParser {
     }
   }
 
+  // Return the UUID start index or -1 if no UUID is present
+  private int parseCopiedApprovalUuidStart(String line, int tagStart) {
+    int separatorIndex = line.indexOf(LABEL_VOTE_UUID_SEPARATOR);
+
+    // The first part of the condition checks whether the footer has the following format:
+    //   Copied-Label: <LABEL>=VOTE <Gerrit Account>,<Gerrit Real Account> :"<TAG>"
+    //   Weird tag that contains uuid delimiter. The uuid is actually not present.
+    if ((tagStart != -1 && separatorIndex > tagStart)
+        ||
+
+        // The second part of the condition allows us to distinguish the following two lines:
+        //   Label2=+1, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 1 <1@gerrit>
+        //   Label2=+1 User Name (company_name, department) <2@gerrit>
+        (line.indexOf(' ') < separatorIndex)) {
+      return -1;
+    }
+    return separatorIndex;
+  }
+
+  // Splitting on "," breaks for identities containing commas. The below re-implements splitting on
+  // "(?<=>),", but it's 3-5x faster, as performance matters here.
+  private String[] parseIdentities(String line) {
+    String[] idents = line.split(",");
+    List<String> identitiesList = new ArrayList<>();
+    for (int i = 0; i < idents.length; i++) {
+      if (i == 0 || idents[i - 1].endsWith(">")) {
+        identitiesList.add(idents[i]);
+      } else {
+        int lastIndex = identitiesList.size() - 1;
+        identitiesList.set(lastIndex, identitiesList.get(lastIndex) + "," + idents[i]);
+      }
+    }
+    return identitiesList.toArray(new String[0]);
+  }
+
   // Footer example: Copied-Label: <LABEL>=VOTE <Gerrit Account>,<Gerrit Real Account> :"<TAG>"
   // ":<"TAG>"" is optional. <Gerrit Real Account> is also optional, if it was not set.
   // The label, vote, and the Gerrit account are mandatory (unlike FOOTER_LABEL where Gerrit
@@ -817,14 +854,9 @@ class ChangeNotesParser {
     int tagStart = line.indexOf(":\"");
     // UUID introduced in https://gerrit-review.googlesource.com/c/gerrit/+/324937
     // Only parsed for backward compatibility
-    // Footer has the following format in this case:
-    // Copied-Label: <LABEL>=VOTE <Gerrit Account>,<Gerrit Real Account> :"<TAG>"
-    int uuidStart = line.indexOf(", ");
-    // Wired tag that contains uuid delimiter. The uuid is actually not present.
-    if (tagStart != -1 && uuidStart > tagStart) {
-      uuidStart = -1;
-    }
-    int identitiesStart = line.indexOf(' ', uuidStart != -1 ? uuidStart + 2 : 0);
+    int uuidStart = parseCopiedApprovalUuidStart(line, tagStart);
+    int identitiesStart =
+        line.indexOf(' ', uuidStart != -1 ? uuidStart + LABEL_VOTE_UUID_SEPARATOR.length() : 0);
     // The first account is the accountId, and second (if applicable) is the realAccountId.
     try {
       labelVoteStr = line.substring(0, uuidStart != -1 ? uuidStart : identitiesStart);
@@ -832,7 +864,9 @@ class ChangeNotesParser {
       throw new ConfigInvalidException(ex.getMessage(), ex);
     }
     String[] identities =
-        line.substring(identitiesStart + 1, tagStart == -1 ? line.length() : tagStart).split(",");
+        parseIdentities(
+            line.substring(identitiesStart + 1, tagStart == -1 ? line.length() : tagStart));
+
     PersonIdent ident = RawParseUtils.parsePersonIdent(identities[0]);
     checkFooter(ident != null, FOOTER_COPIED_LABEL, line);
     accountId = parseIdent(ident);
@@ -899,19 +933,37 @@ class ChangeNotesParser {
     //     update operation was executed by this user on behalf of the effective
     //     user.
     Account.Id effectiveAccountId;
-    String labelVoteStr;
     // UUID introduced in https://gerrit-review.googlesource.com/c/gerrit/+/324937
     // Only parsed for backward compatibility
-    // Footer has the following format in this case: Label: <LABEL>=VOTE, <UUID> <Gerrit Account>
-    int uuidStart = line.indexOf(", ");
-    int reviewerStart = line.indexOf(' ', uuidStart != -1 ? uuidStart + 2 : 0);
-    if (uuidStart != -1) {
-      labelVoteStr = line.substring(0, uuidStart);
-    } else if (reviewerStart != -1) {
-      labelVoteStr = line.substring(0, reviewerStart);
-    } else {
-      labelVoteStr = line;
+    int voteUuidSeparatorIndex = line.indexOf(LABEL_VOTE_UUID_SEPARATOR);
+    // We need some additional logic to differentiate between labels that have a UUID and those that
+    // have a user with a comma. This allows us to separate the following cases (note that the
+    // leading `Label: ` has been elided at this point):
+    //   Label: <LABEL>=VOTE, <UUID> <Gerrit Account>
+    //   Label: <LABEL>=VOTE <Gerrit, Account>
+    int reviewerStartOffset = 0;
+    int scoreStart = line.indexOf('=') + 1;
+    StringBuilder labelNameScore = new StringBuilder(line.substring(0, scoreStart));
+    for (int i = scoreStart; i < line.length(); i++) {
+      char currentChar = line.charAt(i);
+      // If we hit ',' before ' ' we have a UUID
+      if (currentChar == ',') {
+        labelNameScore.append(line, scoreStart, i);
+        reviewerStartOffset = voteUuidSeparatorIndex + LABEL_VOTE_UUID_SEPARATOR.length();
+        break;
+      }
+      // Otherwise we don't
+      if (currentChar == ' ') {
+        labelNameScore.append(line, scoreStart, i);
+        break;
+      }
+      // If we hit neither we're defensive assign the whole line
+      if (i == line.length() - 1) {
+        labelNameScore = new StringBuilder(line);
+        break;
+      }
     }
+    int reviewerStart = line.indexOf(' ', reviewerStartOffset);
     if (reviewerStart > 0) {
       // Account in the label line (2) becomes the effective ID of the
       // approval. If there is a real user (3) different from the commit user
@@ -926,7 +978,7 @@ class ChangeNotesParser {
 
     LabelVote l;
     try {
-      l = LabelVote.parseWithEquals(labelVoteStr);
+      l = LabelVote.parseWithEquals(labelNameScore.toString());
     } catch (IllegalArgumentException e) {
       ConfigInvalidException pe = parseException("invalid %s: %s", FOOTER_LABEL, line);
       pe.initCause(e);
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
index 361932e7f7..fc8ab42128 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -164,6 +164,19 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
             + "Label: Label1=+1, 577fb248e474018276351785930358ec0450e9f7\n"
             + "Label: Label1=+1, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 2 <2@gerrit>\n"
             + "Label: Label1=0, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 2 <2@gerrit>\n"
+            + "Label: Label1=0, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 2 (name,with, comma) <2@gerrit>\n"
+            + "Subject: This is a test change\n");
+  }
+
+  @Test
+  public void parseApprovalNoUUIDUserWithComma() throws Exception {
+    assertParseSucceeds(
+        "Update change\n"
+            + "\n"
+            + "Branch: refs/heads/master\n"
+            + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+            + "Patch-set: 1\n"
+            + "Label: Label1=+1 Gerrit User 1 (name,with, comma) <1@gerrit>\n"
             + "Subject: This is a test change\n");
   }
 
@@ -220,6 +233,20 @@ public class ChangeNotesParserTest extends AbstractChangeNotesTest {
             + "Copied-Label: Label4=+1, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 1 <1@gerrit> :\"tag with uuid delimiter , \"\n"
             + "Copied-Label: Label4=+1, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 1 <1@gerrit>,Gerrit User 2 <2@gerrit> :\"tag with characters %^#@^( *::!\"\n"
             + "Copied-Label: Label4=+1, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 1 <1@gerrit>,Gerrit User 2 <2@gerrit> :\"tag with uuid delimiter , \"\n"
+            + "Copied-Label: Label4=+1, 577fb248e474018276351785930358ec0450e9f7 Gerrit User 1 (name,with, comma) <1@gerrit>,Gerrit User 2 (name,with, comma) <2@gerrit>\n"
+            + "Subject: This is a test change\n");
+  }
+
+  @Test
+  public void parseCopiedApprovalNoUUIDUserWithComma() throws Exception {
+    assertParseSucceeds(
+        "Update change\n"
+            + "\n"
+            + "Branch: refs/heads/master\n"
+            + "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+            + "Patch-set: 1\n"
+            + "Copied-Label: Label1=+1 Gerrit User 1 (name,with, comma) <1@gerrit>\n"
+            + "Copied-Label: Label2=+1 Gerrit User 1 (name,with, comma) <1@gerrit>,Gerrit User 2 (name,with, comma) <2@gerrit>\n"
             + "Subject: This is a test change\n");
   }