Merge branch 'stable-2.15'

* stable-2.15:
  Fix auto-rebuilding of changes with missing NoteDb refs
  NoteDbOnlyIT: Add test for ChangeNotes.Factory#create(Checked)

Change-Id: I5e5620381c7a53b5f8ada38582c79d4552790fdf

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

@@ -732,16 +732,27 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
     if (autoRebuild) {
       NoteDbChangeState state = NoteDbChangeState.parse(change);
+      if (args.migration.disableChangeReviewDb()) {
+        checkState(
+            state != null,
+            "shouldn't have null NoteDbChangeState when ReviewDb disabled: %s",
+            change);
+      }
       ObjectId id = readRef(repo);
       if (id == null) {
+        // Meta ref doesn't exist in NoteDb.
+
         if (state == null) {
+          // Either ReviewDb change is being newly created, or it exists in ReviewDb but has not yet
+          // been rebuilt for the first time, e.g. because we just turned on write-only mode. In
+          // both cases, we don't want to auto-rebuild, just proceed with an empty ChangeNotes.
           return super.openHandle(repo, id);
-        } else if (shouldExist) {
+        } else if (shouldExist && state.getPrimaryStorage() == PrimaryStorage.NOTE_DB) {
-          // TODO(dborowitz): This means we have a state recorded in noteDbState but the ref doesn't
-          // exist for whatever reason. Doesn't this mean we should trigger an auto-rebuild, rather
-          // than throwing?
           throw new NoSuchChangeException(getChangeId());
         }
+
+        // ReviewDb claims NoteDb state exists, but meta ref isn't present: fall through and
+        // auto-rebuild if necessary.
       }
       RefCache refs = this.refs != null ? this.refs : new RepoRefCache(repo);
       if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {

javatests/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -1309,6 +1309,36 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertThat(getMetaRef(project, refName)).isNull();
   }
 
+  @Test
+  public void autoRebuildMissingRefWriteOnly() throws Exception {
+    setNotesMigration(true, false);
+    testAutoRebuildMissingRef();
+  }
+
+  @Test
+  public void autoRebuildMissingRefReadWrite() throws Exception {
+    setNotesMigration(true, true);
+    testAutoRebuildMissingRef();
+  }
+
+  private void testAutoRebuildMissingRef() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getChange().getId();
+
+    assertChangeUpToDate(true, id);
+    notesFactory.createChecked(db, project, id);
+
+    try (Repository repo = repoManager.openRepository(project)) {
+      RefUpdate ru = repo.updateRef(RefNames.changeMetaRef(id));
+      ru.setForceUpdate(true);
+      assertThat(ru.delete()).isEqualTo(RefUpdate.Result.FORCED);
+    }
+    assertChangeUpToDate(false, id);
+
+    notesFactory.createChecked(db, project, id);
+    assertChangeUpToDate(true, id);
+  }
+
   private void assertChangesReadOnly(RestApiException e) throws Exception {
     Throwable cause = e.getCause();
     assertThat(cause).isInstanceOf(UpdateException.class);
@@ -1332,8 +1362,10 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
       Change c = getUnwrappedDb().changes().get(id);
       assertThat(c).isNotNull();
       assertThat(c.getNoteDbState()).isNotNull();
-      assertThat(NoteDbChangeState.parse(c).isChangeUpToDate(new RepoRefCache(repo)))
+      NoteDbChangeState state = NoteDbChangeState.parse(c);
-          .isEqualTo(expected);
+      assertThat(state).isNotNull();
+      assertThat(state.getPrimaryStorage()).isEqualTo(PrimaryStorage.REVIEW_DB);
+      assertThat(state.isChangeUpToDate(new RepoRefCache(repo))).isEqualTo(expected);
     }
   }
 

javatests/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java

@@ -28,6 +28,7 @@ import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.BatchUpdateListener;
 import com.google.gerrit.server.update.BatchUpdateOp;
@@ -40,6 +41,7 @@ import java.io.IOException;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.concurrent.Callable;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.CommitBuilder;
@@ -192,6 +194,22 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void missingChange() throws Exception {
+    Change.Id changeId = new Change.Id(1234567);
+    assertNoSuchChangeException(() -> notesFactory.create(db, project, changeId));
+    assertNoSuchChangeException(() -> notesFactory.createChecked(db, project, changeId));
+  }
+
+  private void assertNoSuchChangeException(Callable<?> callable) throws Exception {
+    try {
+      callable.call();
+      fail("expected NoSuchChangeException");
+    } catch (NoSuchChangeException e) {
+      // Expected.
+    }
+  }
+
   private class ConcurrentWritingListener implements BatchUpdateListener {
     static final String MSG_PREFIX = "Other writer ";