Lazily build ChangeControls from ChangeData

This simplifies logic in a few places where this is used.

Change-Id: I1bd2c931ba5d03e05d50050fa964b4e3da14c23c

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

@@ -86,6 +86,7 @@ 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;
@@ -349,22 +350,20 @@ public class ChangeJson {
   }
 
   private ChangeControl control(ChangeData cd) throws OrmException {
-    ChangeControl ctrl = cd.changeControl();
+    if (lastControl != null
-    if (ctrl != null && ctrl.getCurrentUser() == userProvider.get()) {
-      return ctrl;
-    } else if (lastControl != null
         && cd.getId().equals(lastControl.getChange().getId())) {
       return lastControl;
     }
-
+    ChangeControl ctrl;
     try {
-      Change change = cd.change();
+      if (cd.hasChangeControl()) {
-      if (change == null) {
+        ctrl = cd.changeControl().forUser(userProvider.get());
-        return null;
+      } else {
+        ctrl = projectControls.get(cd.change().getProject())
+            .controlFor(cd.change());
       }
-      ctrl = projectControls.get(change.getProject()).controlFor(change);
+    } catch (NoSuchChangeException | ExecutionException e) {
-    } catch (ExecutionException e) {
+      throw new OrmException(e);
-      return null;
     }
     lastControl = ctrl;
     return ctrl;

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

@@ -217,6 +217,9 @@ public class ChangeControl {
   }
 
   public ChangeControl forUser(final CurrentUser who) {
+    if (getCurrentUser().equals(who)) {
+      return this;
+    }
     return new ChangeControl(approvalsUtil, changeDataFactory,
         getRefControl().forUser(who), notes);
   }

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

@@ -33,6 +33,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.NotesMigration;
@@ -150,11 +151,13 @@ public class ChangeData {
    * @return instance for testing.
    */
   static ChangeData createForTest(Change.Id id) {
-    return new ChangeData(null, null, null, null, null, null, id);
+    return new ChangeData(null, null, null, null, null, null, null, null, id);
   }
 
   private final ReviewDb db;
   private final GitRepositoryManager repoManager;
+  private final ChangeControl.GenericFactory changeControlFactory;
+  private final IdentifiedUser.GenericFactory userFactory;
   private final ChangeNotes.Factory notesFactory;
   private final ApprovalsUtil approvalsUtil;
   private final PatchListCache patchListCache;
@@ -180,6 +183,8 @@ public class ChangeData {
   @AssistedInject
   private ChangeData(
       GitRepositoryManager repoManager,
+      ChangeControl.GenericFactory changeControlFactory,
+      IdentifiedUser.GenericFactory userFactory,
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       PatchListCache patchListCache,
@@ -188,6 +193,8 @@ public class ChangeData {
       @Assisted Change.Id id) {
     this.db = db;
     this.repoManager = repoManager;
+    this.changeControlFactory = changeControlFactory;
+    this.userFactory = userFactory;
     this.notesFactory = notesFactory;
     this.approvalsUtil = approvalsUtil;
     this.patchListCache = patchListCache;
@@ -198,6 +205,8 @@ public class ChangeData {
   @AssistedInject
   private ChangeData(
       GitRepositoryManager repoManager,
+      ChangeControl.GenericFactory changeControlFactory,
+      IdentifiedUser.GenericFactory userFactory,
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       PatchListCache patchListCache,
@@ -206,6 +215,8 @@ public class ChangeData {
       @Assisted Change c) {
     this.db = db;
     this.repoManager = repoManager;
+    this.changeControlFactory = changeControlFactory;
+    this.userFactory = userFactory;
     this.notesFactory = notesFactory;
     this.approvalsUtil = approvalsUtil;
     this.patchListCache = patchListCache;
@@ -217,6 +228,8 @@ public class ChangeData {
   @AssistedInject
   private ChangeData(
       GitRepositoryManager repoManager,
+      ChangeControl.GenericFactory changeControlFactory,
+      IdentifiedUser.GenericFactory userFactory,
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       PatchListCache patchListCache,
@@ -225,6 +238,8 @@ public class ChangeData {
       @Assisted ChangeControl c) {
     this.db = db;
     this.repoManager = repoManager;
+    this.changeControlFactory = changeControlFactory;
+    this.userFactory = userFactory;
     this.notesFactory = notesFactory;
     this.approvalsUtil = approvalsUtil;
     this.patchListCache = patchListCache;
@@ -324,7 +339,17 @@ public class ChangeData {
     return visibleTo == user;
   }
 
-  public ChangeControl changeControl() {
+  public boolean hasChangeControl() {
+    return changeControl != null;
+  }
+
+  public ChangeControl changeControl() throws NoSuchChangeException,
+      OrmException {
+    if (changeControl == null) {
+      Change c = change();
+      changeControl =
+          changeControlFactory.controlFor(c, userFactory.create(c.getOwner()));
+    }
     return changeControl;
   }
 

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

@@ -97,7 +97,6 @@ public class QueryProcessor {
   private final ChangeQueryBuilder queryBuilder;
   private final ChangeQueryRewriter queryRewriter;
   private final Provider<ReviewDb> db;
-  private final ChangeControl.GenericFactory changeControlFactory;
   private final TrackingFooters trackingFooters;
   private final CurrentUser user;
   private final int maxLimit;
@@ -124,14 +123,12 @@ public class QueryProcessor {
   QueryProcessor(EventFactory eventFactory,
       ChangeQueryBuilder.Factory queryBuilder, CurrentUser currentUser,
       ChangeQueryRewriter queryRewriter, Provider<ReviewDb> db,
-      TrackingFooters trackingFooters,
+      TrackingFooters trackingFooters) {
-      ChangeControl.GenericFactory changeControlFactory) {
     this.eventFactory = eventFactory;
     this.queryBuilder = queryBuilder.create(currentUser);
     this.queryRewriter = queryRewriter;
     this.db = db;
     this.trackingFooters = trackingFooters;
-    this.changeControlFactory = changeControlFactory;
     this.user = currentUser;
     this.maxLimit = currentUser.getCapabilities()
       .getRange(GlobalCapability.QUERY_LIMIT)
@@ -302,10 +299,7 @@ public class QueryProcessor {
         List<ChangeData> results = queryChanges(queryString);
         ChangeAttribute c = null;
         for (ChangeData d : results) {
-          ChangeControl cc = d.changeControl();
+          ChangeControl cc = d.changeControl().forUser(user);
-          if (cc == null || cc.getCurrentUser() != user) {
-            cc = changeControlFactory.controlFor(d.change(), user);
-          }
 
           LabelTypes labelTypes = cc.getLabelTypes();
           c = eventFactory.asChangeAttribute(d.change());

gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java

@@ -29,20 +29,29 @@ import com.google.gerrit.reviewdb.client.AccountProjectWatch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.rules.PrologEnvironment;
 import com.google.gerrit.rules.RulesCache;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.CapabilityControl;
+import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.account.GroupMembership;
 import com.google.gerrit.server.account.ListGroupMembership;
 import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.config.AnonymousCowardName;
+import com.google.gerrit.server.config.AnonymousCowardNameProvider;
+import com.google.gerrit.server.config.CanonicalWebUrl;
+import com.google.gerrit.server.config.CanonicalWebUrlProvider;
 import com.google.gerrit.server.config.FactoryModule;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.testutil.FakeAccountCache;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
 import com.google.inject.Guice;
 import com.google.inject.Injector;
@@ -193,6 +202,7 @@ public class Util {
       protected void configure() {
         bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(
             new Config());
+        bind(ReviewDb.class).toProvider(Providers.<ReviewDb> of(null));
         bind(GitRepositoryManager.class).toInstance(repoManager);
         bind(PatchListCache.class)
             .toProvider(Providers.<PatchListCache> of(null));
@@ -201,6 +211,12 @@ public class Util {
         factory(ChangeControl.AssistedFactory.class);
         factory(ChangeData.Factory.class);
         bind(ProjectCache.class).toInstance(projectCache);
+        bind(AccountCache.class).toInstance(new FakeAccountCache());
+        bind(GroupBackend.class).to(SystemGroupBackend.class);
+        bind(String.class).annotatedWith(CanonicalWebUrl.class)
+            .toProvider(CanonicalWebUrlProvider.class);
+        bind(String.class).annotatedWith(AnonymousCowardName.class)
+            .toProvider(AnonymousCowardNameProvider.class);
       }
     });