Merge "RestApiServlet: Introduce constant for HTTP 429 code"
diff --git a/java/com/google/gerrit/server/index/account/AccountIndexRewriter.java b/java/com/google/gerrit/server/index/account/AccountIndexRewriter.java
index 35b967c..a1fa338 100644
--- a/java/com/google/gerrit/server/index/account/AccountIndexRewriter.java
+++ b/java/com/google/gerrit/server/index/account/AccountIndexRewriter.java
@@ -16,22 +16,26 @@
import static java.util.Objects.requireNonNull;
+import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.IndexRewriter;
import com.google.gerrit.index.QueryOptions;
+import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.account.AccountState;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import org.eclipse.jgit.util.MutableInteger;
@Singleton
public class AccountIndexRewriter implements IndexRewriter<AccountState> {
-
private final AccountIndexCollection indexes;
+ private final IndexConfig config;
@Inject
- AccountIndexRewriter(AccountIndexCollection indexes) {
+ AccountIndexRewriter(AccountIndexCollection indexes, IndexConfig config) {
this.indexes = indexes;
+ this.config = config;
}
@Override
@@ -39,6 +43,32 @@
throws QueryParseException {
AccountIndex index = indexes.getSearchIndex();
requireNonNull(index, "no active search index configured for accounts");
+ validateMaxTermsInQuery(in);
return new IndexedAccountQuery(index, in, opts);
}
+
+ /**
+ * Validates whether a query has too many terms.
+ *
+ * @param predicate the predicate for which the leaf predicates should be counted
+ * @throws QueryParseException thrown if the query has too many terms
+ */
+ public void validateMaxTermsInQuery(Predicate<AccountState> predicate)
+ throws QueryParseException {
+ MutableInteger leafTerms = new MutableInteger();
+ validateMaxTermsInQuery(predicate, leafTerms);
+ }
+
+ private void validateMaxTermsInQuery(Predicate<AccountState> predicate, MutableInteger leafTerms)
+ throws QueryParseException {
+ if (!(predicate instanceof IndexPredicate)) {
+ if (++leafTerms.value > config.maxTerms()) {
+ throw new QueryParseException("too many terms in query");
+ }
+ }
+
+ for (Predicate<AccountState> childPredicate : predicate.getChildren()) {
+ validateMaxTermsInQuery(childPredicate, leafTerms);
+ }
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
index 7fd5ecf..866ca1f 100644
--- a/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
+++ b/java/com/google/gerrit/server/restapi/change/ReviewersUtil.java
@@ -50,6 +50,7 @@
import com.google.gerrit.server.change.ReviewerAdder;
import com.google.gerrit.server.index.account.AccountField;
import com.google.gerrit.server.index.account.AccountIndexCollection;
+import com.google.gerrit.server.index.account.AccountIndexRewriter;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchProjectException;
@@ -120,6 +121,7 @@
private final AccountLoader.Factory accountLoaderFactory;
private final AccountQueryBuilder accountQueryBuilder;
+ private final AccountIndexRewriter accountIndexRewriter;
private final GroupBackend groupBackend;
private final GroupMembers groupMembers;
private final ReviewerRecommender reviewerRecommender;
@@ -133,6 +135,7 @@
ReviewersUtil(
AccountLoader.Factory accountLoaderFactory,
AccountQueryBuilder accountQueryBuilder,
+ AccountIndexRewriter accountIndexRewriter,
GroupBackend groupBackend,
GroupMembers groupMembers,
ReviewerRecommender reviewerRecommender,
@@ -143,6 +146,7 @@
Provider<CurrentUser> self) {
this.accountLoaderFactory = accountLoaderFactory;
this.accountQueryBuilder = accountQueryBuilder;
+ this.accountIndexRewriter = accountIndexRewriter;
this.groupBackend = groupBackend;
this.groupMembers = groupMembers;
this.reviewerRecommender = reviewerRecommender;
@@ -226,35 +230,34 @@
private List<Account.Id> suggestAccounts(SuggestReviewers suggestReviewers) {
try (Timer0.Context ctx = metrics.queryAccountsLatency.start()) {
- try {
- // For performance reasons we don't use AccountQueryProvider as it would always load the
- // complete account from the cache (or worse, from NoteDb) even though we only need the ID
- // which we can directly get from the returned results.
- Predicate<AccountState> pred =
- Predicate.and(
- AccountPredicates.isActive(),
- accountQueryBuilder.defaultQuery(suggestReviewers.getQuery()));
- logger.atFine().log("accounts index query: %s", pred);
- ResultSet<FieldBundle> result =
- accountIndexes
- .getSearchIndex()
- .getSource(
- pred,
- QueryOptions.create(
- indexConfig,
- 0,
- suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER,
- ImmutableSet.of(AccountField.ID.getName())))
- .readRaw();
- List<Account.Id> matches =
- result.toList().stream()
- .map(f -> Account.id(f.getValue(AccountField.ID).intValue()))
- .collect(toList());
- logger.atFine().log("Matches: %s", matches);
- return matches;
- } catch (QueryParseException e) {
- return ImmutableList.of();
- }
+ // For performance reasons we don't use AccountQueryProvider as it would always load the
+ // complete account from the cache (or worse, from NoteDb) even though we only need the ID
+ // which we can directly get from the returned results.
+ Predicate<AccountState> pred =
+ Predicate.and(
+ AccountPredicates.isActive(),
+ accountQueryBuilder.defaultQuery(suggestReviewers.getQuery()));
+ logger.atFine().log("accounts index query: %s", pred);
+ accountIndexRewriter.validateMaxTermsInQuery(pred);
+ ResultSet<FieldBundle> result =
+ accountIndexes
+ .getSearchIndex()
+ .getSource(
+ pred,
+ QueryOptions.create(
+ indexConfig,
+ 0,
+ suggestReviewers.getLimit() * CANDIDATE_LIST_MULTIPLIER,
+ ImmutableSet.of(AccountField.ID.getName())))
+ .readRaw();
+ List<Account.Id> matches =
+ result.toList().stream()
+ .map(f -> Account.id(f.getValue(AccountField.ID).intValue()))
+ .collect(toList());
+ logger.atFine().log("Matches: %s", matches);
+ return matches;
+ } catch (QueryParseException e) {
+ return ImmutableList.of();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index 7ac655f..9c9e746 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -136,6 +136,27 @@
}
@Test
+ @GerritConfig(name = "index.maxTerms", value = "10")
+ public void suggestReviewersTooManyQueryTerms() throws Exception {
+ String changeId = createChange().getChangeId();
+
+ // Do a query which doesn't exceed index.maxTerms succeeds (add only 9 terms, since on
+ // 'inactive:1' term is implicitly added) and assert that a result is returned
+ StringBuilder query = new StringBuilder();
+ for (int i = 1; i <= 9; i++) {
+ query.append(name("u")).append(" ");
+ }
+ assertThat(suggestReviewers(changeId, query.toString())).isNotEmpty();
+
+ // Do a query which exceed index.maxTerms succeeds (10 terms plus 'inactive:1' term which is
+ // implicitly added). If index.maxTerms is exceeded a QueryParseException is thrown ("too many
+ // terms in query") but it is caught in ReviewersUtil which then returns an empty result to the
+ // client. Hence assert that no result is returned.
+ query.append(name("u"));
+ assertThat(suggestReviewers(changeId, query.toString())).isEmpty();
+ }
+
+ @Test
public void suggestReviewersWithExcludeGroups() throws Exception {
String changeId = createChange().getChangeId();
