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
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 8aa29cc..0fc6324 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -107,6 +107,8 @@
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 @@
}
}
+ // 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 @@
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 @@
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 @@
// 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 @@
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 361932e..fc8ab42 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -164,6 +164,19 @@
+ "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 @@
+ "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");
}
