ChangeUtil: Return ChangeControls from findChanges

Existing callers all immediately construct ChangeControls from the
returned objects to check visibility, so returning ChangeControls
avoids callers needing their own ChangeControl.GenericFactory.

Change-Id: I7710befac7a4937e634b254c7141e040a442ada2

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

@@ -14,15 +14,12 @@
 
 package com.google.gerrit.server;
 
-import static com.google.gerrit.server.query.change.ChangeData.asChanges;
-
 import com.google.common.base.Function;
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Ordering;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -43,6 +40,7 @@ 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.project.RefControl;
+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.OrmConcurrencyException;
@@ -73,6 +71,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;
@@ -180,6 +179,7 @@ public class ChangeUtil {
   private final Provider<IdentifiedUser> user;
   private final Provider<ReviewDb> db;
   private final Provider<InternalChangeQuery> queryProvider;
+  private final ChangeControl.GenericFactory changeControlFactory;
   private final RevertedSender.Factory revertedSenderFactory;
   private final ChangeInserter.Factory changeInserterFactory;
   private final GitRepositoryManager gitManager;
@@ -193,6 +193,7 @@ public class ChangeUtil {
   ChangeUtil(Provider<IdentifiedUser> user,
       Provider<ReviewDb> db,
       Provider<InternalChangeQuery> queryProvider,
+      ChangeControl.GenericFactory changeControlFactory,
       RevertedSender.Factory revertedSenderFactory,
       ChangeInserter.Factory changeInserterFactory,
       GitRepositoryManager gitManager,
@@ -204,6 +205,7 @@ public class ChangeUtil {
     this.user = user;
     this.db = db;
     this.queryProvider = queryProvider;
+    this.changeControlFactory = changeControlFactory;
     this.revertedSenderFactory = revertedSenderFactory;
     this.changeInserterFactory = changeInserterFactory;
     this.gitManager = gitManager;
@@ -425,34 +427,46 @@ public class ChangeUtil {
    *
    * @param id change identifier, either a numeric ID, a Change-Id, or
    *     project~branch~id triplet.
-   * @return all matching changes, even if they are not visible to the current
+   * @param user user to wrap in controls.
-   *     user.
+   * @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<Change> findChanges(String id)
+  public List<ChangeControl> findChanges(String id, CurrentUser user)
-      throws OrmException, ResourceNotFoundException {
+      throws OrmException {
     // Try legacy id
     if (id.matches("^[1-9][0-9]*$")) {
-      Change c = db.get().changes().get(Change.Id.parse(id));
+      try {
-      if (c != null) {
+        return ImmutableList.of(
-        return ImmutableList.of(c);
+            changeControlFactory.controlFor(Change.Id.parse(id), user));
+      } catch (NoSuchChangeException e) {
+        return Collections.emptyList();
       }
-      return Collections.emptyList();
     }
 
     // Try isolated changeId
     if (!id.contains("~")) {
-      return asChanges(queryProvider.get().byKeyPrefix(id));
+      return asChangeControls(queryProvider.get().byKeyPrefix(id));
     }
 
     // Try change triplet
     Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id);
     if (triplet.isPresent()) {
-      return asChanges(queryProvider.get().byBranchKey(
+      return asChangeControls(queryProvider.get().byBranchKey(
           triplet.get().branch(),
           triplet.get().id()));
     }
 
-    throw new ResourceNotFoundException(id);
+    return Collections.emptyList();
+  }
+
+  private List<ChangeControl> asChangeControls(List<ChangeData> cds)
+      throws OrmException {
+    List<ChangeControl> ctls = new ArrayList<>(cds.size());
+    for (ChangeData cd : cds) {
+      ctls.add(cd.changeControl(user.get()));
+    }
+    return ctls;
   }
 
   private static void deleteOnlyDraftPatchSetPreserveRef(ReviewDb db,

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

@@ -24,11 +24,11 @@ import com.google.gerrit.extensions.restapi.RestCollection;
 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.CurrentUser;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.QueryChanges;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -42,8 +42,8 @@ import java.util.List;
 public class ChangesCollection implements
     RestCollection<TopLevelResource, ChangeResource>,
     AcceptsPost<TopLevelResource> {
+  private final Provider<ReviewDb> db;
   private final Provider<CurrentUser> user;
-  private final ChangeControl.GenericFactory changeControlFactory;
   private final Provider<QueryChanges> queryFactory;
   private final DynamicMap<RestView<ChangeResource>> views;
   private final ChangeUtil changeUtil;
@@ -52,15 +52,15 @@ public class ChangesCollection implements
 
   @Inject
   ChangesCollection(
+      Provider<ReviewDb> db,
       Provider<CurrentUser> user,
-      ChangeControl.GenericFactory changeControlFactory,
       Provider<QueryChanges> queryFactory,
       DynamicMap<RestView<ChangeResource>> views,
       ChangeUtil changeUtil,
       CreateChange createChange,
       ChangeIndexer changeIndexer) {
+    this.db = db;
     this.user = user;
-    this.changeControlFactory = changeControlFactory;
     this.queryFactory = queryFactory;
     this.views = views;
     this.changeUtil = changeUtil;
@@ -81,8 +81,8 @@ public class ChangesCollection implements
   @Override
   public ChangeResource parse(TopLevelResource root, IdString id)
       throws ResourceNotFoundException, OrmException {
-    List<Change> changes = changeUtil.findChanges(id.encoded());
+    List<ChangeControl> ctls = changeUtil.findChanges(id.encoded(), user.get());
-    if (changes.isEmpty()) {
+    if (ctls.isEmpty()) {
       Integer changeId = Ints.tryParse(id.get());
       if (changeId != null) {
         try {
@@ -92,17 +92,15 @@ public class ChangesCollection implements
         }
       }
     }
-    if (changes.size() != 1) {
+    if (ctls.size() != 1) {
       throw new ResourceNotFoundException(id);
     }
 
-    ChangeControl control;
+    ChangeControl ctl = ctls.get(0);
-    try {
+    if (!ctl.isVisible(db.get())) {
-      control = changeControlFactory.validateFor(changes.get(0), user.get());
-    } catch (NoSuchChangeException e) {
       throw new ResourceNotFoundException(id);
     }
-    return new ChangeResource(control);
+    return new ChangeResource(ctl);
   }
 
   public ChangeResource parse(Change.Id id)

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

@@ -44,6 +44,7 @@ import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.git.validators.CommitValidators;
+import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
 import com.google.gerrit.server.project.ProjectResource;
 import com.google.gerrit.server.project.ProjectsCollection;
@@ -154,19 +155,19 @@ public class CreateChange implements
       ObjectId parentCommit;
       List<String> groups;
       if (input.baseChange != null) {
-        List<Change> changes = changeUtil.findChanges(input.baseChange);
+        List<ChangeControl> ctls = changeUtil.findChanges(
-        if (changes.size() != 1) {
+            input.baseChange, rsrc.getControl().getUser());
+        if (ctls.size() != 1) {
           throw new InvalidChangeOperationException(
               "Base change not found: " + input.baseChange);
         }
-        Change change = Iterables.getOnlyElement(changes);
+        ChangeControl ctl = Iterables.getOnlyElement(ctls);
-        if (!rsrc.getControl().controlFor(change).isVisible(db.get())) {
+        if (!ctl.isVisible(db.get())) {
           throw new InvalidChangeOperationException(
               "Base change not found: " + input.baseChange);
         }
-        PatchSet ps = db.get().patchSets().get(
+        PatchSet ps =
-            new PatchSet.Id(change.getId(),
+            db.get().patchSets().get(ctl.getChange().currentPatchSetId());
-            change.currentPatchSetId().get()));
         parentCommit = ObjectId.fromString(ps.getRevision().get());
         groups = ps.getGroups();
       } else {

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

@@ -544,6 +544,23 @@ public class ChangeData {
     return changeControl;
   }
 
+  public ChangeControl changeControl(CurrentUser user) throws OrmException {
+    if (changeControl != null) {
+      throw new IllegalStateException(
+          "user already specified: " + changeControl.getUser());
+    }
+    try {
+      if (change != null) {
+        changeControl = changeControlFactory.controlFor(change, user);
+      } else {
+        changeControl = changeControlFactory.controlFor(legacyId, user);
+      }
+    } catch (NoSuchChangeException e) {
+      throw new OrmException(e);
+    }
+    return changeControl;
+  }
+
   void cacheVisibleTo(ChangeControl ctl) {
     visibleTo = ctl.getUser();
     changeControl = ctl;