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
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java
index 7ba75c5..0c1f871 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewersUtil.java
@@ -201,7 +201,7 @@
QueryResult<AccountState> result =
queryProvider
.get()
- .setLimit(suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER)
+ .setUserProvidedLimit(suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER)
.query(
AccountPredicates.andActive(
accountQueryBuilder.defaultQuery(suggestReviewers.getQuery())));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountLimits.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountLimits.java
index e8e6d9e..6a3b2f5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountLimits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountLimits.java
@@ -21,6 +21,7 @@
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 @@
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();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/QueryAccounts.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/QueryAccounts.java
index a5c6965..cbfb7f5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/QueryAccounts.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/QueryAccounts.java
@@ -71,7 +71,7 @@
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 @@
Predicate<AccountState> queryPred;
if (suggest) {
queryPred = queryBuilder.defaultQuery(query);
- queryProcessor.setLimit(suggestLimit);
+ queryProcessor.setUserProvidedLimit(suggestLimit);
} else {
queryPred = queryBuilder.parse(query);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/QueryGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/QueryGroups.java
index 30bd31b..32ea4e4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/group/QueryGroups.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/QueryGroups.java
@@ -119,7 +119,7 @@
}
if (limit != 0) {
- queryProcessor.setLimit(limit);
+ queryProcessor.setUserProvidedLimit(limit);
}
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/InternalQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/InternalQuery.java
index 30f7328..5409cb4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/InternalQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/InternalQuery.java
@@ -53,7 +53,7 @@
}
public InternalQuery<T> setLimit(int n) {
- queryProcessor.setLimit(n);
+ queryProcessor.setUserProvidedLimit(n);
return this;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java
index af814a6..068673e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java
@@ -23,7 +23,6 @@
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.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 @@
}
}
- 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 @@
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 @@
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 @@
* @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 @@
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);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
index 67f62a4..dc17f486 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java
@@ -58,14 +58,13 @@
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;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
index 3759a67..f1bdaaa 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java
@@ -59,6 +59,7 @@
}
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 @@
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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
index 91d7dbc..879d34b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -125,7 +125,7 @@
}
void setLimit(int n) {
- queryProcessor.setLimit(n);
+ queryProcessor.setUserProvidedLimit(n);
}
public void setStart(int n) {
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 a66eda8..c1fc0ec 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
@@ -60,7 +60,7 @@
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")
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
index 4abb7aa..a1b966fa 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java
@@ -39,6 +39,7 @@
* 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 @@
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;
}
diff --git a/plugins/singleusergroup b/plugins/singleusergroup
index 89e92bc..50d04dd 160000
--- a/plugins/singleusergroup
+++ b/plugins/singleusergroup
@@ -1 +1 @@
-Subproject commit 89e92bc36f04184a709cf16fc9773eebba8d9c0f
+Subproject commit 50d04dd09a747868cd2f2707d1aac3d9b2a2d630
