Fix NPE when default field expansions are omitted
A recent change[1] fixes "too many terms" errors when default fields are
used on sites with many accounts. Its approach was to replace overly
large OR predicates with the ANY predicate. It turns out that AND and OR
predicates do not support ANY as a child.
To fix this, we'll omit adding any predicate at all to the tree when the
predicate is elided. This involves checking for and accommodating ANY in
a few different places so we can avoid adding a predicate to the list
produced in the defaultField method.
[1] Ibf6960976122e8170954ec20d3df9b50f597c5f7
Change-Id: If727aeabbc0de3d50febbf329ecfd32971f6c216
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 611a1a4..8ea100d 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,7 +118,7 @@
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;
+ static final int MAX_ACCOUNTS_PER_DEFAULT_FIELD = 10;
// NOTE: As new search operations are added, please keep the
// SearchSuggestOracle up to date.
@@ -993,12 +993,15 @@
private Predicate<ChangeData> reviewer(String who, boolean forDefaultField)
throws QueryParseException, OrmException {
+ Predicate byState = reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField);
+ if (byState == Predicate.any()) {
+ return Predicate.any();
+ }
if (args.getSchema().hasField(ChangeField.WIP)) {
return Predicate.and(
- Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)),
- reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField));
+ Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)), byState);
}
- return reviewerByState(who, ReviewerStateInternal.REVIEWER, forDefaultField);
+ return byState;
}
@Operator
@@ -1162,12 +1165,18 @@
// Adapt the capacity of this list when adding more default predicates.
List<Predicate<ChangeData>> predicates = Lists.newArrayListWithCapacity(11);
try {
- predicates.add(ownerDefaultField(query));
+ Predicate p = ownerDefaultField(query);
+ if (p != Predicate.any()) {
+ predicates.add(p);
+ }
} catch (OrmException | QueryParseException e) {
// Skip.
}
try {
- predicates.add(reviewerDefaultField(query));
+ Predicate p = reviewerDefaultField(query);
+ if (p != Predicate.any()) {
+ predicates.add(p);
+ }
} catch (OrmException | QueryParseException e) {
// Skip.
}
@@ -1266,7 +1275,10 @@
return Predicate.or(reviewerPredicate, reviewerByEmailPredicate);
} else if (reviewerPredicate != null) {
return reviewerPredicate;
+ } else if (reviewerByEmailPredicate != null) {
+ return reviewerByEmailPredicate;
+ } else {
+ return Predicate.any();
}
- return reviewerByEmailPredicate;
}
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 0b01362..ac421f0 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -67,6 +67,7 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.StarredChangesUtil;
+import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.change.ChangeInserter;
@@ -88,6 +89,8 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.gerrit.testutil.ConfigSuite;
@@ -138,6 +141,7 @@
return cfg;
}
+ @Inject protected AccountCache accountCache;
@Inject protected AccountManager accountManager;
@Inject protected AllUsersName allUsersName;
@Inject protected BatchUpdate.Factory updateFactory;
@@ -151,6 +155,7 @@
@Inject protected InMemoryRepositoryManager repoManager;
@Inject protected InternalChangeQuery internalChangeQuery;
@Inject protected ChangeNotes.Factory notesFactory;
+ @Inject protected OneOffRequestContext oneOffRequestContext;
@Inject protected PatchSetInserter.Factory patchSetFactory;
@Inject protected PatchSetUtil psUtil;
@Inject protected ChangeControl.GenericFactory changeControlFactory;
@@ -2001,6 +2006,14 @@
assertQuery("starredby:me", change2, change1);
}
+ @Test
+ public void defaultFieldWithManyUsers() throws Exception {
+ for (int i = 0; i < ChangeQueryBuilder.MAX_ACCOUNTS_PER_DEFAULT_FIELD * 2; i++) {
+ createAccount("user" + i, "User " + i, "user" + i + "@example.com", true);
+ }
+ assertQuery("us");
+ }
+
protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
return newChange(repo, null, null, null, null);
}
@@ -2204,4 +2217,21 @@
Patch.COMMIT_MSG, ImmutableList.<ReviewInput.CommentInput>of(comment));
gApi.changes().id(changeId).current().review(input);
}
+
+ private Account.Id createAccount(String username, String fullName, String email, boolean active)
+ throws Exception {
+ try (ManualRequestContext ctx = oneOffRequestContext.open()) {
+ Account.Id id = accountManager.authenticate(AuthRequest.forUser(username)).getAccountId();
+ if (email != null) {
+ accountManager.link(id, AuthRequest.forEmail(email));
+ }
+ Account a = db.accounts().get(id);
+ a.setFullName(fullName);
+ a.setPreferredEmail(email);
+ a.setActive(active);
+ db.accounts().update(ImmutableList.of(a));
+ accountCache.evict(id);
+ return id;
+ }
+ }
}
