ChangeBundle: Ignore ReviewDb current PSID if invalid

Change-Id: I3d65834e20d2115d4ad8cceab197871a86df62f3

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java

@@ -389,19 +389,22 @@ public class ChangeBundle {
 
   private <K, V> Map<K, V> limitToValidPatchSets(Map<K, V> in,
       final Function<K, PatchSet.Id> func) {
-    final Predicate<PatchSet.Id> upToCurrent = upToCurrentPredicate();
     return Maps.filterKeys(
-        in, new Predicate<K>() {
+        in, Predicates.compose(validPatchSetPredicate(), func));
-          @Override
+  }
-          public boolean apply(K in) {
+
-            PatchSet.Id psId = func.apply(in);
+  private Predicate<PatchSet.Id> validPatchSetPredicate() {
-            return upToCurrent.apply(psId) && patchSets.containsKey(psId);
+    final Predicate<PatchSet.Id> upToCurrent = upToCurrentPredicate();
-          }
+    return new Predicate<PatchSet.Id>() {
-        });
+      @Override
+      public boolean apply(PatchSet.Id in) {
+        return upToCurrent.apply(in) && patchSets.containsKey(in);
+      }
+    };
   }
 
   private Collection<ChangeMessage> filterChangeMessages() {
-    final Predicate<PatchSet.Id> upToCurrent = upToCurrentPredicate();
+    final Predicate<PatchSet.Id> validPatchSet = validPatchSetPredicate();
     return Collections2.filter(changeMessages,
         new Predicate<ChangeMessage>() {
           @Override
@@ -410,7 +413,7 @@ public class ChangeBundle {
             if (psId == null) {
               return true;
             }
-            return upToCurrent.apply(psId) && patchSets.containsKey(psId);
+            return validPatchSet.apply(psId);
           }
         });
   }
@@ -440,6 +443,7 @@ public class ChangeBundle {
     String desc = a.getId().equals(b.getId()) ? describe(a.getId()) : "Changes";
 
     boolean excludeCreatedOn = false;
+    boolean excludeCurrentPatchSetId = false;
     boolean excludeTopic = false;
     Timestamp aUpdated = a.getLastUpdatedOn();
     Timestamp bUpdated = b.getLastUpdatedOn();
@@ -483,12 +487,17 @@ public class ChangeBundle {
     //
     // Ignore empty topic on the ReviewDb side if it is null on the NoteDb side.
     //
+    // Ignore currentPatchSetId on NoteDb side if ReviewDb does not point to a
+    // valid patch set.
+    //
     // Use max timestamp of all ReviewDb entities when comparing with NoteDb.
     if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
       excludeCreatedOn = !timestampsDiffer(
           bundleA, bundleA.getFirstPatchSetTime(), bundleB, b.getCreatedOn());
       aSubj = cleanReviewDbSubject(aSubj);
-      excludeSubject = bSubj.startsWith(aSubj);
+      excludeCurrentPatchSetId =
+          !bundleA.validPatchSetPredicate().apply(a.currentPatchSetId());
+      excludeSubject = bSubj.startsWith(aSubj) || excludeCurrentPatchSetId;
       excludeOrigSubj = true;
       String aTopic = trimLeadingOrNull(a.getTopic());
       excludeTopic = Objects.equals(aTopic, b.getTopic())
@@ -498,7 +507,9 @@ public class ChangeBundle {
       excludeCreatedOn = !timestampsDiffer(
           bundleA, a.getCreatedOn(), bundleB, bundleB.getFirstPatchSetTime());
       bSubj = cleanReviewDbSubject(bSubj);
-      excludeSubject = aSubj.startsWith(bSubj);
+      excludeCurrentPatchSetId =
+          !bundleB.validPatchSetPredicate().apply(b.currentPatchSetId());
+      excludeSubject = aSubj.startsWith(bSubj) || excludeCurrentPatchSetId;
       excludeOrigSubj = true;
       String bTopic = trimLeadingOrNull(b.getTopic());
       excludeTopic = Objects.equals(bTopic, a.getTopic())
@@ -513,6 +524,9 @@ public class ChangeBundle {
     if (excludeCreatedOn) {
       exclude.add("createdOn");
     }
+    if (excludeCurrentPatchSetId) {
+      exclude.add("currentPatchSetId");
+    }
     if (excludeOrigSubj) {
       exclude.add("originalSubject");
     }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java

@@ -413,9 +413,9 @@ public class ChangeBundleTest {
     assertNoDiffs(b1, b2);
 
     // NoteDb has shorter subject, not allowed.
-    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+    b1 = new ChangeBundle(c1, messages(), latest(c1), approvals(),
         comments(), reviewers(), REVIEW_DB);
-    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+    b2 = new ChangeBundle(c2, messages(), latest(c2), approvals(),
         comments(), reviewers(), NOTE_DB);
     assertDiffs(b1, b2,
         "subject differs for Change.Id " + c1.getId() + ":"
@@ -468,9 +468,9 @@ public class ChangeBundleTest {
             + " {Change subject} != {\tChange subject}");
 
     // One NoteDb.
-    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
+    b1 = new ChangeBundle(c1, messages(), latest(c1), approvals(),
         comments(), reviewers(), NOTE_DB);
-    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
+    b2 = new ChangeBundle(c2, messages(), latest(c2), approvals(),
         comments(), reviewers(), REVIEW_DB);
     assertDiffs(b1, b2,
         "subject differs for Change.Id " + c1.getId() + ":"
@@ -508,6 +508,39 @@ public class ChangeBundleTest {
     assertNoDiffs(b2, b1);
   }
 
+  @Test
+  public void diffChangesIgnoresInvalidCurrentPatchSetIdInReviewDb()
+      throws Exception {
+    Change c1 = TestChanges.newChange(
+        new Project.NameKey("project"), new Account.Id(100));
+    Change c2 = clone(c1);
+    c2.setCurrentPatchSet(new PatchSet.Id(c2.getId(), 0), "Unrelated subject",
+        c2.getOriginalSubject());
+
+    // Both ReviewDb.
+    ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(),
+        approvals(), comments(), reviewers(), REVIEW_DB);
+    ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(),
+        approvals(), comments(), reviewers(), REVIEW_DB);
+    assertDiffs(b1, b2,
+        "currentPatchSetId differs for Change.Id " + c1.getId() + ":"
+            + " {1} != {0}",
+        "subject differs for Change.Id " + c1.getId() + ":"
+            + " {Change subject} != {Unrelated subject}");
+
+    // One NoteDb.
+    //
+    // This is based on a real corrupt change where all patch sets were deleted
+    // but the Change entity stuck around, resulting in a currentPatchSetId of 0
+    // after converting to NoteDb.
+    b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
+        reviewers(), REVIEW_DB);
+    b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
+        reviewers(), NOTE_DB);
+    assertNoDiffs(b1, b2);
+    assertNoDiffs(b2, b1);
+  }
+
   @Test
   public void diffChangeMessageKeySets() throws Exception {
     Change c = TestChanges.newChange(project, accountId);