Merge "Support read-only mode for NoteDb changes"

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java

@@ -20,6 +20,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.changeMetaRef;
 import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
+import static org.junit.Assert.fail;
 
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
@@ -34,6 +35,7 @@ import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -52,9 +54,11 @@ import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.RepoRefCache;
+import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.notedb.ChangeBundle;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager;
 import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
 import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper;
 import com.google.gerrit.testutil.ConfigSuite;
@@ -880,6 +884,71 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void failWhenWritesDisabled() throws Exception {
+    setNotesMigration(true, true);
+
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    assertChangeUpToDate(true, id);
+    assertThat(gApi.changes().id(id.get()).info().topic).isNull();
+
+    // Turning off writes causes failure.
+    setNotesMigration(false, true);
+    try {
+      gApi.changes().id(id.get()).topic(name("a-topic"));
+      fail("Expected write to fail");
+    } catch (RestApiException e) {
+      assertChangesReadOnly(e);
+    }
+
+    // Update was not written.
+    assertThat(gApi.changes().id(id.get()).info().topic).isNull();
+    assertChangeUpToDate(true, id);
+  }
+
+  @Test
+  public void rebuildWhenWritesDisabledWorksButDoesNotWrite() throws Exception {
+    setNotesMigration(true, true);
+
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getPatchSetId().getParentKey();
+    assertChangeUpToDate(true, id);
+
+    // Make a ReviewDb change behind NoteDb's back and ensure it's detected.
+    setNotesMigration(false, false);
+    gApi.changes().id(id.get()).topic(name("a-topic"));
+    setInvalidNoteDbState(id);
+    assertChangeUpToDate(false, id);
+
+    // On next NoteDb read, change is rebuilt in-memory but not stored.
+    setNotesMigration(false, true);
+    assertThat(gApi.changes().id(id.get()).info().topic)
+        .isEqualTo(name("a-topic"));
+    assertChangeUpToDate(false, id);
+
+    // Attempting to write directly causes failure.
+    try {
+      gApi.changes().id(id.get()).topic(name("other-topic"));
+      fail("Expected write to fail");
+    } catch (RestApiException e) {
+      assertChangesReadOnly(e);
+    }
+
+    // Update was not written.
+    assertThat(gApi.changes().id(id.get()).info().topic)
+        .isEqualTo(name("a-topic"));
+    assertChangeUpToDate(false, id);
+  }
+
+  private void assertChangesReadOnly(RestApiException e) throws Exception {
+    Throwable cause = e.getCause();
+    assertThat(cause).isInstanceOf(UpdateException.class);
+    assertThat(cause.getCause()).isInstanceOf(OrmException.class);
+    assertThat(cause.getCause())
+        .hasMessage(NoteDbUpdateManager.CHANGES_READ_ONLY);
+  }
+
   private void setInvalidNoteDbState(Change.Id id) throws Exception {
     ReviewDb db = unwrapDb();
     Change c = db.changes().get(id);

gerrit-server/src/main/java/com/google/gerrit/server/change/Rebuild.java

@@ -54,7 +54,7 @@ public class Rebuild implements RestModifyView<ChangeResource, Input> {
   public Response<?> apply(ChangeResource rsrc, Input input)
       throws ResourceNotFoundException, IOException, OrmException,
       ConfigInvalidException {
-    if (!migration.writeChanges()) {
+    if (!migration.commitChangeWrites()) {
       throw new ResourceNotFoundException();
     }
     try {

gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java

@@ -636,6 +636,11 @@ public class BatchUpdate implements AutoCloseable {
 
     List<ChangeTask> tasks = new ArrayList<>(ops.keySet().size());
     try {
+      if (!ops.isEmpty() && notesMigration.failChangeWrites()) {
+        // Fail fast before attempting any writes if changes are read-only, as
+        // this is a programmer error.
+        throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY);
+      }
       List<ListenableFuture<?>> futures = new ArrayList<>(ops.keySet().size());
       for (Map.Entry<Change.Id, Collection<Op>> e : ops.asMap().entrySet()) {
         ChangeTask task =
@@ -645,13 +650,15 @@ public class BatchUpdate implements AutoCloseable {
       }
       Futures.allAsList(futures).get();
 
-      if (notesMigration.writeChanges()) {
+      if (notesMigration.commitChangeWrites()) {
         executeNoteDbUpdates(tasks);
       }
     } catch (ExecutionException | InterruptedException e) {
       Throwables.propagateIfInstanceOf(e.getCause(), UpdateException.class);
       Throwables.propagateIfInstanceOf(e.getCause(), RestApiException.class);
       throw new UpdateException(e);
+    } catch (OrmException e) {
+      throw new UpdateException(e);
     }
 
     // Reindex changes.
@@ -802,7 +809,7 @@ public class BatchUpdate implements AutoCloseable {
           deleted = ctx.deleted;
 
           // Stage the NoteDb update and store its state in the Change.
-          if (notesMigration.writeChanges()) {
+          if (notesMigration.commitChangeWrites()) {
             updateManager = stageNoteDbUpdate(ctx, deleted);
           }
 
@@ -821,7 +828,7 @@ public class BatchUpdate implements AutoCloseable {
           db.rollback();
         }
 
-        if (notesMigration.writeChanges()) {
+        if (notesMigration.commitChangeWrites()) {
           try {
             // Do not execute the NoteDbUpdateManager, as we don't want too much
             // contention on the underlying repo, and we would rather use a

gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java

@@ -204,9 +204,7 @@ public class ChangeIndexer {
   public void index(ReviewDb db, Change change)
       throws IOException, OrmException {
     ChangeData cd;
-    if (notesMigration.readChanges()) {
+    if (notesMigration.commitChangeWrites()) {
-      cd = changeDataFactory.create(db, change);
-    } else if (notesMigration.writeChanges()) {
       // Auto-rebuilding when NoteDb reads are disabled just increases
       // contention on the meta ref from a background indexing thread with
       // little benefit. The next actual write to the entity may still incur a

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

@@ -192,6 +192,11 @@ public abstract class AbstractChangeUpdate {
     if (isEmpty()) {
       return null;
     }
+
+    // Allow this method to proceed even if migration.failChangeWrites() = true.
+    // This may be used by an auto-rebuilding step that the caller does not plan
+    // to actually store.
+
     checkArgument(rw.getObjectReader().getCreatedFromInserter() == ins);
     ObjectId z = ObjectId.zeroId();
     CommitBuilder cb = applyImpl(rw, ins, curr);

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

@@ -122,6 +122,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   private final ChangeNoteUtil changeNoteUtil;
   private final ChangeUpdate.Factory updateFactory;
   private final NoteDbUpdateManager.Factory updateManagerFactory;
+  private final NotesMigration migration;
   private final PatchListCache patchListCache;
   private final PersonIdent serverIdent;
   private final ProjectCache projectCache;
@@ -134,6 +135,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
       ChangeNoteUtil changeNoteUtil,
       ChangeUpdate.Factory updateFactory,
       NoteDbUpdateManager.Factory updateManagerFactory,
+      NotesMigration migration,
       PatchListCache patchListCache,
       @GerritPersonIdent PersonIdent serverIdent,
       @Nullable ProjectCache projectCache,
@@ -144,6 +146,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     this.changeNoteUtil = changeNoteUtil;
     this.updateFactory = updateFactory;
     this.updateManagerFactory = updateManagerFactory;
+    this.migration = migration;
     this.patchListCache = patchListCache;
     this.serverIdent = serverIdent;
     this.projectCache = projectCache;
@@ -221,7 +224,14 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
           return change;
         }
       });
-      manager.execute();
+      if (!migration.failChangeWrites()) {
+        manager.execute();
+      } else {
+        // Don't even attempt to execute if read-only, it would fail anyway. But
+        // do throw an exception to the caller so they know to use the staged
+        // results instead of reading from the repo.
+        throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY);
+      }
     } catch (AbortUpdateException e) {
       // Drop this rebuild; another thread completed it.
     }

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

@@ -64,10 +64,6 @@ public class ConfigNotesMigration extends NotesMigration {
         checkArgument(lk.equals(WRITE) || lk.equals(READ),
             "invalid NoteDb key: %s.%s", t, key);
       }
-      boolean write = cfg.getBoolean(NOTE_DB, t, WRITE, false);
-      boolean read = cfg.getBoolean(NOTE_DB, t, READ, false);
-      checkArgument(!(read && !write),
-          "must have write enabled when read enabled: %s", t);
     }
   }
 
@@ -95,7 +91,7 @@ public class ConfigNotesMigration extends NotesMigration {
   }
 
   @Override
-  public boolean writeChanges() {
+  protected boolean writeChanges() {
     return writeChanges;
   }
 

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

@@ -70,6 +70,8 @@ import java.util.Set;
  * of updates, use {@link #stage()}.
  */
 public class NoteDbUpdateManager {
+  public static String CHANGES_READ_ONLY = "NoteDb changes are read-only";
+
   public interface Factory {
     NoteDbUpdateManager create(Project.NameKey projectName);
   }
@@ -249,7 +251,7 @@ public class NoteDbUpdateManager {
   }
 
   private boolean isEmpty() {
-    if (!migration.writeChanges()) {
+    if (!migration.commitChangeWrites()) {
       return true;
     }
     return changeUpdates.isEmpty()
@@ -382,6 +384,10 @@ public class NoteDbUpdateManager {
   }
 
   public void execute() throws OrmException, IOException {
+    // Check before even inspecting the list, as this is a programmer error.
+    if (migration.failChangeWrites()) {
+      throw new OrmException(CHANGES_READ_ONLY);
+    }
     if (isEmpty()) {
       return;
     }

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

@@ -34,9 +34,33 @@ package com.google.gerrit.server.notedb;
  * these reasons, the options remain undocumented.
  */
 public abstract class NotesMigration {
+  /**
+   * Read changes from NoteDb.
+   * <p>
+   * Change data is read from NoteDb refs, but ReviewDb is still the source of
+   * truth. If the loader determines NoteDb is out of date, the change data in
+   * NoteDb will be transparently rebuilt. This means that some code paths that
+   * look read-only may in fact attempt to write.
+   * <p>
+   * If true and {@code writeChanges() = false}, changes can still be read from
+   * NoteDb, but any attempts to write will generate an error.
+   */
   public abstract boolean readChanges();
 
-  public abstract boolean writeChanges();
+  /**
+   * Write changes to NoteDb.
+   * <p>
+   * Updates to change data are written to NoteDb refs, but ReviewDb is still
+   * the source of truth. Change data will not be written unless the NoteDb refs
+   * are already up to date, and the write path will attempt to rebuild the
+   * change if not.
+   * <p>
+   * If false, the behavior when attempting to write depends on
+   * {@code readChanges()}. If {@code readChanges() = false}, writes to NoteDb
+   * are simply ignored; if {@code true}, any attempts to write will generate an
+   * error.
+   */
+  protected abstract boolean writeChanges();
 
   public abstract boolean readAccounts();
 
@@ -51,6 +75,24 @@ public abstract class NotesMigration {
     return false;
   }
 
+  public boolean commitChangeWrites() {
+    // It may seem odd that readChanges() without writeChanges() means we should
+    // attempt to commit writes. However, this method is used by callers to know
+    // whether or not they should short-circuit and skip attempting to read or
+    // write NoteDb refs.
+    //
+    // It is possible for commitChangeWrites() to return true and
+    // failChangeWrites() to also return true, causing an error later in the
+    // same codepath. This specific condition is used by the auto-rebuilding
+    // path to rebuild a change and stage the results, but not commit them due
+    // to failChangeWrites().
+    return writeChanges() || readChanges();
+  }
+
+  public boolean failChangeWrites() {
+    return !writeChanges() && readChanges();
+  }
+
   public boolean enabled() {
     return writeChanges() || readChanges()
         || writeAccounts() || readAccounts();

gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java

@@ -29,6 +29,8 @@ public class TestNotesMigration extends NotesMigration {
     return readChanges;
   }
 
+  // Increase visbility from superclass, as tests may want to check whether
+  // NoteDb data is written in specific migration scenarios.
   @Override
   public boolean writeChanges() {
     return writeChanges;