opendev
/
gerrit
Code Issues Proposed changes

Move AccountLimits usage out of QueryProcessor 

We want to move QueryProcessor to the index package, which means it
cannot depend on this piece of core Gerrit. Move the lazy computation
into an IntSupplier that can be created at subclass construction time,
and add a method to AccountLimits to limit repetition. Clarify the
documentation around the different kinds of limits.

Change-Id: Iac182571944d0c648f1bc1d2669566170c81f2d4
This commit is contained in:
Dave Borowitz 2017-08-09 12:32:35 -04:00
parent d61094126c
commit 0b6fb95218
12 changed files with 50 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -201,7 +201,7 @@ public class ReviewersUtil {
QueryResult<AccountState> result =
queryProvider
.get()
.setLimit(suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER)
.setUserProvidedLimit(suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER)
.query(
AccountPredicates.andActive(
accountQueryBuilder.defaultQuery(suggestReviewers.getQuery())));

10
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountLimits.java
View File

@ -21,6 +21,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.QueueProvider;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.QueryProcessor;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.ArrayList;
@ -89,6 +90,15 @@ public class AccountLimits {
return QueueProvider.QueueType.INTERACTIVE;
}
/**
* Get the limit on a {@link QueryProcessor} for a given user.
*
* @return limit according to {@link GlobalCapability#QUERY_LIMIT}.
*/
public int getQueryLimit() {
return getRange(GlobalCapability.QUERY_LIMIT).getMax();
}
/** @return true if the user has a permission rule specifying the range. */
public boolean hasExplicitRange(String permission) {
return GlobalCapability.hasRange(permission) && !getRules(permission).isEmpty();

4
gerrit-server/src/main/java/com/google/gerrit/server/account/QueryAccounts.java
View File

@ -71,7 +71,7 @@ public class QueryAccounts implements RestReadView<TopLevelResource> {
usage = "maximum number of users to return"
)
public void setLimit(int n) {
queryProcessor.setLimit(n);
queryProcessor.setUserProvidedLimit(n);
if (n < 0) {
suggestLimit = 10;
@ -177,7 +177,7 @@ public class QueryAccounts implements RestReadView<TopLevelResource> {
Predicate<AccountState> queryPred;
if (suggest) {
queryPred = queryBuilder.defaultQuery(query);
queryProcessor.setLimit(suggestLimit);
queryProcessor.setUserProvidedLimit(suggestLimit);
} else {
queryPred = queryBuilder.parse(query);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/group/QueryGroups.java
View File

@ -119,7 +119,7 @@ public class QueryGroups implements RestReadView<TopLevelResource> {
}
if (limit != 0) {
queryProcessor.setLimit(limit);
queryProcessor.setUserProvidedLimit(limit);
}
try {

2
gerrit-server/src/main/java/com/google/gerrit/server/query/InternalQuery.java
View File

@ -53,7 +53,7 @@ public class InternalQuery<T> {
}
public InternalQuery<T> setLimit(int n) {
queryProcessor.setLimit(n);
queryProcessor.setUserProvidedLimit(n);
return this;
}

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

@ -23,7 +23,6 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.index.Index;
import com.google.gerrit.index.IndexCollection;
import com.google.gerrit.index.IndexConfig;
@ -39,19 +38,17 @@ import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Field;
import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountLimits;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.IntSupplier;
import java.util.stream.IntStream;
/**
@ -78,15 +75,13 @@ public abstract class QueryProcessor<T> {
}
}
protected final Provider<CurrentUser> userProvider;
private final AccountLimits.Factory limitsFactory;
private final Metrics metrics;
private final SchemaDefinitions<T> schemaDef;
private final IndexConfig indexConfig;
private final IndexCollection<?, T, ? extends Index<?, T>> indexes;
private final IndexRewriter<T> rewriter;
private final String limitField;
private final IntSupplier permittedLimit;
// This class is not generally thread-safe, but programmer error may result in it being shared
// across threads. At least ensure the bit for checking if it's been used is threadsafe.
@ -95,26 +90,24 @@ public abstract class QueryProcessor<T> {
protected int start;
private boolean enforceVisibility = true;
private int limitFromCaller;
private int userProvidedLimit;
private Set<String> requestedFields;
protected QueryProcessor(
Provider<CurrentUser> userProvider,
AccountLimits.Factory limitsFactory,
Metrics metrics,
SchemaDefinitions<T> schemaDef,
IndexConfig indexConfig,
IndexCollection<?, T, ? extends Index<?, T>> indexes,
IndexRewriter<T> rewriter,
String limitField) {
this.userProvider = userProvider;
this.limitsFactory = limitsFactory;
String limitField,
IntSupplier permittedLimit) {
this.metrics = metrics;
this.schemaDef = schemaDef;
this.indexConfig = indexConfig;
this.indexes = indexes;
this.rewriter = rewriter;
this.limitField = limitField;
this.permittedLimit = permittedLimit;
this.used = new AtomicBoolean(false);
}
@ -142,8 +135,18 @@ public abstract class QueryProcessor<T> {
return this;
}
public QueryProcessor<T> setLimit(int n) {
limitFromCaller = n;
/**
* Set an end-user-provided limit on the number of results returned.
*
* <p>Since this limit is provided by an end user, it may exceed the limit that they are
* authorized to use. This is allowed; the processor will take multiple possible limits into
* account and choose the one that makes the most sense.
*
* @param n limit; zero or negative means no limit.
* @return this.
*/
public QueryProcessor<T> setUserProvidedLimit(int n) {
userProvidedLimit = n;
return this;
}
@ -309,17 +312,11 @@ public abstract class QueryProcessor<T> {
* @return true if querying should be disabled.
*/
public boolean isDisabled() {
return getPermittedLimit() <= 0;
return enforceVisibility && getPermittedLimit() <= 0;
}
private int getPermittedLimit() {
if (enforceVisibility) {
return limitsFactory
.create(userProvider.get())
.getRange(GlobalCapability.QUERY_LIMIT)
.getMax();
}
return Integer.MAX_VALUE;
return enforceVisibility ? permittedLimit.getAsInt() : Integer.MAX_VALUE;
}
private int getBackendSupportedLimit() {
@ -330,8 +327,8 @@ public abstract class QueryProcessor<T> {
List<Integer> possibleLimits = new ArrayList<>(4);
possibleLimits.add(getBackendSupportedLimit());
possibleLimits.add(getPermittedLimit());
if (limitFromCaller > 0) {
possibleLimits.add(limitFromCaller);
if (userProvidedLimit > 0) {
possibleLimits.add(userProvidedLimit);
}
if (limitField != null) {
Integer limitFromPredicate = LimitPredicate.getLimit(limitField, p);

5
gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
View File

@ -58,14 +58,13 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> {
AccountIndexRewriter rewriter,
AccountControl.Factory accountControlFactory) {
super(
userProvider,
limitsFactory,
metrics,
AccountSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
rewriter,
FIELD_LIMIT);
FIELD_LIMIT,
() -> limitsFactory.create(userProvider.get()).getQueryLimit());
this.accountControlFactory = accountControlFactory;
}

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

@ -59,6 +59,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
}
private final Provider<ReviewDb> db;
private final Provider<CurrentUser> userProvider;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeNotes.Factory notesFactory;
private final DynamicMap<ChangeAttributeFactory> attributeFactories;
@ -85,15 +86,15 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData>
DynamicMap<ChangeAttributeFactory> attributeFactories,
PermissionBackend permissionBackend) {
super(
userProvider,
limitsFactory,
metrics,
ChangeSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
rewriter,
FIELD_LIMIT);
FIELD_LIMIT,
() -> limitsFactory.create(userProvider.get()).getQueryLimit());
this.db = db;
this.userProvider = userProvider;
this.changeControlFactory = changeControlFactory;
this.notesFactory = notesFactory;
this.attributeFactories = attributeFactories;

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
View File

@ -125,7 +125,7 @@ public class OutputStreamQuery {
}
void setLimit(int n) {
queryProcessor.setLimit(n);
queryProcessor.setUserProvidedLimit(n);
}
public void setStart(int n) {

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

@ -60,7 +60,7 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
usage = "Maximum number of results to return"
)
public void setLimit(int limit) {
imp.setLimit(limit);
imp.setUserProvidedLimit(limit);
}
@Option(name = "-o", usage = "Output options per change")

7
gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
View File

@ -39,6 +39,7 @@ import com.google.inject.Provider;
* holding on to a single instance.
*/
public class GroupQueryProcessor extends QueryProcessor<AccountGroup> {
private final Provider<CurrentUser> userProvider;
private final GroupControl.GenericFactory groupControlFactory;
static {
@ -58,14 +59,14 @@ public class GroupQueryProcessor extends QueryProcessor<AccountGroup> {
GroupIndexRewriter rewriter,
GroupControl.GenericFactory groupControlFactory) {
super(
userProvider,
limitsFactory,
metrics,
GroupSchemaDefinitions.INSTANCE,
indexConfig,
indexes,
rewriter,
FIELD_LIMIT);
FIELD_LIMIT,
() -> limitsFactory.create(userProvider.get()).getQueryLimit());
this.userProvider = userProvider;
this.groupControlFactory = groupControlFactory;
}

2
plugins/singleusergroup

@ -1 +1 @@
Subproject commit 89e92bc36f04184a709cf16fc9773eebba8d9c0f
Subproject commit 50d04dd09a747868cd2f2707d1aac3d9b2a2d630