Refactor ChangesCollection to be injectable

Remove the user specific control factory, allowing the collection
to be injected from the system module rather than a higher level
request scoped module. With this refactoring in place it is now
possible to reuse ChangesCollection in other REST APIs, such as
to call its parse() method to lookup a change as a child member.

Change-Id: I8d14b3b9c2dab3cc9f29d968c6a15d0ef6835a04

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

@@ -31,6 +31,9 @@ import static com.google.gerrit.common.changes.ListChangesOption.REVIEWED;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Objects;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
 import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
@@ -64,6 +67,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.PatchSetInfo.ParentInfo;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.UserIdentity;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.AnonymousUser;
@@ -77,7 +81,8 @@ import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectControl;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.ResultSet;
@@ -87,6 +92,7 @@ import com.google.inject.Provider;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.Collection;
 import java.util.Collections;
@@ -97,6 +103,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
+import java.util.concurrent.ExecutionException;
 
 public class ChangeJson {
   private static final Logger log = LoggerFactory.getLogger(ChangeJson.class);
@@ -122,7 +129,7 @@ public class ChangeJson {
   private final Provider<CurrentUser> userProvider;
   private final AnonymousUser anonymous;
   private final IdentifiedUser.GenericFactory userFactory;
-  private final ChangeControl.GenericFactory changeControlGenericFactory;
+  private final ProjectControl.GenericFactory projectControlFactory;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final FileInfoJson fileInfoJson;
   private final AccountInfo.Loader.Factory accountLoaderFactory;
@@ -131,20 +138,20 @@ public class ChangeJson {
   private final DynamicMap<RestView<ChangeResource>> changes;
   private final Revisions revisions;
 
-  private ChangeControl.Factory changeControlUserFactory;
   private EnumSet<ListChangesOption> options;
   private AccountInfo.Loader accountLoader;
   private ChangeControl lastControl;
   private Set<Change.Id> reviewed;
+  private LoadingCache<Project.NameKey, ProjectControl> projectControls;
 
   @Inject
   ChangeJson(
       Provider<ReviewDb> db,
       LabelNormalizer ln,
-      Provider<CurrentUser> userProvider,
+      Provider<CurrentUser> user,
       AnonymousUser au,
       IdentifiedUser.GenericFactory uf,
-      ChangeControl.GenericFactory ccf,
+      ProjectControl.GenericFactory pcf,
       PatchSetInfoFactory psi,
       FileInfoJson fileInfoJson,
       AccountInfo.Loader.Factory ailf,
@@ -154,10 +161,10 @@ public class ChangeJson {
       Revisions revisions) {
     this.db = db;
     this.labelNormalizer = ln;
-    this.userProvider = userProvider;
+    this.userProvider = user;
     this.anonymous = au;
     this.userFactory = uf;
-    this.changeControlGenericFactory = ccf;
+    this.projectControlFactory = pcf;
     this.patchSetInfoFactory = psi;
     this.fileInfoJson = fileInfoJson;
     this.accountLoaderFactory = ailf;
@@ -167,6 +174,15 @@ public class ChangeJson {
     this.revisions = revisions;
 
     options = EnumSet.noneOf(ListChangesOption.class);
+    projectControls = CacheBuilder.newBuilder()
+      .concurrencyLevel(1)
+      .build(new CacheLoader<Project.NameKey, ProjectControl>() {
+        @Override
+        public ProjectControl load(Project.NameKey key)
+            throws NoSuchProjectException, IOException {
+          return projectControlFactory.controlFor(key, userProvider.get());
+        }
+      });
   }
 
   public ChangeJson addOption(ListChangesOption o) {
@@ -179,11 +195,6 @@ public class ChangeJson {
     return this;
   }
 
-  public ChangeJson setChangeControlFactory(ChangeControl.Factory cf) {
-    changeControlUserFactory = cf;
-    return this;
-  }
-
   public ChangeInfo format(ChangeResource rsrc) throws OrmException {
     return format(new ChangeData(rsrc.getControl()));
   }
@@ -321,13 +332,12 @@ public class ChangeJson {
     }
 
     try {
-      if (changeControlUserFactory != null) {
+      Change change = cd.change(db);
-        ctrl = changeControlUserFactory.controlFor(cd.change(db));
+      if (change == null) {
-      } else {
+        return null;
-        ctrl = changeControlGenericFactory.controlFor(cd.change(db),
-            userProvider.get());
       }
-    } catch (NoSuchChangeException e) {
+      ctrl = projectControls.get(change.getProject()).controlFor(change);
+    } catch (ExecutionException e) {
       return null;
     }
     lastControl = ctrl;

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

@@ -23,6 +23,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.CurrentUser;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.query.change.QueryChanges;
@@ -37,17 +38,20 @@ import java.util.List;
 public class ChangesCollection implements
     RestCollection<TopLevelResource, ChangeResource> {
   private final Provider<ReviewDb> db;
-  private final ChangeControl.Factory changeControlFactory;
+  private final Provider<CurrentUser> user;
+  private final ChangeControl.GenericFactory changeControlFactory;
   private final Provider<QueryChanges> queryFactory;
   private final DynamicMap<RestView<ChangeResource>> views;
 
   @Inject
   ChangesCollection(
       Provider<ReviewDb> dbProvider,
-      ChangeControl.Factory changeControlFactory,
+      Provider<CurrentUser> user,
+      ChangeControl.GenericFactory changeControlFactory,
       Provider<QueryChanges> queryFactory,
       DynamicMap<RestView<ChangeResource>> views) {
     this.db = dbProvider;
+    this.user = user;
     this.changeControlFactory = changeControlFactory;
     this.queryFactory = queryFactory;
     this.views = views;
@@ -74,7 +78,7 @@ public class ChangesCollection implements
 
     ChangeControl control;
     try {
-      control = changeControlFactory.validateFor(changes.get(0));
+      control = changeControlFactory.validateFor(changes.get(0), user.get());
     } catch (NoSuchChangeException e) {
       throw new ResourceNotFoundException(id);
     }

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

@@ -31,6 +31,7 @@ import com.google.gerrit.server.config.FactoryModule;
 public class Module extends RestApiModule {
   @Override
   protected void configure() {
+    bind(ChangesCollection.class);
     bind(Revisions.class);
     bind(Reviewers.class);
     bind(Drafts.class);

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

@@ -94,6 +94,15 @@ public class ChangeControl {
       return controlFor(change, user);
     }
 
+    public ChangeControl validateFor(Change change, CurrentUser user)
+        throws NoSuchChangeException, OrmException {
+      ChangeControl c = controlFor(change, user);
+      if (!c.isVisible(db.get())) {
+        throw new NoSuchChangeException(c.getChange().getId());
+      }
+      return c;
+    }
+
     public ChangeControl validateFor(Change.Id id, CurrentUser user)
         throws NoSuchChangeException, OrmException {
       ChangeControl c = controlFor(id, user);

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

@@ -24,7 +24,6 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.ChangeJson;
 import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
-import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.query.QueryParseException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -82,16 +81,12 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
   }
 
   @Inject
-  QueryChanges(ChangeJson json,
+  QueryChanges(ChangeJson json, QueryProcessor qp, Provider<CurrentUser> user) {
-      QueryProcessor qp,
-      ChangeControl.Factory cf,
-      Provider<CurrentUser> user) {
     this.json = json;
     this.imp = qp;
     this.user = user;
 
     options = EnumSet.noneOf(ListChangesOption.class);
-    json.setChangeControlFactory(cf);
   }
 
   public void addQuery(String query) {

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

@@ -98,7 +98,8 @@ public class QueryProcessor {
   private final ChangeQueryRewriter queryRewriter;
   private final Provider<ReviewDb> db;
   private final GitRepositoryManager repoManager;
-  private final ChangeControl.Factory changeControlFactory;
+  private final ChangeControl.GenericFactory changeControlFactory;
+  private final CurrentUser user;
   private final int maxLimit;
 
   private OutputFormat outputFormat = OutputFormat.TEXT;
@@ -123,13 +124,14 @@ public class QueryProcessor {
       ChangeQueryBuilder.Factory queryBuilder, CurrentUser currentUser,
       ChangeQueryRewriter queryRewriter, Provider<ReviewDb> db,
       GitRepositoryManager repoManager,
-      ChangeControl.Factory changeControlFactory) {
+      ChangeControl.GenericFactory changeControlFactory) {
     this.eventFactory = eventFactory;
     this.queryBuilder = queryBuilder.create(currentUser);
     this.queryRewriter = queryRewriter;
     this.db = db;
     this.repoManager = repoManager;
     this.changeControlFactory = changeControlFactory;
+    this.user = currentUser;
     this.maxLimit = currentUser.getCapabilities()
       .getRange(GlobalCapability.QUERY_LIMIT)
       .getMax();
@@ -298,8 +300,11 @@ public class QueryProcessor {
         List<ChangeData> results = queryChanges(queryString);
         ChangeAttribute c = null;
         for (ChangeData d : results) {
-          LabelTypes labelTypes = changeControlFactory.controlFor(d.getChange())
+          ChangeControl cc = d.changeControl();
-              .getLabelTypes();
+          if (cc == null || cc.getCurrentUser() != user) {
+            cc = changeControlFactory.controlFor(d.change(db), user);
+          }
+          LabelTypes labelTypes = cc.getLabelTypes();
           c = eventFactory.asChangeAttribute(d.getChange());
           eventFactory.extend(c, d.getChange());
           eventFactory.addTrackingIds(c, d.trackingIds(db));
@@ -307,7 +312,7 @@ public class QueryProcessor {
           if (includeSubmitRecords) {
             PatchSet.Id psId = d.getChange().currentPatchSetId();
             PatchSet patchSet = db.get().patchSets().get(psId);
-            List<SubmitRecord> submitResult = d.changeControl().canSubmit( //
+            List<SubmitRecord> submitResult = cc.canSubmit( //
                 db.get(), patchSet, null, false, true, true);
             eventFactory.addSubmitRecords(c, submitResult);
           }