Load change in ChangeNotes.Factory

ChangeNotes.Factory now loads the change from the database. This
should become the only place where changes are loaded from the
database, but for this more work is required in follow-up changes.

We need to load the change for the database also when notedb is
enabled. This is because for now we still want to write updates to the
database when notedb is enabled. In order to write update to the
database the change instance must be created by loading it from the
database because otherwise its row version is not set and then all
updates to the database fail with OrmConcurrencyException.

The signature of the ChangeNotes.Factory#create is changed to require
project name and a change ID instead of a change. Project name is not
needed yet, since the change is always loaded by change ID from the
database, but later when reviewdb is gone we will need the project
name to instantiate the change from notedb.

For changes that have been loaded from the secondary index we don't
want to reload them from the database. This is why a new
ChangeNotes.Factory#createFromIndexedChange method is added that still
takes a change as input.

Change-Id: Idc274d6938f989b4d7fa49ee496d7ac84c749455
Signed-off-by: Edwin Kempin <ekempin@google.com>

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

@@ -273,8 +273,9 @@ public class PushOneCommit {
 
     private void assertReviewers(Change c, TestAccount... expectedReviewers)
         throws OrmException {
-      Iterable<Account.Id> actualIds =
+      Iterable<Account.Id> actualIds = approvalsUtil
-          approvalsUtil.getReviewers(db, notesFactory.create(db, c)).values();
+          .getReviewers(db, notesFactory.create(db, c.getProject(), c.getId()))
+          .values();
       assertThat(actualIds).containsExactlyElementsIn(
           Sets.newHashSet(TestAccount.ids(expectedReviewers)));
     }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java

@@ -223,8 +223,8 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
 
   private PatchSetApproval getSubmitter(PatchSet.Id patchSetId)
       throws OrmException {
-    Change c = db.changes().get(patchSetId.getParentKey());
+    ChangeNotes notes = changeNotesFactory
-    ChangeNotes notes = changeNotesFactory.create(db, c).load();
+        .create(db, project, patchSetId.getParentKey()).load();
     return approvalsUtil.getSubmitter(db, notes, patchSetId);
   }
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java

@@ -313,8 +313,9 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
 
   protected void assertSubmitter(String changeId, int psId)
       throws OrmException {
-    ChangeNotes cn = notesFactory.create(db,
+    Change c =
-        getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change());
+        getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change();
+    ChangeNotes cn = notesFactory.create(db, c.getProject(), c.getId());
     PatchSetApproval submitter = approvalsUtil.getSubmitter(
         db, cn, new PatchSet.Id(cn.getChangeId(), psId));
     assertThat(submitter).isNotNull();
@@ -324,8 +325,9 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
 
   protected void assertNoSubmitter(String changeId, int psId)
       throws OrmException {
-    ChangeNotes cn = notesFactory.create(db,
+    Change c =
-        getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change());
+        getOnlyElement(queryProvider.get().byKeyPrefix(changeId)).change();
+    ChangeNotes cn = notesFactory.create(db, c.getProject(), c.getId());
     PatchSetApproval submitter = approvalsUtil.getSubmitter(
         db, cn, new PatchSet.Id(cn.getChangeId(), psId));
     assertThat(submitter).isNull();

gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java

@@ -309,8 +309,9 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
               .build());
     }
 
-    private ChangeNotes newNotes(ReviewDb db, Change change) {
+    private ChangeNotes newNotes(ReviewDb db, Change change)
-      return notesFactory.create(db, change);
+        throws OrmException {
+      return notesFactory.create(db, change.getProject(), change.getId());
     }
 
     private static Optional<Path> hook(Config config, Path path, String name) {

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

@@ -587,7 +587,7 @@ public class ConsistencyChecker {
         if (c == null) {
           throw new OrmException("Change missing: " + cid);
         }
-        ChangeNotes notes = notesFactory.create(db, c);
+        ChangeNotes notes = notesFactory.create(db, c.getProject(), cid);
 
         if (psId.equals(c.currentPatchSetId())) {
           List<PatchSet> all = Lists.newArrayList(db.patchSets().byChange(cid));

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

@@ -31,8 +31,8 @@ import com.google.common.collect.Multimaps;
 import com.google.common.collect.SetMultimap;
 import com.google.common.collect.Sets;
 import com.google.common.collect.SortedSetMultimap;
-import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.PatchSetUtil;
 import com.google.gerrit.server.change.RevisionResource;
@@ -115,18 +115,15 @@ public class GroupCollector {
 
   public static GroupCollector create(Multimap<ObjectId, Ref> changeRefsById,
       final ReviewDb db, final PatchSetUtil psUtil,
-      final ChangeNotes.Factory notesFactory) {
+      final ChangeNotes.Factory notesFactory, final Project.NameKey project) {
     return new GroupCollector(
         transformRefs(changeRefsById),
         new Lookup() {
           @Override
           public List<String> lookup(PatchSet.Id psId) throws OrmException {
-            // TODO(dborowitz): Shouldn't have to look up Change.
+            // TODO(dborowitz): Reuse open repository from caller.
-            Change c = db.changes().get(psId.getParentKey());
+            ChangeNotes notes =
-            if (c == null) {
+                notesFactory.create(db, project, psId.getParentKey());
-              return null;
-            }
-            ChangeNotes notes = notesFactory.create(db, c);
             PatchSet ps = psUtil.get(db, notes, psId);
             return ps != null ? ps.getGroups() : null;
           }

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

@@ -1526,8 +1526,8 @@ public class ReceiveCommits {
     newChanges = Lists.newArrayList();
 
     SetMultimap<ObjectId, Ref> existing = changeRefsById();
-    GroupCollector groupCollector =
+    GroupCollector groupCollector = GroupCollector.create(refsById, db, psUtil,
-        GroupCollector.create(refsById, db, psUtil, notesFactory);
+        notesFactory, project.getNameKey());
 
     rp.getRevWalk().reset();
     rp.getRevWalk().sort(RevSort.TOPO);

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

@@ -50,7 +50,7 @@ public abstract class AbstractChangeNotes<T> extends VersionedMetaData {
     if (loaded) {
       return self();
     }
-    if (!migration.enabled()) {
+    if (!migration.enabled() || changeId == null) {
       loadDefaults();
       return self();
     }

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

@@ -116,16 +116,27 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       this.allUsersProvider = allUsersProvider;
     }
 
-    public ChangeNotes create(@SuppressWarnings("unused") ReviewDb db,
+    public ChangeNotes create(ReviewDb db, Project.NameKey project,
-        Change change) {
+        Change.Id changeId) throws OrmException {
-      return new ChangeNotes(repoManager, migration, allUsersProvider, change);
+      Change change = db.changes().get(changeId);
+      // TODO: Throw NoSuchChangeException when the change is not found in the
+      // database
+      return new ChangeNotes(repoManager, migration, allUsersProvider, project,
+          change);
+    }
+
+    public ChangeNotes createFromIndexedChange(Change change) {
+      return new ChangeNotes(repoManager, migration, allUsersProvider,
+          change.getProject(), change);
     }
 
     public ChangeNotes createForNew(Change change) {
-      return new ChangeNotes(repoManager, migration, allUsersProvider, change);
+      return new ChangeNotes(repoManager, migration, allUsersProvider,
+          change.getProject(), change);
     }
   }
 
+  private final Project.NameKey project;
   private final Change change;
   private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
   private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
@@ -147,10 +158,12 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   @VisibleForTesting
   public ChangeNotes(GitRepositoryManager repoManager, NotesMigration migration,
-      AllUsersNameProvider allUsersProvider, Change change) {
+      AllUsersNameProvider allUsersProvider, Project.NameKey project,
-    super(repoManager, migration, change.getId());
+      Change change) {
+    super(repoManager, migration, change != null ? change.getId() : null);
     this.allUsers = allUsersProvider.get();
-    this.change = new Change(change);
+    this.project = project;
+    this.change = change != null ? new Change(change) : null;
   }
 
   public Change getChange() {
@@ -283,7 +296,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       return;
     }
     try (RevWalk walk = new RevWalk(reader);
-        ChangeNotesParser parser = new ChangeNotesParser(change.getProject(),
+        ChangeNotesParser parser = new ChangeNotesParser(project,
             change.getId(), rev, walk, repoManager)) {
       parser.parseAll();
 
@@ -297,7 +310,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       noteMap = parser.noteMap;
       revisionNotes = parser.revisionNotes;
       change.setKey(new Change.Key(parser.changeId));
-      change.setDest(new Branch.NameKey(getProjectName(), parser.branch));
+      change.setDest(new Branch.NameKey(project, parser.branch));
       change.setTopic(Strings.emptyToNull(parser.topic));
       change.setCreatedOn(parser.createdOn);
       change.setLastUpdatedOn(parser.lastUpdatedOn);
@@ -352,6 +365,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   @Override
   public Project.NameKey getProjectName() {
-    return getChange().getProject();
+    return project;
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -121,10 +121,10 @@ public class ChangeControl {
       this.approvalsUtil = approvalsUtil;
     }
 
-    @SuppressWarnings("unused")
+    ChangeControl create(RefControl refControl, Project.NameKey project,
-    ChangeControl create(RefControl refControl, Change change)
+        Change.Id changeId) throws OrmException {
-        throws OrmException {
+      return create(refControl,
-      return create(refControl, notesFactory.create(db, change));
+          notesFactory.create(db, project, changeId));
     }
 
     ChangeControl create(RefControl refControl, ChangeNotes notes) {

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java

@@ -195,7 +195,8 @@ public class ProjectControl {
   }
 
   public ChangeControl controlFor(Change change) throws OrmException {
-    return changeControlFactory.create(controlForRef(change.getDest()), change);
+    return changeControlFactory.create(controlForRef(change.getDest()),
+        change.getProject(), change.getId());
   }
 
   public ChangeControl controlFor(ChangeNotes notes) {

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java

@@ -597,7 +597,8 @@ public class ChangeData {
 
   public ChangeNotes notes() throws OrmException {
     if (notes == null) {
-      notes = notesFactory.create(db, change());
+      Change c = change();
+      notes = notesFactory.create(db, c.getProject(), c.getId());
     }
     return notes;
   }

gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java

@@ -57,6 +57,7 @@ import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.index.IndexConfig;
 import com.google.gerrit.server.index.IndexRewriter;
 import com.google.gerrit.server.index.Schema;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ListChildProjects;
@@ -161,6 +162,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     final IdentifiedUser.GenericFactory userFactory;
     final CapabilityControl.Factory capabilityControlFactory;
     final ChangeControl.GenericFactory changeControlGenericFactory;
+    final ChangeNotes.Factory notesFactory;
     final ChangeData.Factory changeDataFactory;
     final FieldDef.FillArgs fillArgs;
     final PatchLineCommentsUtil plcUtil;
@@ -192,6 +194,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         Provider<CurrentUser> self,
         CapabilityControl.Factory capabilityControlFactory,
         ChangeControl.GenericFactory changeControlGenericFactory,
+        ChangeNotes.Factory notesFactory,
         ChangeData.Factory changeDataFactory,
         FieldDef.FillArgs fillArgs,
         PatchLineCommentsUtil plcUtil,
@@ -211,12 +214,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         Provider<ListMembers> listMembers,
         @GerritServerConfig Config cfg) {
       this(db, queryProvider, rewriter, opFactories, userFactory, self,
-          capabilityControlFactory, changeControlGenericFactory,
+          capabilityControlFactory, changeControlGenericFactory, notesFactory,
           changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
           allProjectsName, allUsersName, patchListCache, repoManager,
-          projectCache, listChildProjects, submitDryRun,
+          projectCache, listChildProjects, submitDryRun, conflictsCache,
-          conflictsCache, trackingFooters,
+          trackingFooters, indexes != null ? indexes.getSearchIndex() : null,
-          indexes != null ? indexes.getSearchIndex() : null,
           indexConfig, listMembers,
           cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true));
     }
@@ -230,6 +232,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
         Provider<CurrentUser> self,
         CapabilityControl.Factory capabilityControlFactory,
         ChangeControl.GenericFactory changeControlGenericFactory,
+        ChangeNotes.Factory notesFactory,
         ChangeData.Factory changeDataFactory,
         FieldDef.FillArgs fillArgs,
         PatchLineCommentsUtil plcUtil,
@@ -255,6 +258,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
      this.userFactory = userFactory;
      this.self = self;
      this.capabilityControlFactory = capabilityControlFactory;
+     this.notesFactory = notesFactory;
      this.changeControlGenericFactory = changeControlGenericFactory;
      this.changeDataFactory = changeDataFactory;
      this.fillArgs = fillArgs;
@@ -279,7 +283,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
     Arguments asUser(CurrentUser otherUser) {
       return new Arguments(db, queryProvider, rewriter, opFactories, userFactory,
           Providers.of(otherUser),
-          capabilityControlFactory, changeControlGenericFactory,
+          capabilityControlFactory, changeControlGenericFactory, notesFactory,
           changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
           allProjectsName, allUsersName, patchListCache, repoManager,
           projectCache, listChildProjects, submitDryRun,
@@ -729,9 +733,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
   }
 
   public Predicate<ChangeData> visibleto(CurrentUser user) {
-    return new IsVisibleToPredicate(args.db, //
+    return new IsVisibleToPredicate(args.db, args.notesFactory,
-        args.changeControlGenericFactory, //
+        args.changeControlGenericFactory, user);
-        user);
   }
 
   public Predicate<ChangeData> is_visible() throws QueryParseException {

gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsVisibleToPredicate.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.query.change;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.OperatorPredicate;
@@ -36,13 +37,15 @@ class IsVisibleToPredicate extends OperatorPredicate<ChangeData> {
   }
 
   private final Provider<ReviewDb> db;
+  private final ChangeNotes.Factory notesFactory;
   private final ChangeControl.GenericFactory changeControl;
   private final CurrentUser user;
 
-  IsVisibleToPredicate(Provider<ReviewDb> db,
+  IsVisibleToPredicate(Provider<ReviewDb> db, ChangeNotes.Factory notesFactory,
       ChangeControl.GenericFactory changeControlFactory, CurrentUser user) {
     super(ChangeQueryBuilder.FIELD_VISIBLETO, describe(user));
     this.db = db;
+    this.notesFactory = notesFactory;
     this.changeControl = changeControlFactory;
     this.user = user;
   }
@@ -58,7 +61,8 @@ class IsVisibleToPredicate extends OperatorPredicate<ChangeData> {
         return false;
       }
 
-      ChangeControl cc = changeControl.controlFor(c, user);
+      ChangeNotes notes = notesFactory.createFromIndexedChange(c);
+      ChangeControl cc = changeControl.controlFor(notes, user);
       if (cc.isVisible(db.get())) {
         cd.cacheVisibleTo(cc);
         return true;

gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java

@@ -31,6 +31,7 @@ import com.google.gerrit.server.index.IndexCollection;
 import com.google.gerrit.server.index.IndexConfig;
 import com.google.gerrit.server.index.IndexPredicate;
 import com.google.gerrit.server.index.IndexRewriter;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.query.Predicate;
 import com.google.gerrit.server.query.QueryParseException;
@@ -48,6 +49,7 @@ public class QueryProcessor {
   private final Provider<ReviewDb> db;
   private final Provider<CurrentUser> userProvider;
   private final ChangeControl.GenericFactory changeControlFactory;
+  private final ChangeNotes.Factory notesFactory;
   private final IndexCollection indexes;
   private final IndexRewriter rewriter;
   private final IndexConfig indexConfig;
@@ -62,6 +64,7 @@ public class QueryProcessor {
   QueryProcessor(Provider<ReviewDb> db,
       Provider<CurrentUser> userProvider,
       ChangeControl.GenericFactory changeControlFactory,
+      ChangeNotes.Factory notesFactory,
       IndexCollection indexes,
       IndexRewriter rewriter,
       IndexConfig indexConfig,
@@ -69,6 +72,7 @@ public class QueryProcessor {
     this.db = db;
     this.userProvider = userProvider;
     this.changeControlFactory = changeControlFactory;
+    this.notesFactory = notesFactory;
     this.indexes = indexes;
     this.rewriter = rewriter;
     this.indexConfig = indexConfig;
@@ -138,7 +142,8 @@ public class QueryProcessor {
     Timer0.Context context = metrics.executionTime.start();
 
     Predicate<ChangeData> visibleToMe = enforceVisibility
-        ? new IsVisibleToPredicate(db, changeControlFactory, userProvider.get())
+        ? new IsVisibleToPredicate(db, notesFactory, changeControlFactory,
+            userProvider.get())
         : null;
     int cnt = queries.size();
 

gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java

@@ -27,7 +27,7 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
           FakeQueryBuilder.class),
         new ChangeQueryBuilder.Arguments(null, null, null, null, null, null,
           null, null, null, null, null, null, null, null, null, null, null,
-          null, null, indexes, null, null, null, null, null, null));
+          null, null, null, indexes, null, null, null, null, null, null));
   }
 
   @Operator

gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java

@@ -186,7 +186,8 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
   }
 
   protected ChangeNotes newNotes(Change c) throws OrmException {
-    return new ChangeNotes(repoManager, MIGRATION, allUsers, c).load();
+    return new ChangeNotes(repoManager, MIGRATION, allUsers, c.getProject(), c)
+        .load();
   }
 
   protected static SubmitRecord submitRecord(String status,

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

@@ -139,8 +139,9 @@ public class TestChanges {
     expect(ctl.getChange()).andStubReturn(c);
     expect(ctl.getProject()).andStubReturn(new Project(c.getProject()));
     expect(ctl.getUser()).andStubReturn(user);
-    ChangeNotes notes = new ChangeNotes(repoManager, migration, allUsers, c)
+    ChangeNotes notes =
-        .load();
+        new ChangeNotes(repoManager, migration, allUsers, c.getProject(), c)
+            .load();
     expect(ctl.getNotes()).andStubReturn(notes);
     expect(ctl.getId()).andStubReturn(c.getId());
     EasyMock.replay(ctl);

gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PatchSetParser.java

@@ -127,7 +127,7 @@ public class PatchSetParser {
     if (c == null) {
       throw error("\"" + changeId + "\" no such change");
     }
-    return notesFactory.create(db.get(), c);
+    return notesFactory.create(db.get(), c.getProject(), changeId);
   }
 
   private static boolean inProject(Change change,