ChangeControl.GenericFactory#validateFor: Don't load change from db

All callers of ChangeControl.GenericFactory#validateFor(Change.Id,
CurrentUser) cannot provide the project name, which we would need to
load the change through ChangeNotes.Factory. Hence we need to lookup
the change from the index. Since we request no fields when looking up
the change from the index, rereading it is enforced, hence the change
we get back is not stale.

Move the methods to find changes from ChangeUtil into a separate
class. This is needed because ChangeUtil with all its dependencies
cannot be injected into ChangeControl.GenericFactory, but all those
dependencies are not needed for looking up changes from the index.

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

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java

@@ -52,7 +52,7 @@ import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.GetRevisionActions;
 import com.google.gerrit.server.change.RevisionResource;
@@ -80,7 +80,7 @@ import java.util.Map;
 public class RevisionIT extends AbstractDaemonTest {
 
   @Inject
-  private ChangeUtil changeUtil;
+  private ChangeFinder changeFinder;
 
   @Inject
   private GetRevisionActions getRevisionActions;
@@ -693,7 +693,7 @@ public class RevisionIT extends AbstractDaemonTest {
   private RevisionResource parseRevisionResource(PushOneCommit.Result r)
       throws Exception {
     PatchSet.Id psId = r.getPatchSetId();
-    List<ChangeControl> ctls = changeUtil.findChanges(
+    List<ChangeControl> ctls = changeFinder.find(
         Integer.toString(psId.getParentKey().get()), atrScope.get().getUser());
     assertThat(ctls).hasSize(1);
     return revisions.parse(

gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java

@@ -0,0 +1,102 @@
+// 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;
+
+import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.primitives.Ints;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.change.ChangeTriplet;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+@Singleton
+public class ChangeFinder {
+  private final Provider<InternalChangeQuery> queryProvider;
+
+  @Inject
+  ChangeFinder(Provider<InternalChangeQuery> queryProvider) {
+    this.queryProvider = queryProvider;
+  }
+
+  /**
+   * Find changes matching the given identifier.
+   *
+   * @param id change identifier, either a numeric ID, a Change-Id, or
+   *     project~branch~id triplet.
+   * @param user user to wrap in controls.
+   * @return possibly-empty list of controls for all matching changes,
+   *     corresponding to the given user; may or may not be visible.
+   * @throws OrmException if an error occurred querying the database.
+   */
+  public List<ChangeControl> find(String id, CurrentUser user)
+      throws OrmException {
+    // Use the index to search for changes, but don't return any stored fields,
+    // to force rereading in case the index is stale.
+    InternalChangeQuery query = queryProvider.get()
+        .setRequestedFields(ImmutableSet.<String> of());
+
+    // Try legacy id
+    if (!id.isEmpty() && id.charAt(0) != '0') {
+      Integer n = Ints.tryParse(id);
+      if (n != null) {
+        return asChangeControls(query.byLegacyChangeId(new Change.Id(n)), user);
+      }
+    }
+
+    // Try isolated changeId
+    if (!id.contains("~")) {
+      return asChangeControls(query.byKeyPrefix(id), user);
+    }
+
+    // Try change triplet
+    Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id);
+    if (triplet.isPresent()) {
+      return asChangeControls(query.byBranchKey(
+          triplet.get().branch(),
+          triplet.get().id()),
+          user);
+    }
+
+    return Collections.emptyList();
+  }
+
+  public List<ChangeControl> find(Change.Id id, CurrentUser user)
+      throws OrmException {
+    // Use the index to search for changes, but don't return any stored fields,
+    // to force rereading in case the index is stale.
+    InternalChangeQuery query = queryProvider.get()
+        .setRequestedFields(ImmutableSet.<String> of());
+    return asChangeControls(query.byLegacyChangeId(id), user);
+  }
+
+  private List<ChangeControl> asChangeControls(List<ChangeData> cds,
+      CurrentUser user) throws OrmException {
+    List<ChangeControl> ctls = new ArrayList<>(cds.size());
+    for (ChangeData cd : cds) {
+      ctls.add(cd.changeControl(user));
+    }
+    return ctls;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java

@@ -15,10 +15,7 @@
 package com.google.gerrit.server;
 
 import com.google.common.base.Function;
-import com.google.common.base.Optional;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Ordering;
-import com.google.common.primitives.Ints;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -29,7 +26,6 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeMessages;
-import com.google.gerrit.server.change.ChangeTriplet;
 import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.UpdateException;
@@ -39,8 +35,6 @@ import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.util.IdGenerator;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -65,9 +59,7 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.text.MessageFormat;
-import java.util.ArrayList;
 import java.util.Collections;
-import java.util.List;
 import java.util.Map;
 
 @Singleton
@@ -163,7 +155,6 @@ public class ChangeUtil {
   private final Provider<CurrentUser> user;
   private final Provider<ReviewDb> db;
   private final Sequences seq;
-  private final Provider<InternalChangeQuery> queryProvider;
   private final PatchSetUtil psUtil;
   private final RevertedSender.Factory revertedSenderFactory;
   private final ChangeInserter.Factory changeInserterFactory;
@@ -176,7 +167,6 @@ public class ChangeUtil {
   ChangeUtil(Provider<CurrentUser> user,
       Provider<ReviewDb> db,
       Sequences seq,
-      Provider<InternalChangeQuery> queryProvider,
       PatchSetUtil psUtil,
       RevertedSender.Factory revertedSenderFactory,
       ChangeInserter.Factory changeInserterFactory,
@@ -187,7 +177,6 @@ public class ChangeUtil {
     this.user = user;
     this.db = db;
     this.seq = seq;
-    this.queryProvider = queryProvider;
     this.psUtil = psUtil;
     this.revertedSenderFactory = revertedSenderFactory;
     this.changeInserterFactory = changeInserterFactory;
@@ -315,66 +304,6 @@ public class ChangeUtil {
     }
   }
 
-  /**
-   * Find changes matching the given identifier.
-   *
-   * @param id change identifier, either a numeric ID, a Change-Id, or
-   *     project~branch~id triplet.
-   * @param user user to wrap in controls.
-   * @return possibly-empty list of controls for all matching changes,
-   *     corresponding to the given user; may or may not be visible.
-   * @throws OrmException if an error occurred querying the database.
-   */
-  public List<ChangeControl> findChanges(String id, CurrentUser user)
-      throws OrmException {
-    // Use the index to search for changes, but don't return any stored fields,
-    // to force rereading in case the index is stale.
-    InternalChangeQuery query = queryProvider.get()
-        .setRequestedFields(ImmutableSet.<String> of());
-
-    // Try legacy id
-    if (!id.isEmpty() && id.charAt(0) != '0') {
-      Integer n = Ints.tryParse(id);
-      if (n != null) {
-        return asChangeControls(query.byLegacyChangeId(new Change.Id(n)), user);
-      }
-    }
-
-    // Try isolated changeId
-    if (!id.contains("~")) {
-      return asChangeControls(query.byKeyPrefix(id), user);
-    }
-
-    // Try change triplet
-    Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id);
-    if (triplet.isPresent()) {
-      return asChangeControls(query.byBranchKey(
-          triplet.get().branch(),
-          triplet.get().id()),
-          user);
-    }
-
-    return Collections.emptyList();
-  }
-
-  public List<ChangeControl> findChanges(Change.Id id, CurrentUser user)
-      throws OrmException {
-    // Use the index to search for changes, but don't return any stored fields,
-    // to force rereading in case the index is stale.
-    InternalChangeQuery query = queryProvider.get()
-        .setRequestedFields(ImmutableSet.<String> of());
-    return asChangeControls(query.byLegacyChangeId(id), user);
-  }
-
-  private List<ChangeControl> asChangeControls(List<ChangeData> cds,
-      CurrentUser user) throws OrmException {
-    List<ChangeControl> ctls = new ArrayList<>(cds.size());
-    for (ChangeData cd : cds) {
-      ctls.add(cd.changeControl(user));
-    }
-    return ctls;
-  }
-
   public static PatchSet.Id nextPatchSetId(PatchSet.Id id) {
     return new PatchSet.Id(id.getParentKey(), id.get() + 1);
   }

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

@@ -25,7 +25,7 @@ import com.google.gerrit.extensions.restapi.RestView;
 import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.project.ChangeControl;
@@ -46,7 +46,7 @@ public class ChangesCollection implements
   private final Provider<CurrentUser> user;
   private final Provider<QueryChanges> queryFactory;
   private final DynamicMap<RestView<ChangeResource>> views;
-  private final ChangeUtil changeUtil;
+  private final ChangeFinder changeFinder;
   private final CreateChange createChange;
   private final ChangeIndexer changeIndexer;
 
@@ -56,14 +56,14 @@ public class ChangesCollection implements
       Provider<CurrentUser> user,
       Provider<QueryChanges> queryFactory,
       DynamicMap<RestView<ChangeResource>> views,
-      ChangeUtil changeUtil,
+      ChangeFinder changeFinder,
       CreateChange createChange,
       ChangeIndexer changeIndexer) {
     this.db = db;
     this.user = user;
     this.queryFactory = queryFactory;
     this.views = views;
-    this.changeUtil = changeUtil;
+    this.changeFinder = changeFinder;
     this.createChange = createChange;
     this.changeIndexer = changeIndexer;
   }
@@ -81,7 +81,8 @@ public class ChangesCollection implements
   @Override
   public ChangeResource parse(TopLevelResource root, IdString id)
       throws ResourceNotFoundException, OrmException {
-    List<ChangeControl> ctls = changeUtil.findChanges(id.encoded(), user.get());
+    List<ChangeControl> ctls =
+        changeFinder.find(id.encoded(), user.get());
     if (ctls.isEmpty()) {
       Integer changeId = Ints.tryParse(id.get());
       if (changeId != null) {
@@ -108,7 +109,7 @@ public class ChangesCollection implements
 
   public ChangeResource parse(Change.Id id)
       throws ResourceNotFoundException, OrmException {
-    List<ChangeControl> ctls = changeUtil.findChanges(id, user.get());
+    List<ChangeControl> ctls = changeFinder.find(id, user.get());
     if (ctls.isEmpty()) {
       try {
         changeIndexer.delete(id);

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

@@ -33,7 +33,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
@@ -84,7 +84,7 @@ public class CreateChange implements
   private final ProjectsCollection projectsCollection;
   private final ChangeInserter.Factory changeInserterFactory;
   private final ChangeJson.Factory jsonFactory;
-  private final ChangeUtil changeUtil;
+  private final ChangeFinder changeFinder;
   private final BatchUpdate.Factory updateFactory;
   private final PatchSetUtil psUtil;
   private final boolean allowDrafts;
@@ -98,7 +98,7 @@ public class CreateChange implements
       ProjectsCollection projectsCollection,
       ChangeInserter.Factory changeInserterFactory,
       ChangeJson.Factory json,
-      ChangeUtil changeUtil,
+      ChangeFinder changeFinder,
       BatchUpdate.Factory updateFactory,
       PatchSetUtil psUtil,
       @GerritServerConfig Config config) {
@@ -110,7 +110,7 @@ public class CreateChange implements
     this.projectsCollection = projectsCollection;
     this.changeInserterFactory = changeInserterFactory;
     this.jsonFactory = json;
-    this.changeUtil = changeUtil;
+    this.changeFinder = changeFinder;
     this.updateFactory = updateFactory;
     this.psUtil = psUtil;
     this.allowDrafts = config.getBoolean("change", "allowDrafts", true);
@@ -162,7 +162,7 @@ public class CreateChange implements
       ObjectId parentCommit;
       List<String> groups;
       if (input.baseChange != null) {
-        List<ChangeControl> ctls = changeUtil.findChanges(
+        List<ChangeControl> ctls = changeFinder.find(
             input.baseChange, rsrc.getControl().getUser());
         if (ctls.size() != 1) {
           throw new InvalidChangeOperationException(

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

@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -47,15 +48,18 @@ public class ChangeControl {
     private final ProjectControl.GenericFactory projectControl;
     private final Provider<ReviewDb> db;
     private final ChangeNotes.Factory notesFactory;
+    private final ChangeFinder changeFinder;
 
     @Inject
     GenericFactory(
         ProjectControl.GenericFactory p,
         Provider<ReviewDb> d,
-        ChangeNotes.Factory n) {
+        ChangeNotes.Factory n,
+        ChangeFinder f) {
       projectControl = p;
       db = d;
       notesFactory = n;
+      changeFinder = f;
     }
 
     public ChangeControl controlFor(Project.NameKey project, Change.Id changeId,
@@ -88,11 +92,11 @@ public class ChangeControl {
 
     public ChangeControl validateFor(Change.Id changeId, CurrentUser user)
         throws NoSuchChangeException, OrmException {
-      Change change = db.get().changes().get(changeId);
-      if (change == null) {
+      List<ChangeControl> ctls = changeFinder.find(changeId, user);
+      if (ctls.size() != 1) {
         throw new NoSuchChangeException(changeId);
       }
-      return validateFor(change, user);
+      return validateFor(ctls.get(0).getChange(), user);
     }
 
     public ChangeControl validateFor(Change change, CurrentUser user)

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

@@ -20,7 +20,7 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.ChangeFinder;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.ChangesCollection;
@@ -94,7 +94,7 @@ public class SetReviewersCommand extends SshCommand {
   private ChangesCollection changesCollection;
 
   @Inject
-  private ChangeUtil changeUtil;
+  private ChangeFinder changeFinder;
 
   private Set<Account.Id> toRemove = new HashSet<>();
 
@@ -165,7 +165,7 @@ public class SetReviewersCommand extends SshCommand {
 
   private void addChangeImpl(String id) throws UnloggedFailure, OrmException {
     List<ChangeControl> matched =
-        changeUtil.findChanges(id, userProvider.get());
+        changeFinder.find(id, userProvider.get());
     List<ChangeControl> toAdd = new ArrayList<>(changes.size());
     for (ChangeControl ctl : matched) {
       if (!changes.containsKey(ctl.getId()) && inProject(ctl.getProject())