opendev/gerrit
Code Issues Proposed changes

ChangeQueryBuilder: Allow ownerin predicate to be evaluated by the index

Browse Source 
The previous implementation of ownerin predicate was not efficient as
it first found all the matched changes and then checked the membership
of the owner for each one of them. This change modifies this predicate
such that for each account in the group, it finds all the matched
changes that belong to that account, which can be evaluated by the
index.

Note that for external groups (e.g., ldap groups), it falls back to the
previous implementation of this ownerin predicate, as the account lists
of external groups are not available in the index.

Bug: Issue 8916
Change-Id: I57b0a33c824f103d8b0e5e533af30e7cb23aa41d
This commit is contained in:
Borui Tao
2018-05-04 15:11:48 -04:00
committed by David Pursehouse
parent 6a5d6ed03e
commit 65a1ae50eb
1 changed files with 36 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

54
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.errors.NotSignedInException;
@@ -742,23 +743,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
}
// expand a group predicate into multiple user predicates
if (group != null) {
Set<Account.Id> allMembers =
args.listMembers
.get()
.setRecursive(true)
.apply(group)
.stream()
.map(a -> new Account.Id(a._accountId))
.collect(toSet());
int maxTerms = args.indexConfig.maxTerms();
if (allMembers.size() > maxTerms) {
// limit the number of query terms otherwise Gerrit will barf
accounts = ImmutableSet.copyOf(Iterables.limit(allMembers, maxTerms));
} else {
accounts = allMembers;
}
accounts = getMembers(group);
}
// If the vote piece looks like Code-Review=NEED with a valid non-numeric
@@ -923,12 +909,24 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
@Operator
public Predicate<ChangeData> ownerin(String group) throws QueryParseException {
public Predicate<ChangeData> ownerin(String group) throws QueryParseException, OrmException {
GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group);
if (g == null) {
throw error("Group " + group + " not found");
}
return new OwnerinPredicate(args.userFactory, g.getUUID());
AccountGroup.UUID groupId = g.getUUID();
GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
if (!(groupDescription instanceof GroupDescription.Internal)) {
return new OwnerinPredicate(args.userFactory, groupId);
}
Set<Account.Id> accounts = getMembers(groupId);
List<OwnerPredicate> p = Lists.newArrayListWithCapacity(accounts.size());
for (Account.Id id : accounts) {
p.add(new OwnerPredicate(id));
}
return Predicate.or(p);
}
@Operator
@@ -1134,6 +1132,26 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return Predicate.or(predicates);
}
private Set<Account.Id> getMembers(AccountGroup.UUID g) throws OrmException {
Set<Account.Id> accounts;
Set<Account.Id> allMembers =
args.listMembers
.get()
.setRecursive(true)
.apply(g)
.stream()
.map(a -> new Account.Id(a._accountId))
.collect(toSet());
int maxTerms = args.indexConfig.maxTerms();
if (allMembers.size() > maxTerms) {
// limit the number of query terms otherwise Gerrit will barf
accounts = ImmutableSet.copyOf(Iterables.limit(allMembers, maxTerms));
} else {
accounts = allMembers;
}
return accounts;
}
private Set<Account.Id> parseAccount(String who) throws QueryParseException, OrmException {
if ("self".equals(who)) {
return Collections.singleton(self());