From c893c02b32a9be96eb5f5d2665fd273fa8853a26 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 20 Dec 2017 08:14:27 -0500 Subject: [PATCH] AbstractChangeNotes: Never open repo when NoteDb is off This line has seen some action before in I1867f633, and before that in I265ef862. Rereading those commit messages, I see no reason why we should be opening the repo and possibly triggering auto-rebuilding when NoteDb writes and reads are both disabled. This was causing NoSuchChangeExceptions in the case where the Change contained a noteDbState field but the ref was absent. That logic is also questionable, for reasons now mentioned in a TODO. But really we should never be reaching the openHandle method when NoteDb is completely off. Add a regression test that would have caught this. Change-Id: If0970e1cf61e1d98ccbb3ce27549186f5771466a --- .../server/notedb/ChangeRebuilderIT.java | 22 +++++++++++++++++++ .../server/notedb/AbstractChangeNotes.java | 5 ++++- .../gerrit/server/notedb/ChangeNotes.java | 3 +++ 3 files changed, 29 insertions(+), 1 deletion(-) 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()); } }