ChangeNotesParser: Extract class for parsing patch set fields

A small subset of fields on PatchSet are mutable, meaning they can be
encountered during parsing before the initial PatchSet data is
populated. Previously, ChangeNotesParser handled this case by recording
a PatchSet with a sentinel invalid RevId. This was a little ugly, and
prevents us from either enforcing that RevId has the correct format or
migrating entirely to ObjectId.

Instead, define a separate class encapsulating just the pending mutable
fields. When encountering the initial patch set definition commit, copy
the mutable fields into the PatchSet.

Change-Id: Ia86390ff604c3d5ab1e0d2f7f53e1ce2b0570ac9

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

@@ -101,10 +101,6 @@ import org.eclipse.jgit.util.RawParseUtils;
 class ChangeNotesParser {
   private static final FluentLogger logger = FluentLogger.forEnclosingClass();
 
-  // Sentinel RevId indicating a mutable field on a patch set was parsed, but
-  // the parser does not yet know its commit SHA-1.
-  private static final RevId PARTIAL_PATCH_SET = new RevId("INVALID PARTIAL PATCH SET");
-
   @AutoValue
   abstract static class ApprovalKey {
     abstract PatchSet.Id psId();
@@ -118,6 +114,18 @@ class ChangeNotesParser {
     }
   }
 
+  /**
+   * Subset of field on patch sets that are mutable after patch set creation, and therefore may be
+   * parsed before the rest of the patch set is available.
+   *
+   * <p>In the future, if {@code PatchSet} becomes an immutable type with a builder, this class can
+   * be replaced with the builder. For now, keep it as simple as possible.
+   */
+  static class PendingPatchSetFields {
+    List<String> groups = Collections.emptyList();
+    String description;
+  }
+
   // Private final members initialized in the constructor.
   private final ChangeNoteJson changeNoteJson;
   private final LegacyChangeNoteRead legacyChangeNoteRead;
@@ -136,6 +144,7 @@ class ChangeNotesParser {
   private final List<SubmitRecord> submitRecords;
   private final ListMultimap<ObjectId, Comment> comments;
   private final Map<PatchSet.Id, PatchSet> patchSets;
+  private final Map<PatchSet.Id, PendingPatchSetFields> pendingPatchSets;
   private final Set<PatchSet.Id> deletedPatchSets;
   private final Map<PatchSet.Id, PatchSetState> patchSetStates;
   private final List<PatchSet.Id> currentPatchSets;
@@ -193,6 +202,7 @@ class ChangeNotesParser {
     allChangeMessages = new ArrayList<>();
     comments = MultimapBuilder.hashKeys().arrayListValues().build();
     patchSets = new HashMap<>();
+    pendingPatchSets = new HashMap<>();
     deletedPatchSets = new HashSet<>();
     patchSetStates = new HashMap<>();
     currentPatchSets = new ArrayList<>();
@@ -364,11 +374,14 @@ class ChangeNotesParser {
       submissionId = parseSubmissionId(commit);
     }
 
+    // Parse mutable patch set fields first so they can be recorded in the PendingPatchSetFields.
+    parseDescription(psId, commit);
+    parseGroups(psId, commit);
+
     ObjectId currRev = parseRevision(commit);
     if (currRev != null) {
       parsePatchSet(psId, currRev, accountId, ts);
     }
-    parseGroups(psId, commit);
     parseCurrentPatchSet(psId, commit);
 
     if (submitRecords.isEmpty()) {
@@ -412,8 +425,6 @@ class ChangeNotesParser {
     if (lastUpdatedOn == null || ts.after(lastUpdatedOn)) {
       lastUpdatedOn = ts;
     }
-
-    parseDescription(psId, commit);
   }
 
   private String parseSubmissionId(ChangeNotesCommit commit) throws ConfigInvalidException {
@@ -486,14 +497,9 @@ class ChangeNotesParser {
     if (accountId == null) {
       throw parseException("patch set %s requires an identified user as uploader", psId.get());
     }
-    PatchSet ps = patchSets.get(psId);
+    if (patchSets.containsKey(psId)) {
-    if (ps == null) {
-      ps = new PatchSet(psId);
-      patchSets.put(psId, ps);
-    } else if (!ps.getRevision().equals(PARTIAL_PATCH_SET)) {
       if (deletedPatchSets.contains(psId)) {
-        // Do not update PS details as PS was deleted and this meta data is of
+        // Do not update PS details as PS was deleted and this meta data is of no relevance.
-        // no relevance
         return;
       }
       throw new ConfigInvalidException(
@@ -501,9 +507,16 @@ class ChangeNotesParser {
               "Multiple revisions parsed for patch set %s: %s and %s",
               psId.get(), patchSets.get(psId).getRevision(), rev.name()));
     }
+    PatchSet ps = new PatchSet(psId);
+    patchSets.put(psId, ps);
     ps.setRevision(new RevId(rev.name()));
     ps.setUploader(accountId);
     ps.setCreatedOn(ts);
+    PendingPatchSetFields pending = pendingPatchSets.remove(psId);
+    if (pending != null) {
+      ps.setGroups(pending.groups);
+      ps.setDescription(pending.description);
+    }
   }
 
   private void parseGroups(PatchSet.Id psId, ChangeNotesCommit commit)
@@ -512,15 +525,14 @@ class ChangeNotesParser {
     if (groupsStr == null) {
       return;
     }
-    PatchSet ps = patchSets.get(psId);
+    if (patchSets.containsKey(psId)) {
-    if (ps == null) {
+      throw patchSetFieldBeforeDefinition(psId, FOOTER_GROUPS);
-      ps = new PatchSet(psId);
+    }
-      ps.setRevision(PARTIAL_PATCH_SET);
+    PendingPatchSetFields pending =
-      patchSets.put(psId, ps);
+        pendingPatchSets.computeIfAbsent(psId, p -> new PendingPatchSetFields());
-    } else if (!ps.getGroups().isEmpty()) {
+    if (pending.groups.isEmpty()) {
-      return;
+      pending.groups = PatchSet.splitGroups(groupsStr);
     }
-    ps.setGroups(PatchSet.splitGroups(groupsStr));
   }
 
   private void parseCurrentPatchSet(PatchSet.Id psId, ChangeNotesCommit commit)
@@ -661,16 +673,17 @@ class ChangeNotesParser {
     List<String> descLines = commit.getFooterLineValues(FOOTER_PATCH_SET_DESCRIPTION);
     if (descLines.isEmpty()) {
       return;
-    } else if (descLines.size() == 1) {
-      String desc = descLines.get(0).trim();
-      PatchSet ps = patchSets.get(psId);
-      if (ps == null) {
-        ps = new PatchSet(psId);
-        ps.setRevision(PARTIAL_PATCH_SET);
-        patchSets.put(psId, ps);
     }
-      if (ps.getDescription() == null) {
+
-        ps.setDescription(desc);
+    if (patchSets.containsKey(psId)) {
+      throw patchSetFieldBeforeDefinition(psId, FOOTER_PATCH_SET_DESCRIPTION);
+    }
+    if (descLines.size() == 1) {
+      String desc = descLines.get(0).trim();
+      PendingPatchSetFields pending =
+          pendingPatchSets.computeIfAbsent(psId, p -> new PendingPatchSetFields());
+      if (pending.description == null) {
+        pending.description = desc;
       }
     } else {
       throw expectedOneFooter(FOOTER_PATCH_SET_DESCRIPTION, descLines);
@@ -1003,13 +1016,7 @@ class ChangeNotesParser {
 
   private void updatePatchSetStates() {
     Set<PatchSet.Id> missing = new TreeSet<>(comparing(PatchSet.Id::get));
-    for (Iterator<PatchSet> it = patchSets.values().iterator(); it.hasNext(); ) {
+    missing.addAll(pendingPatchSets.keySet());
-      PatchSet ps = it.next();
-      if (ps.getRevision().equals(PARTIAL_PATCH_SET)) {
-        missing.add(ps.getId());
-        it.remove();
-      }
-    }
     for (Map.Entry<PatchSet.Id, PatchSetState> e : patchSetStates.entrySet()) {
       switch (e.getValue()) {
         case PUBLISHED:
@@ -1089,6 +1096,13 @@ class ChangeNotesParser {
     }
   }
 
+  private ConfigInvalidException patchSetFieldBeforeDefinition(
+      PatchSet.Id psId, FooterKey footerPatchSetDescription) {
+    return parseException(
+        "%s field found for patch set %s before it was defined",
+        footerPatchSetDescription.getName(), psId.get());
+  }
+
   private ConfigInvalidException parseException(String fmt, Object... args) {
     return ChangeNotes.parseException(id, fmt, args);
   }