opendev/gerrit
Code Issues Proposed changes

ChangeQueryBuilder: Always get user lazily from Arguments

Browse Source 
There were a few special predicates that depended on constructing new
ChangeQueryBuilders with the same arguments but a different user. This
created the error-prone situation of the code in these predicates
having two different users available (from the args and passed in
explicitly), where only one of them was the right user to be using.
Teach these predicates to get the user they need directly from the
Arguments, and change callers to construct new Arguments containing
the appropriate user.

Also add some new methods to Arguments to extract a CurrentUser and/or
IdentifiedUser, throwing QueryParseException with a useful message if
the user is not logged in. This allows queries that do not depend on
the current user to succeed as in contexts without a user, e.g.
searching for submitted changes in a background thread.

Change-Id: Ic6479455b0978306ccd3239b0cc35c7253f04854
This commit is contained in:
Dave Borowitz 2015-01-05 10:16:35 -08:00
parent b9378a4009
commit 0669078106
11 changed files with 162 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/AccountServiceImpl.java
View File

@ -51,7 +51,7 @@ class AccountServiceImpl extends BaseServiceImplementation implements
private final AccountCache accountCache;
private final ProjectControl.Factory projectControlFactory;
private final AgreementInfoFactory.Factory agreementInfoFactory;
private final ChangeQueryBuilder.Factory queryBuilder;
private final ChangeQueryBuilder queryBuilder;
@Inject
AccountServiceImpl(final Provider<ReviewDb> schema,
@ -59,7 +59,7 @@ class AccountServiceImpl extends BaseServiceImplementation implements
final AccountCache accountCache,
final ProjectControl.Factory projectControlFactory,
final AgreementInfoFactory.Factory agreementInfoFactory,
final ChangeQueryBuilder.Factory queryBuilder) {
final ChangeQueryBuilder queryBuilder) {
super(schema, identifiedUser);
this.currentUser = identifiedUser;
this.accountCache = accountCache;
@ -156,7 +156,7 @@ class AccountServiceImpl extends BaseServiceImplementation implements
if (filter != null) {
try {
queryBuilder.create(currentUser.get()).parse(filter);
queryBuilder.parse(filter);
} catch (QueryParseException badFilter) {
throw new InvalidQueryException(badFilter.getMessage(), filter);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@ -123,7 +123,6 @@ import com.google.gerrit.server.project.ProjectNode;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SectionSortCache;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ConflictsCacheImpl;
import com.google.gerrit.server.ssh.SshAddressesModule;
import com.google.gerrit.server.tools.ToolsCatalog;
@ -190,7 +189,6 @@ public class GerritGlobalModule extends FactoryModule {
factory(AddReviewerSender.Factory.class);
factory(CapabilityControl.Factory.class);
factory(ChangeData.Factory.class);
factory(ChangeQueryBuilder.Factory.class);
factory(CreateChangeSender.Factory.class);
factory(GroupDetailFactory.Factory.class);
factory(GroupInfoCacheFactory.Factory.class);

4
gerrit-server/src/main/java/com/google/gerrit/server/mail/EmailArguments.java
View File

@ -62,7 +62,7 @@ public class EmailArguments {
final AllProjectsName allProjectsName;
final List<String> sshAddresses;
final ChangeQueryBuilder.Factory queryBuilder;
final ChangeQueryBuilder queryBuilder;
final Provider<ReviewDb> db;
final ChangeData.Factory changeDataFactory;
final RuntimeInstance velocityRuntime;
@ -83,7 +83,7 @@ public class EmailArguments {
@AnonymousCowardName String anonymousCowardName,
@CanonicalWebUrl @Nullable Provider<String> urlProvider,
AllProjectsName allProjectsName,
ChangeQueryBuilder.Factory queryBuilder,
ChangeQueryBuilder queryBuilder,
Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
RuntimeInstance velocityRuntime,

4
gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java
View File

@ -200,9 +200,9 @@ public class ProjectWatch {
Predicate<ChangeData> p = null;
if (user == null) {
qb = args.queryBuilder.create(args.anonymousUser);
qb = args.queryBuilder.asUser(args.anonymousUser);
} else {
qb = args.queryBuilder.create(user);
qb = args.queryBuilder.asUser(user);
p = qb.is_visible();
}

3
gerrit-server/src/main/java/com/google/gerrit/server/query/QueryBuilder.java
View File

@ -160,11 +160,14 @@ public abstract class QueryBuilder<T> {
return null;
}
protected final Definition<T, ? extends QueryBuilder<T>> builderDef;
@SuppressWarnings("rawtypes")
private final Map<String, OperatorFactory> opFactories;
@SuppressWarnings({"unchecked", "rawtypes"})
protected QueryBuilder(Definition<T, ? extends QueryBuilder<T>> def) {
builderDef = def;
opFactories = (Map) def.opFactories;
}

3
gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
View File

@ -28,8 +28,7 @@ public class BasicChangeRewrites extends QueryRewriter<ChangeData> {
new InvalidProvider<ReviewDb>(),
new InvalidProvider<ChangeQueryRewriter>(),
null, null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null, null),
null);
null, null, null, null, null, null, null, null));
private static final QueryRewriter.Definition<ChangeData, BasicChangeRewrites> mydef =
new QueryRewriter.Definition<>(BasicChangeRewrites.class, BUILDER);

193
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@ -19,6 +19,7 @@ import com.google.common.base.Optional;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NotSignedInException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch;
@ -51,7 +52,8 @@ import com.google.gerrit.server.query.QueryParseException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.ProvisionException;
import com.google.inject.util.Providers;
import org.eclipse.jgit.lib.AbbreviatedObjectId;
import org.eclipse.jgit.lib.Config;
@ -125,7 +127,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
final Provider<ReviewDb> db;
final Provider<ChangeQueryRewriter> rewriter;
final IdentifiedUser.GenericFactory userFactory;
final Provider<CurrentUser> self;
final CapabilityControl.Factory capabilityControlFactory;
final ChangeControl.GenericFactory changeControlGenericFactory;
final ChangeData.Factory changeDataFactory;
@ -144,9 +145,11 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
final TrackingFooters trackingFooters;
final boolean allowsDrafts;
private final Provider<CurrentUser> self;
@Inject
@VisibleForTesting
public Arguments(Provider<ReviewDb> dbProvider,
public Arguments(Provider<ReviewDb> db,
Provider<ChangeQueryRewriter> rewriter,
IdentifiedUser.GenericFactory userFactory,
Provider<CurrentUser> self,
@ -167,53 +170,121 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
ConflictsCache conflictsCache,
TrackingFooters trackingFooters,
@GerritServerConfig Config cfg) {
this.db = dbProvider;
this.rewriter = rewriter;
this.userFactory = userFactory;
this.self = self;
this.capabilityControlFactory = capabilityControlFactory;
this.changeControlGenericFactory = changeControlGenericFactory;
this.changeDataFactory = changeDataFactory;
this.fillArgs = fillArgs;
this.plcUtil = plcUtil;
this.accountResolver = accountResolver;
this.groupBackend = groupBackend;
this.allProjectsName = allProjectsName;
this.patchListCache = patchListCache;
this.repoManager = repoManager;
this.projectCache = projectCache;
this.listChildProjects = listChildProjects;
this.indexes = indexes;
this.submitStrategyFactory = submitStrategyFactory;
this.conflictsCache = conflictsCache;
this.trackingFooters = trackingFooters;
this.allowsDrafts = cfg == null
? true
: cfg.getBoolean("change", "allowDrafts", true);
this(db, rewriter, userFactory, self, capabilityControlFactory,
changeControlGenericFactory, changeDataFactory, fillArgs, plcUtil,
accountResolver, groupBackend, allProjectsName, patchListCache,
repoManager, projectCache, listChildProjects, indexes,
submitStrategyFactory, conflictsCache, trackingFooters,
cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true));
}
private Arguments(
Provider<ReviewDb> db,
Provider<ChangeQueryRewriter> rewriter,
IdentifiedUser.GenericFactory userFactory,
Provider<CurrentUser> self,
CapabilityControl.Factory capabilityControlFactory,
ChangeControl.GenericFactory changeControlGenericFactory,
ChangeData.Factory changeDataFactory,
FieldDef.FillArgs fillArgs,
PatchLineCommentsUtil plcUtil,
AccountResolver accountResolver,
GroupBackend groupBackend,
AllProjectsName allProjectsName,
PatchListCache patchListCache,
GitRepositoryManager repoManager,
ProjectCache projectCache,
Provider<ListChildProjects> listChildProjects,
IndexCollection indexes,
SubmitStrategyFactory submitStrategyFactory,
ConflictsCache conflictsCache,
TrackingFooters trackingFooters,
boolean allowsDrafts) {
this.db = db;
this.rewriter = rewriter;
this.userFactory = userFactory;
this.self = self;
this.capabilityControlFactory = capabilityControlFactory;
this.changeControlGenericFactory = changeControlGenericFactory;
this.changeDataFactory = changeDataFactory;
this.fillArgs = fillArgs;
this.plcUtil = plcUtil;
this.accountResolver = accountResolver;
this.groupBackend = groupBackend;
this.allProjectsName = allProjectsName;
this.patchListCache = patchListCache;
this.repoManager = repoManager;
this.projectCache = projectCache;
this.listChildProjects = listChildProjects;
this.indexes = indexes;
this.submitStrategyFactory = submitStrategyFactory;
this.conflictsCache = conflictsCache;
this.trackingFooters = trackingFooters;
this.allowsDrafts = allowsDrafts;
}
Arguments asUser(CurrentUser otherUser) {
return new Arguments(db, rewriter, userFactory,
Providers.of(otherUser),
capabilityControlFactory, changeControlGenericFactory,
changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
allProjectsName, patchListCache, repoManager, projectCache,
listChildProjects, indexes, submitStrategyFactory, conflictsCache,
trackingFooters, allowsDrafts);
}
Arguments asUser(Account.Id otherId) {
try {
CurrentUser u = self.get();
if (u.isIdentifiedUser()
&& otherId.equals(((IdentifiedUser) u).getAccountId())) {
return this;
}
} catch (ProvisionException e) {
// Doesn't match current user, continue.
}
return asUser(userFactory.create(db, otherId));
}
IdentifiedUser getIdentifiedUser() throws QueryParseException {
try {
CurrentUser u = getCurrentUser();
if (u.isIdentifiedUser()) {
return (IdentifiedUser) u;
}
throw new QueryParseException(NotSignedInException.MESSAGE);
} catch (ProvisionException e) {
throw new QueryParseException(NotSignedInException.MESSAGE, e);
}
}
CurrentUser getCurrentUser() throws QueryParseException {
try {
return self.get();
} catch (ProvisionException e) {
throw new QueryParseException(NotSignedInException.MESSAGE, e);
}
}
}
public interface Factory {
ChangeQueryBuilder create(CurrentUser user);
}
private final Arguments args;
private final CurrentUser currentUser;
@Inject
public ChangeQueryBuilder(Arguments args, @Assisted CurrentUser currentUser) {
ChangeQueryBuilder(Arguments args) {
super(mydef);
this.args = args;
this.currentUser = currentUser;
}
@VisibleForTesting
protected ChangeQueryBuilder(
QueryBuilder.Definition<ChangeData, ? extends ChangeQueryBuilder> def,
Arguments args, CurrentUser currentUser) {
Definition<ChangeData, ? extends QueryBuilder<ChangeData>> def,
Arguments args) {
super(def);
this.args = args;
this.currentUser = currentUser;
}
public ChangeQueryBuilder asUser(CurrentUser user) {
return new ChangeQueryBuilder(builderDef, args.asUser(user));
}
@Operator
@ -279,9 +350,9 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
@Operator
public Predicate<ChangeData> has(String value) {
public Predicate<ChangeData> has(String value) throws QueryParseException {
if ("star".equalsIgnoreCase(value)) {
return new IsStarredByPredicate(args, currentUser);
return new IsStarredByPredicate(args);
}
if ("draft".equalsIgnoreCase(value)) {
@ -292,13 +363,13 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
@Operator
public Predicate<ChangeData> is(String value) {
public Predicate<ChangeData> is(String value) throws QueryParseException {
if ("starred".equalsIgnoreCase(value)) {
return new IsStarredByPredicate(args, currentUser);
return new IsStarredByPredicate(args);
}
if ("watched".equalsIgnoreCase(value)) {
return new IsWatchedByPredicate(args, currentUser, false);
return new IsWatchedByPredicate(args, false);
}
if ("visible".equalsIgnoreCase(value)) {
@ -483,13 +554,12 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
public Predicate<ChangeData> starredby(String who)
throws QueryParseException, OrmException {
if ("self".equals(who)) {
return new IsStarredByPredicate(args, currentUser);
return new IsStarredByPredicate(args);
}
Set<Account.Id> m = parseAccount(who);
List<IsStarredByPredicate> p = Lists.newArrayListWithCapacity(m.size());
for (Account.Id id : m) {
p.add(new IsStarredByPredicate(args,
args.userFactory.create(args.db, id)));
p.add(new IsStarredByPredicate(args.asUser(id)));
}
return Predicate.or(p);
}
@ -499,14 +569,26 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
throws QueryParseException, OrmException {
Set<Account.Id> m = parseAccount(who);
List<IsWatchedByPredicate> p = Lists.newArrayListWithCapacity(m.size());
for (Account.Id id : m) {
if (currentUser.isIdentifiedUser()
&& id.equals(((IdentifiedUser) currentUser).getAccountId())) {
p.add(new IsWatchedByPredicate(args, currentUser, false));
Account.Id callerId;
try {
CurrentUser caller = args.self.get();
if (caller.isIdentifiedUser()) {
callerId = ((IdentifiedUser) caller).getAccountId();
} else {
p.add(new IsWatchedByPredicate(args,
args.userFactory.create(args.db, id), true));
callerId = null;
}
} catch (ProvisionException e) {
callerId = null;
}
for (Account.Id id : m) {
// Each child IsWatchedByPredicate includes a visibility filter for the
// corresponding user, to ensure that predicate subtree only returns
// changes visible to that user. The exception is if one of the users is
// the caller of this method, in which case visibility is already being
// checked at the top level.
p.add(new IsWatchedByPredicate(args.asUser(id), !id.equals(callerId)));
}
return Predicate.or(p);
}
@ -557,8 +639,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
user);
}
public Predicate<ChangeData> is_visible() {
return visibleto(currentUser);
public Predicate<ChangeData> is_visible() throws QueryParseException {
return visibleto(args.getCurrentUser());
}
@Operator
@ -743,11 +825,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return value;
}
private Account.Id self() {
if (currentUser.isIdentifiedUser()) {
return ((IdentifiedUser) currentUser).getAccountId();
}
throw new IllegalArgumentException();
private Account.Id self() throws QueryParseException {
return args.getIdentifiedUser().getAccountId();
}
private static Schema<ChangeData> schema(@Nullable IndexCollection indexes) {

7
gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByPredicate.java
View File

@ -20,6 +20,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.query.OrPredicate;
import com.google.gerrit.server.query.Predicate;
import com.google.gerrit.server.query.QueryParseException;
import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
@ -49,7 +50,11 @@ class IsStarredByPredicate extends OrPredicate<ChangeData> implements
private final Arguments args;
private final CurrentUser user;
IsStarredByPredicate(Arguments args, CurrentUser user) {
IsStarredByPredicate(Arguments args) throws QueryParseException {
this(args, args.getIdentifiedUser());
}
private IsStarredByPredicate(Arguments args, IdentifiedUser user) {
super(predicates(args, user.getStarredChanges()));
this.args = args;
this.user = user;

13
gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsWatchedByPredicate.java
View File

@ -37,18 +37,17 @@ class IsWatchedByPredicate extends AndPredicate<ChangeData> {
private final CurrentUser user;
IsWatchedByPredicate(ChangeQueryBuilder.Arguments args,
CurrentUser user,
boolean checkIsVisible) {
super(filters(args, user, checkIsVisible));
this.user = user;
boolean checkIsVisible) throws QueryParseException {
super(filters(args, checkIsVisible));
this.user = args.getCurrentUser();
}
private static List<Predicate<ChangeData>> filters(
ChangeQueryBuilder.Arguments args,
CurrentUser user,
boolean checkIsVisible) {
boolean checkIsVisible) throws QueryParseException {
CurrentUser user = args.getCurrentUser();
List<Predicate<ChangeData>> r = Lists.newArrayList();
ChangeQueryBuilder builder = new ChangeQueryBuilder(args, user);
ChangeQueryBuilder builder = new ChangeQueryBuilder(args);
for (AccountProjectWatch w : user.getNotificationFilters()) {
Predicate<ChangeData> f = null;
if (w.getFilter() != null) {

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

@ -39,10 +39,10 @@ public class QueryProcessor {
private int start;
@Inject
QueryProcessor(ChangeQueryBuilder.Factory queryBuilder,
QueryProcessor(ChangeQueryBuilder queryBuilder,
CurrentUser currentUser,
ChangeQueryRewriter queryRewriter) {
this.queryBuilder = queryBuilder.create(currentUser);
this.queryBuilder = queryBuilder;
this.queryRewriter = queryRewriter;
this.permittedLimit = currentUser.getCapabilities()
.getRange(GlobalCapability.QUERY_LIMIT)

3
gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
View File

@ -27,8 +27,7 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
FakeQueryBuilder.class),
new ChangeQueryBuilder.Arguments(null, null, null, null, null, null,
null, null, null, null, null, null, null, null, null, null, indexes,
null, null, null, null),
null);
null, null, null, null));
}
@Operator