Fix query for changes using a label with a group operator.
The group operator is being ignored when searching for changes
with labels because the search index does not contain group
information. This change will convert a group query into
mutiple user queries.
For example the query:
'label:Code-Review=+1,group=heros'
Gets expanded to:
'label:Code-Review=+1,user=superman OR label:Code-Review=+1,user=batman'
The latter is the actual query.
Bug: issue 3018
Change-Id: Ief5408ee4774f9491fe713b0089e76aa4cae4403
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
index 1053d92..851bbcd 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/BasicChangeRewrites.java
@@ -29,7 +29,7 @@
new InvalidProvider<InternalChangeQuery>(),
new InvalidProvider<ChangeQueryRewriter>(),
null, null, null, null, null, null, null, null, null, null, null,
- null, null, null, null, null, null, null, null));
+ null, null, null, null, null, null, null, null, null, null, null));
private static final QueryRewriter.Definition<ChangeData, BasicChangeRewrites> mydef =
new QueryRewriter.Definition<>(BasicChangeRewrites.class, BUILDER);
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 250b1f295..eb71f88 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
@@ -18,12 +18,18 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.data.GroupDetail;
import com.google.gerrit.common.data.GroupReference;
+import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.common.errors.NotSignedInException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.AccountGroupById;
+import com.google.gerrit.reviewdb.client.AccountGroupMember;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -34,6 +40,8 @@
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupBackends;
+import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.account.GroupDetailFactory;
import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -43,6 +51,7 @@
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.FieldDef;
import com.google.gerrit.server.index.IndexCollection;
+import com.google.gerrit.server.index.IndexConfig;
import com.google.gerrit.server.index.Schema;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.project.ChangeControl;
@@ -131,6 +140,7 @@
final Provider<InternalChangeQuery> queryProvider;
final Provider<ChangeQueryRewriter> rewriter;
final IdentifiedUser.GenericFactory userFactory;
+ final GroupDetailFactory.Factory groupDetailFactory;
final CapabilityControl.Factory capabilityControlFactory;
final ChangeControl.GenericFactory changeControlGenericFactory;
final ChangeData.Factory changeDataFactory;
@@ -142,12 +152,14 @@
final PatchListCache patchListCache;
final GitRepositoryManager repoManager;
final ProjectCache projectCache;
+ final GroupCache groupCache;
final Provider<ListChildProjects> listChildProjects;
final IndexCollection indexes;
final SubmitStrategyFactory submitStrategyFactory;
final ConflictsCache conflictsCache;
final TrackingFooters trackingFooters;
final boolean allowsDrafts;
+ final IndexConfig indexConfig;
private final Provider<CurrentUser> self;
@@ -159,6 +171,7 @@
IdentifiedUser.GenericFactory userFactory,
Provider<CurrentUser> self,
CapabilityControl.Factory capabilityControlFactory,
+ GroupDetailFactory.Factory groupDetailFactory,
ChangeControl.GenericFactory changeControlGenericFactory,
ChangeData.Factory changeDataFactory,
FieldDef.FillArgs fillArgs,
@@ -169,18 +182,20 @@
PatchListCache patchListCache,
GitRepositoryManager repoManager,
ProjectCache projectCache,
+ GroupCache groupCache,
Provider<ListChildProjects> listChildProjects,
IndexCollection indexes,
SubmitStrategyFactory submitStrategyFactory,
ConflictsCache conflictsCache,
TrackingFooters trackingFooters,
+ IndexConfig indexConfig,
@GerritServerConfig Config cfg) {
this(db, queryProvider, rewriter, userFactory, self,
- capabilityControlFactory, changeControlGenericFactory,
+ capabilityControlFactory, groupDetailFactory, changeControlGenericFactory,
changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
allProjectsName, patchListCache, repoManager, projectCache,
- listChildProjects, indexes, submitStrategyFactory, conflictsCache,
- trackingFooters,
+ groupCache, listChildProjects, indexes, submitStrategyFactory,
+ conflictsCache, trackingFooters, indexConfig,
cfg == null ? true : cfg.getBoolean("change", "allowDrafts", true));
}
@@ -191,6 +206,7 @@
IdentifiedUser.GenericFactory userFactory,
Provider<CurrentUser> self,
CapabilityControl.Factory capabilityControlFactory,
+ GroupDetailFactory.Factory groupDetailFactory,
ChangeControl.GenericFactory changeControlGenericFactory,
ChangeData.Factory changeDataFactory,
FieldDef.FillArgs fillArgs,
@@ -201,11 +217,13 @@
PatchListCache patchListCache,
GitRepositoryManager repoManager,
ProjectCache projectCache,
+ GroupCache groupCache,
Provider<ListChildProjects> listChildProjects,
IndexCollection indexes,
SubmitStrategyFactory submitStrategyFactory,
ConflictsCache conflictsCache,
TrackingFooters trackingFooters,
+ IndexConfig indexConfig,
boolean allowsDrafts) {
this.db = db;
this.queryProvider = queryProvider;
@@ -213,6 +231,7 @@
this.userFactory = userFactory;
this.self = self;
this.capabilityControlFactory = capabilityControlFactory;
+ this.groupDetailFactory = groupDetailFactory;
this.changeControlGenericFactory = changeControlGenericFactory;
this.changeDataFactory = changeDataFactory;
this.fillArgs = fillArgs;
@@ -223,22 +242,24 @@
this.patchListCache = patchListCache;
this.repoManager = repoManager;
this.projectCache = projectCache;
+ this.groupCache = groupCache;
this.listChildProjects = listChildProjects;
this.indexes = indexes;
this.submitStrategyFactory = submitStrategyFactory;
this.conflictsCache = conflictsCache;
this.trackingFooters = trackingFooters;
this.allowsDrafts = allowsDrafts;
+ this.indexConfig = indexConfig;
}
Arguments asUser(CurrentUser otherUser) {
return new Arguments(db, queryProvider, rewriter, userFactory,
Providers.of(otherUser),
- capabilityControlFactory, changeControlGenericFactory,
+ capabilityControlFactory, groupDetailFactory, changeControlGenericFactory,
changeDataFactory, fillArgs, plcUtil, accountResolver, groupBackend,
allProjectsName, patchListCache, repoManager, projectCache,
- listChildProjects, indexes, submitStrategyFactory, conflictsCache,
- trackingFooters, allowsDrafts);
+ groupCache, listChildProjects, indexes, submitStrategyFactory, conflictsCache,
+ trackingFooters, indexConfig, allowsDrafts);
}
Arguments asUser(Account.Id otherId) {
@@ -547,6 +568,19 @@
}
}
+ // expand a group predicate into multiple user predicates
+ if (group != null) {
+ Set<Account.Id> allMembers = getMemberIds(
+ group, new HashSet<AccountGroup.UUID>());
+ int maxTerms = args.indexConfig.maxLimit();
+ 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 new LabelPredicate(args.projectCache,
args.changeControlGenericFactory, args.userFactory, args.db,
name, accounts, group);
@@ -808,6 +842,39 @@
return g;
}
+ private Set<Account.Id> getMemberIds(AccountGroup.UUID groupUUID,
+ Set<AccountGroup.UUID> seenGroups) throws OrmException {
+ seenGroups.add(groupUUID);
+
+ Set<Account.Id> members = new HashSet<>();
+ AccountGroup group = args.groupCache.get(groupUUID);
+ if (group != null) {
+ try {
+ GroupDetail groupDetail =
+ args.groupDetailFactory.create(group.getId()).call();
+ if (groupDetail.members != null) {
+ for (AccountGroupMember m : groupDetail.members) {
+ if (!members.contains(m.getAccountId())) {
+ members.add(m.getAccountId());
+ }
+ }
+ }
+ // Get members of subgroups
+ if (groupDetail.includes != null) {
+ for (AccountGroupById includedGroup : groupDetail.includes) {
+ if (!seenGroups.contains(includedGroup.getIncludeUUID())) {
+ members.addAll(
+ getMemberIds(includedGroup.getIncludeUUID(), seenGroups));
+ }
+ }
+ }
+ } catch (NoSuchGroupException e) {
+ // The included group is not visible
+ }
+ }
+ return members;
+ }
+
private List<Change> parseChange(String value) throws OrmException,
QueryParseException {
if (PAT_LEGACY_ID.matcher(value).matches()) {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
index c9a2056..04fed32 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/FakeQueryBuilder.java
@@ -25,9 +25,10 @@
super(
new FakeQueryBuilder.Definition<>(
FakeQueryBuilder.class),
- new ChangeQueryBuilder.Arguments(null, null, null, null, null, null,
- null, null, null, null, null, null, null, null, null, null, null,
- indexes, null, null, null, null));
+ new ChangeQueryBuilder.Arguments(null, null, null, null,
+ null, null, null, null, null, null, null, null, null,
+ null, null, null, null, null, null, indexes, null, null,
+ null, null, null));
}
@Operator
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 34588fa..eea8e58 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
@@ -40,6 +40,7 @@
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.lifecycle.LifecycleManager;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Patch;
@@ -49,6 +50,9 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AuthRequest;
+import com.google.gerrit.server.account.CreateGroupArgs;
+import com.google.gerrit.server.account.GroupCache;
+import com.google.gerrit.server.account.PerformCreateGroup;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.change.ChangesCollection;
@@ -109,6 +113,8 @@
@Inject protected Provider<QueryChanges> queryProvider;
@Inject protected SchemaCreator schemaCreator;
@Inject protected ThreadLocalRequestContext requestContext;
+ @Inject protected GroupCache groupCache;
+ @Inject protected PerformCreateGroup.Factory performCreateGroupFactory;
protected LifecycleManager lifecycle;
protected ReviewDb db;
@@ -536,6 +542,49 @@
assertResultEquals(change, queryOne("label:Code-Review=+1,group=Administrators"));
}
+ private void createGroup(String name, AccountGroup.Id owner, Account.Id member)
+ throws Exception {
+ CreateGroupArgs args = new CreateGroupArgs();
+ args.setGroupName(name);
+ args.ownerGroupId = owner;
+ args.initialMembers = ImmutableList.of(member);
+ performCreateGroupFactory.create(args).createGroup();
+ }
+
+ @Test
+ public void byLabelGroup() throws Exception {
+ Account.Id user1 = accountManager
+ .authenticate(AuthRequest.forUser("user1")).getAccountId();
+ Account.Id user2 = accountManager
+ .authenticate(AuthRequest.forUser("user2")).getAccountId();
+ TestRepository<InMemoryRepository> repo = createProject("repo");
+
+ // create group and add users
+ AccountGroup.Id adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators")).getId();
+ createGroup("group1", adminGroup, user1);
+ createGroup("group2", adminGroup, user2);
+
+ // create a change
+ ChangeInserter ins = newChange(repo, null, null, user1.get(), null);
+ Change change1 = ins.insert();
+
+ // post a review with user1
+ requestContext.setContext(newRequestContext(user1));
+ ReviewInput input = new ReviewInput();
+ input.labels = ImmutableMap.<String, Short> of("Code-Review", (short) 1);
+ postReview.apply(new RevisionResource(
+ changes.parse(change1.getId()), ins.getPatchSet()), input);
+
+ // verify that query with user1 will return results.
+ requestContext.setContext(newRequestContext(userId));
+ assertResultEquals(change1, queryOne("label:Code-Review=+1,group1"));
+ assertResultEquals(change1, queryOne("label:Code-Review=+1,group=group1"));
+ assertResultEquals(change1, queryOne("label:Code-Review=+1,user=user1"));
+ assertThat(query("label:Code-Review=+1,user=user2")).isEmpty();
+ assertThat(query("label:Code-Review=+1,group=group2")).isEmpty();
+ }
+
@Test
public void limit() throws Exception {
TestRepository<InMemoryRepository> repo = createProject("repo");
