Extract an interface to replace ChangeBundle#fromReviewDb

Unfortunately due to a quirk of our googlesource.com implementation,
we can't provide a strong enough consistency guarantee when reading a
change's sub-entities inside of a beginTransaction block, so we need
to provide a hand-written implementation.

For a gwtorm-backed implementation, beginTransaction is likely good
enough, although the actual semantics are still dependent on the
underlying database's isolation level. If this code were going to stay
around for longer and see more production use, we might see more
implementations, but that is almost certainly more trouble than it's
worth.

Change-Id: I27aa35d95b9a4eab3178518065b0cab8823feb4e

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java

@@ -27,6 +27,8 @@ import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.config.TrackingFooters;
 import com.google.gerrit.server.config.TrackingFootersProvider;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeBundleReader;
+import com.google.gerrit.server.notedb.GwtormChangeBundleReader;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.schema.DataSourceType;
 import com.google.gerrit.server.schema.NotesMigrationSchemaFactory;
@@ -89,6 +91,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule {
     bind(Key.get(schemaFactory, ReviewDbFactory.class))
         .to(InMemoryDatabase.class);
     bind(InMemoryDatabase.class).in(SINGLETON);
+    bind(ChangeBundleReader.class).to(GwtormChangeBundleReader.class);
 
     listener().to(CreateDatabase.class);
 

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

@@ -58,6 +58,7 @@ 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.ChangeBundleReader;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager;
@@ -123,6 +124,9 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
   @Inject
   private Sequences seq;
 
+  @Inject
+  private ChangeBundleReader bundleReader;
+
   @Before
   public void setUp() throws Exception {
     assume().that(NoteDbMode.readWrite()).isFalse();
@@ -388,7 +392,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     // Check that the bundles are equal.
     ChangeBundle actual = ChangeBundle.fromNotes(
         plcUtil, notesFactory.create(dbProvider.get(), project, id));
-    ChangeBundle expected = ChangeBundle.fromReviewDb(getUnwrappedDb(), id);
+    ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
     assertThat(actual.differencesFrom(expected)).isEmpty();
   }
 
@@ -439,7 +443,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     // Check that the bundles are equal.
     ChangeNotes notes = notesFactory.create(dbProvider.get(), project, id);
     ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
-    ChangeBundle expected = ChangeBundle.fromReviewDb(getUnwrappedDb(), id);
+    ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
     assertThat(actual.differencesFrom(expected)).isEmpty();
     assertThat(
             Iterables.transform(
@@ -478,7 +482,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     // Check that the bundles are equal.
     ChangeBundle actual = ChangeBundle.fromNotes(
         plcUtil, notesFactory.create(dbProvider.get(), project, id));
-    ChangeBundle expected = ChangeBundle.fromReviewDb(getUnwrappedDb(), id);
+    ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
     assertThat(actual.differencesFrom(expected)).isEmpty();
   }
 
@@ -508,7 +512,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertChangeUpToDate(false, id);
     assertThat(getMetaRef(project, changeMetaRef(id))).isEqualTo(oldMetaId);
     ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
-    ChangeBundle expected = ChangeBundle.fromReviewDb(getUnwrappedDb(), id);
+    ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
     assertThat(actual.differencesFrom(expected)).isEmpty();
     assertChangeUpToDate(false, id);
 
@@ -550,7 +554,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     // Not up to date, but the actual returned state matches anyway.
     assertDraftsUpToDate(false, id, user);
     ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
-    ChangeBundle expected = ChangeBundle.fromReviewDb(getUnwrappedDb(), id);
+    ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
     assertThat(actual.differencesFrom(expected)).isEmpty();
 
     // Another rebuild attempt succeeds
@@ -605,7 +609,7 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
     assertChangeUpToDate(true, id);
     assertDraftsUpToDate(false, id, user);
     ChangeBundle actual = ChangeBundle.fromNotes(plcUtil, notes);
-    ChangeBundle expected = ChangeBundle.fromReviewDb(getUnwrappedDb(), id);
+    ChangeBundle expected = bundleReader.fromReviewDb(getUnwrappedDb(), id);
     assertThat(actual.differencesFrom(expected)).isEmpty();
 
     // Another rebuild attempt succeeds

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

@@ -50,7 +50,6 @@ import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilderImpl;
@@ -84,25 +83,6 @@ public class ChangeBundle {
     REVIEW_DB, NOTE_DB;
   }
 
-  public static ChangeBundle fromReviewDb(ReviewDb db, Change.Id id)
-      throws OrmException {
-    db.changes().beginTransaction(id);
-    try {
-      List<PatchSetApproval> approvals =
-          db.patchSetApprovals().byChange(id).toList();
-      return new ChangeBundle(
-          db.changes().get(id),
-          db.changeMessages().byChange(id),
-          db.patchSets().byChange(id),
-          approvals,
-          db.patchComments().byChange(id),
-          ReviewerSet.fromApprovals(approvals),
-          Source.REVIEW_DB);
-    } finally {
-      db.rollback();
-    }
-  }
-
   public static ChangeBundle fromNotes(PatchLineCommentsUtil plcUtil,
       ChangeNotes notes) throws OrmException {
     return new ChangeBundle(

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

@@ -0,0 +1,23 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gwtorm.server.OrmException;
+
+public interface ChangeBundleReader {
+  ChangeBundle fromReviewDb(ReviewDb db, Change.Id id) throws OrmException;
+}

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

@@ -0,0 +1,53 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.notedb.ChangeBundle.Source;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+import java.util.List;
+
+@Singleton
+public class GwtormChangeBundleReader implements ChangeBundleReader {
+  @Inject
+  GwtormChangeBundleReader() {
+  }
+
+  @Override
+  public ChangeBundle fromReviewDb(ReviewDb db, Change.Id id)
+      throws OrmException {
+    db.changes().beginTransaction(id);
+    try {
+      List<PatchSetApproval> approvals =
+          db.patchSetApprovals().byChange(id).toList();
+      return new ChangeBundle(
+          db.changes().get(id),
+          db.changeMessages().byChange(id),
+          db.patchSets().byChange(id),
+          approvals,
+          db.patchComments().byChange(id),
+          ReviewerSet.fromApprovals(approvals),
+          Source.REVIEW_DB);
+    } finally {
+      db.rollback();
+    }
+  }
+}

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

@@ -53,6 +53,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.git.ChainedReceiveCommands;
 import com.google.gerrit.server.notedb.ChangeBundle;
+import com.google.gerrit.server.notedb.ChangeBundleReader;
 import com.google.gerrit.server.notedb.ChangeDraftUpdate;
 import com.google.gerrit.server.notedb.ChangeNoteUtil;
 import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -116,6 +117,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   static final long MAX_DELTA_MS = SECONDS.toMillis(1);
 
   private final AccountCache accountCache;
+  private final ChangeBundleReader bundleReader;
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
   private final ChangeNoteUtil changeNoteUtil;
   private final ChangeUpdate.Factory updateFactory;
@@ -129,6 +131,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   @Inject
   ChangeRebuilderImpl(SchemaFactory<ReviewDb> schemaFactory,
       AccountCache accountCache,
+      ChangeBundleReader bundleReader,
       ChangeDraftUpdate.Factory draftUpdateFactory,
       ChangeNoteUtil changeNoteUtil,
       ChangeUpdate.Factory updateFactory,
@@ -140,6 +143,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
       @AnonymousCowardName String anonymousCowardName) {
     super(schemaFactory);
     this.accountCache = accountCache;
+    this.bundleReader = bundleReader;
     this.draftUpdateFactory = draftUpdateFactory;
     this.changeNoteUtil = changeNoteUtil;
     this.updateFactory = updateFactory;
@@ -162,7 +166,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
     try (NoteDbUpdateManager manager =
         updateManagerFactory.create(change.getProject())) {
-      buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
+      buildUpdates(manager, bundleReader.fromReviewDb(db, changeId));
       return execute(db, changeId, manager);
     }
   }
@@ -186,7 +190,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
     NoteDbUpdateManager manager =
         updateManagerFactory.create(change.getProject());
-    buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
+    buildUpdates(manager, bundleReader.fromReviewDb(db, changeId));
     manager.stage();
     return manager;
   }
@@ -267,7 +271,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
           new ChainedReceiveCommands(allUsersRepo));
       for (Change.Id changeId : allChanges.get(project)) {
         try {
-          buildUpdates(manager, ChangeBundle.fromReviewDb(db, changeId));
+          buildUpdates(manager, bundleReader.fromReviewDb(db, changeId));
         } catch (NoPatchSetsException e) {
           log.warn(e.getMessage());
         } catch (Throwable t) {

gerrit-server/src/main/java/com/google/gerrit/server/schema/DatabaseModule.java

@@ -18,6 +18,8 @@ import static com.google.inject.Scopes.SINGLETON;
 
 import com.google.gerrit.extensions.config.FactoryModule;
 import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.notedb.ChangeBundleReader;
+import com.google.gerrit.server.notedb.GwtormChangeBundleReader;
 import com.google.gwtorm.jdbc.Database;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Key;
@@ -37,5 +39,6 @@ public class DatabaseModule extends FactoryModule {
         .to(database)
         .in(SINGLETON);
     bind(database).toProvider(ReviewDbDatabaseProvider.class);
+    bind(ChangeBundleReader.class).to(GwtormChangeBundleReader.class);
   }
 }

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

@@ -50,6 +50,8 @@ import com.google.gerrit.server.git.SendEmailExecutor;
 import com.google.gerrit.server.index.IndexModule.IndexType;
 import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
 import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
+import com.google.gerrit.server.notedb.ChangeBundleReader;
+import com.google.gerrit.server.notedb.GwtormChangeBundleReader;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.patch.DiffExecutor;
 import com.google.gerrit.server.schema.DataSourceType;
@@ -175,6 +177,7 @@ public class InMemoryModule extends FactoryModule {
       .to(InMemoryH2Type.class);
     bind(new TypeLiteral<SchemaFactory<ReviewDb>>() {})
         .to(InMemoryDatabase.class);
+    bind(ChangeBundleReader.class).to(GwtormChangeBundleReader.class);
 
     bind(SecureStore.class).to(DefaultSecureStore.class);
 

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

@@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.PatchLineCommentsUtil;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeBundle;
+import com.google.gerrit.server.notedb.ChangeBundleReader;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
 import com.google.inject.Inject;
@@ -48,6 +49,7 @@ public class NoteDbChecker {
   private final Provider<ReviewDb> dbProvider;
   private final GitRepositoryManager repoManager;
   private final TestNotesMigration notesMigration;
+  private final ChangeBundleReader bundleReader;
   private final ChangeNotes.Factory notesFactory;
   private final ChangeRebuilder changeRebuilder;
   private final PatchLineCommentsUtil plcUtil;
@@ -56,11 +58,13 @@ public class NoteDbChecker {
   NoteDbChecker(Provider<ReviewDb> dbProvider,
       GitRepositoryManager repoManager,
       TestNotesMigration notesMigration,
+      ChangeBundleReader bundleReader,
       ChangeNotes.Factory notesFactory,
       ChangeRebuilder changeRebuilder,
       PatchLineCommentsUtil plcUtil) {
     this.dbProvider = dbProvider;
     this.repoManager = repoManager;
+    this.bundleReader = bundleReader;
     this.notesMigration = notesMigration;
     this.notesFactory = notesFactory;
     this.changeRebuilder = changeRebuilder;
@@ -131,7 +135,7 @@ public class NoteDbChecker {
           ReviewDbUtil.intKeyOrdering().sortedCopy(changeIds);
       List<ChangeBundle> expected = new ArrayList<>(sortedIds.size());
       for (Change.Id id : sortedIds) {
-        expected.add(ChangeBundle.fromReviewDb(db, id));
+        expected.add(bundleReader.fromReviewDb(db, id));
       }
       return expected;
     } finally {