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
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index f64e424..5a3ab3b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -31,6 +31,9 @@
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.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.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 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.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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
try {
- if (changeControlUserFactory != null) {
- ctrl = changeControlUserFactory.controlFor(cd.change(db));
- } else {
- ctrl = changeControlGenericFactory.controlFor(cd.change(db),
- userProvider.get());
+ Change change = cd.change(db);
+ if (change == null) {
+ return null;
}
- } catch (NoSuchChangeException e) {
+ ctrl = projectControls.get(change.getProject()).controlFor(change);
+ } catch (ExecutionException e) {
return null;
}
lastControl = ctrl;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
index 45328b1..ecb83b48 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
@@ -23,6 +23,7 @@
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 @@
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 @@
ChangeControl control;
try {
- control = changeControlFactory.validateFor(changes.get(0));
+ control = changeControlFactory.validateFor(changes.get(0), user.get());
} catch (NoSuchChangeException e) {
throw new ResourceNotFoundException(id);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
index 7fb09b5..9ed2bee 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
@@ -31,6 +31,7 @@
public class Module extends RestApiModule {
@Override
protected void configure() {
+ bind(ChangesCollection.class);
bind(Revisions.class);
bind(Reviewers.class);
bind(Drafts.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index d87db2f..698a0ac 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -94,6 +94,15 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
index 44c71a9..4b6a5a6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryChanges.java
@@ -24,7 +24,6 @@
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 @@
}
@Inject
- QueryChanges(ChangeJson json,
- QueryProcessor qp,
- ChangeControl.Factory cf,
- Provider<CurrentUser> user) {
+ QueryChanges(ChangeJson json, QueryProcessor qp, 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) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
index 5624f45..2f3fe54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
@@ -98,7 +98,8 @@
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 @@
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 @@
List<ChangeData> results = queryChanges(queryString);
ChangeAttribute c = null;
for (ChangeData d : results) {
- LabelTypes labelTypes = changeControlFactory.controlFor(d.getChange())
- .getLabelTypes();
+ ChangeControl cc = d.changeControl();
+ 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 @@
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);
}
