diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index dc6a933a54..ed9cd90886 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java @@ -1275,6 +1275,28 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { assertThat(newPs3.getCreatedOn()).isGreaterThan(ps1.getCreatedOn()); } + @Test + public void ignoreNoteDbStateWithNoCorrespondingRefWhenWritesAndReadsDisabled() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + ReviewDb db = getUnwrappedDb(); + Change c = db.changes().get(id); + c.setNoteDbState("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + db.changes().update(Collections.singleton(c)); + c = db.changes().get(id); + + String refName = RefNames.changeMetaRef(id); + assertThat(getMetaRef(project, refName)).isNull(); + + ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id); + assertThat(notes.getChange().getRowVersion()).isEqualTo(c.getRowVersion()); + + notes = notesFactory.createChecked(dbProvider.get(), project, id); + assertThat(notes.getChange().getRowVersion()).isEqualTo(c.getRowVersion()); + + assertThat(getMetaRef(project, refName)).isNull(); + } + private void assertChangesReadOnly(RestApiException e) throws Exception { Throwable cause = e.getCause(); assertThat(cause).isInstanceOf(UpdateException.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java index 4cb570a422..7b19b39427 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java @@ -144,7 +144,10 @@ public abstract class AbstractChangeNotes { throw new OrmException("NoteDb is required to read change " + changeId); } boolean readOrWrite = read || args.migration.writeChanges(); - if (!readOrWrite && !autoRebuild) { + if (!readOrWrite) { + // Don't even open the repo if we neither write to nor read from NoteDb. It's possible that + // there is some garbage in the noteDbState field and/or the repo, but at this point NoteDb is + // completely off so it's none of our business. loadDefaults(); return self(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index d821a78661..9582fb372f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -604,6 +604,9 @@ public class ChangeNotes extends AbstractChangeNotes { if (state == null) { return super.openHandle(repo, id); } else if (shouldExist) { + // 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()); } }