Limit expansion of accounts for default fields
Terms in search queries that aren't qualified by a field are replaced
with an expansive OR predicate that applies the term to a number of
hard-coded "default" fields. Two of these default fields, owner and
reviewer, are designed to match against account IDs.
To construct an owner or reviewer predicate, we immediately resolve the
term to a set of matching account IDs, wrapping each with the specific
predicate for the field, collected by a single OR predicate. For
example, if the query [hi] resolves to three accounts, then the default
field predicate might expand to something like:
(OR
(OR (owner acct1) (owner acct2) (owner acct3))
(OR (reviewer acct1) (reviewer acct2) (reviewer acct3))
(file "hi")
(project "hi")
...
)
On a site with many users, a short term can resolve to hundreds of
accounts. Adding two very large predicates in such cases can lead to
very innocuous queries like [hi] resulting in a "too many terms" error.
The error is due to protection of the search backend from denial of
service by superficially examining the "cost" of the query, which is
proportional to the size of the predicate tree. As a site gathers more
users, this error is increasingly likely to occur (as observed in issue
6118).
To resolve this, we simply omit owner and reviewer predicates from
default field expansion when they are overly large. This commit sets the
maximum number of accounts per default field to 10 as a hard-coded
constant. Explicit uses of owner and reviewer fields are unaffected by
this limit. It only applies during default field expansion.
Bug: Issue 5856
Change-Id: Ibf6960976122e8170954ec20d3df9b50f597c5f7
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 099a3d1..f360778 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -118,6 +118,8 @@
private static final Pattern DEF_CHANGE =
Pattern.compile("^(?:[1-9][0-9]*|(?:[^~]+~[^~]+~)?[iI][0-9a-f]{4,}.*)$");
+ private static final int MAX_ACCOUNTS_PER_DEFAULT_FIELD = 10;
+
// NOTE: As new search operations are added, please keep the
// SearchSuggestOracle up to date.
@@ -939,6 +941,15 @@
return Predicate.or(p);
}
+ private Predicate<ChangeData> ownerDefaultField(String who)
+ throws QueryParseException, OrmException {
+ Set<Account.Id> accounts = parseAccount(who);
+ if (accounts.size() > MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
+ return Predicate.any();
+ }
+ return owner(accounts);
+ }
+
@Operator
public Predicate<ChangeData> assignee(String who) throws QueryParseException, OrmException {
return assignee(parseAccount(who));
@@ -968,17 +979,26 @@
@Operator
public Predicate<ChangeData> reviewer(String who) throws QueryParseException, OrmException {
+ return reviewer(who, false);
+ }
+
+ private Predicate<ChangeData> reviewerDefaultField(String who) throws QueryParseException, OrmException {
+ return reviewer(who, true);
+ }
+
+ private Predicate<ChangeData> reviewer(String who, boolean forDefaultField)
+ throws QueryParseException, OrmException {
if (args.getSchema().hasField(ChangeField.WIP)) {
return Predicate.and(
Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)),
- reviewerByState(who, ReviewerStateInternal.REVIEWER));
+ reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField));
}
- return reviewerByState(who, ReviewerStateInternal.REVIEWER);
+ return reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField);
}
@Operator
public Predicate<ChangeData> cc(String who) throws QueryParseException, OrmException {
- return reviewerByState(who, ReviewerStateInternal.CC);
+ return reviewerByState(who, ReviewerStateInternal.CC, false);
}
@Operator
@@ -1137,12 +1157,12 @@
// Adapt the capacity of this list when adding more default predicates.
List<Predicate<ChangeData>> predicates = Lists.newArrayListWithCapacity(11);
try {
- predicates.add(owner(query));
+ predicates.add(ownerDefaultField(query));
} catch (OrmException | QueryParseException e) {
// Skip.
}
try {
- predicates.add(reviewer(query));
+ predicates.add(reviewerDefaultField(query));
} catch (OrmException | QueryParseException e) {
// Skip.
}
@@ -1208,8 +1228,8 @@
return args.getIdentifiedUser().getAccountId();
}
- public Predicate<ChangeData> reviewerByState(String who, ReviewerStateInternal state)
- throws QueryParseException, OrmException {
+ public Predicate<ChangeData> reviewerByState(String who, ReviewerStateInternal state,
+ boolean forDefaultField) throws QueryParseException, OrmException {
Predicate<ChangeData> reviewerByEmailPredicate = null;
if (args.index.getSchema().hasField(ChangeField.REVIEWER_BY_EMAIL)) {
Address address = Address.tryParse(who);
@@ -1220,12 +1240,13 @@
Predicate<ChangeData> reviewerPredicate = null;
try {
- reviewerPredicate =
- Predicate.or(
- parseAccount(who)
- .stream()
- .map(id -> ReviewerPredicate.forState(args, id, state))
- .collect(toList()));
+ Set<Account.Id> accounts = parseAccount(who);
+ if (!forDefaultField || accounts.size() <= MAX_ACCOUNTS_PER_DEFAULT_FIELD) {
+ reviewerPredicate = Predicate.or(
+ accounts.stream()
+ .map(id -> ReviewerPredicate.forState(args, id, state))
+ .collect(toList()));
+ }
} catch (QueryParseException e) {
// Propagate this exception only if we can't use 'who' to query by email
if (reviewerByEmailPredicate == null) {
