Fix label operator to work with external groups
Before this change, EqualsLabelPredicate's match() was not called
after change index search. EqualsLabelPredicate's match() contains
the required validations for external groups which was never run. Due
to which label operator does not work with external groups.
Introduce a new ChangeIndexPostFilterPredicate that is able to do
additional filtering in the match method after searching the change
index. EqualsLabelPredicate uses ChangeIndexPostFilterPredicate to do
additional filtering post index search.
Also add tests for label operator with external groups.
Bug: Issue 16106
Release-Notes: Fix label operator to work with external groups
Change-Id: Ibfba11daef9386fb0f1e5533bffd01d73edb5384
diff --git a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
index 601ac59..12d8c93 100644
--- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
+++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
@@ -118,6 +118,13 @@
@Override
public Collection<GroupReference> suggest(String name, ProjectState project) {
+ AccountGroup.UUID uuid = AccountGroup.uuid(name);
+ if (handles(uuid)) {
+ GroupDescription.Basic g = get(uuid);
+ if (g != null) {
+ return ImmutableList.of(GroupReference.forGroup(g));
+ }
+ }
return ImmutableList.of();
}
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index 57a2091..be5f803 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -34,6 +34,7 @@
import com.google.gerrit.index.query.ResultSet;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeDataSource;
+import com.google.gerrit.server.query.change.ChangeIndexPostFilterPredicate;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
@@ -112,13 +113,30 @@
};
}
+ public boolean postIndexMatch(Predicate<ChangeData> pred, ChangeData cd) {
+ if (pred instanceof ChangeIndexPostFilterPredicate) {
+ checkState(
+ pred.isMatchable(),
+ "match invoked, but child predicate %s doesn't implement %s",
+ pred,
+ Matchable.class.getName());
+ return pred.asMatchable().match(cd);
+ }
+ for (int i = 0; i < pred.getChildCount(); i++) {
+ if (!postIndexMatch(pred.getChild(i), cd)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
@Override
public boolean match(ChangeData cd) {
- if (source != null && fromSource.get(cd) == source) {
+ Predicate<ChangeData> pred = getChild(0);
+ if (source != null && fromSource.get(cd) == source && postIndexMatch(pred, cd)) {
return true;
}
- Predicate<ChangeData> pred = getChild(0);
checkState(
pred.isMatchable(),
"match invoked, but child predicate %s doesn't implement %s",
diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java
new file mode 100644
index 0000000..d86d366
--- /dev/null
+++ b/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java
@@ -0,0 +1,31 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.query.change;
+
+import com.google.gerrit.index.FieldDef;
+
+/**
+ * Predicate that is mapped to a field in the change index, with additional filtering done in the
+ * {@code match} method.
+ */
+public abstract class ChangeIndexPostFilterPredicate extends ChangeIndexPredicate {
+ protected ChangeIndexPostFilterPredicate(FieldDef<ChangeData, ?> def, String value) {
+ super(def, value);
+ }
+
+ protected ChangeIndexPostFilterPredicate(FieldDef<ChangeData, ?> def, String name, String value) {
+ super(def, name, value);
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
index 30d5e2f..7f910f1 100644
--- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
@@ -30,7 +30,7 @@
import com.google.gerrit.server.project.ProjectState;
import java.util.Optional;
-public class EqualsLabelPredicate extends ChangeIndexPredicate {
+public class EqualsLabelPredicate extends ChangeIndexPostFilterPredicate {
protected final ProjectCache projectCache;
protected final PermissionBackend permissionBackend;
protected final IdentifiedUser.GenericFactory userFactory;
diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD
index 93c996e8..4e1866f 100644
--- a/java/com/google/gerrit/testing/BUILD
+++ b/java/com/google/gerrit/testing/BUILD
@@ -33,6 +33,7 @@
"//java/com/google/gerrit/server/audit",
"//java/com/google/gerrit/server/cache/h2",
"//java/com/google/gerrit/server/cache/mem",
+ "//java/com/google/gerrit/server/group/testing",
"//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/schema",
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 1c322b2..b957e13 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -25,6 +25,7 @@
import com.google.gerrit.auth.AuthModule;
import com.google.gerrit.extensions.client.AuthType;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.systemstatus.ServerInformation;
import com.google.gerrit.gpg.GpgModule;
import com.google.gerrit.httpd.auth.restapi.OAuthRestModule;
@@ -38,6 +39,7 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.GerritPersonIdentProvider;
import com.google.gerrit.server.PluginUser;
+import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.api.GerritApiModule;
import com.google.gerrit.server.api.PluginApiModule;
import com.google.gerrit.server.audit.AuditModule;
@@ -71,6 +73,7 @@
import com.google.gerrit.server.git.PerThreadRequestScope;
import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.group.testing.TestGroupBackend;
import com.google.gerrit.server.index.account.AccountSchemaDefinitions;
import com.google.gerrit.server.index.account.AllAccountsIndexer;
import com.google.gerrit.server.index.change.AllChangesIndexer;
@@ -251,6 +254,8 @@
install(new FileInfoJsonModule(cfg));
bind(ProjectOperations.class).to(ProjectOperationsImpl.class);
+ bind(TestGroupBackend.class).in(SINGLETON);
+ DynamicSet.bind(binder(), GroupBackend.class).to(TestGroupBackend.class);
}
/** Copy of SchemaModule with a slightly different server ID provider. */
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 99ccd31..1a18442 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -101,6 +101,7 @@
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.AuthRequest;
+import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.account.VersionedAccountQueries;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.change.ChangeInserter;
@@ -109,6 +110,7 @@
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.group.testing.TestGroupBackend;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexer;
@@ -186,6 +188,7 @@
@Inject protected SchemaCreator schemaCreator;
@Inject protected Sequences seq;
@Inject protected ThreadLocalRequestContext requestContext;
+ @Inject protected TestGroupBackend testGroupBackend;
@Inject protected ProjectCache projectCache;
@Inject protected MetaDataUpdate.Server metaDataUpdateFactory;
@Inject protected IdentifiedUser.GenericFactory identifiedUserFactory;
@@ -1175,6 +1178,44 @@
}
@Test
+ public void byLabelExternalGroup() throws Exception {
+ Account.Id user1 = createAccount("user1");
+ Account.Id user2 = createAccount("user2");
+ TestRepository<InMemoryRepositoryManager.Repo> repo = createProject("repo");
+
+ // create group and add users
+ AccountGroup.UUID external_group1 = AccountGroup.uuid("testbackend:group1");
+ AccountGroup.UUID external_group2 = AccountGroup.uuid("testbackend:group2");
+ testGroupBackend.create(external_group1);
+ testGroupBackend.create(external_group2);
+ testGroupBackend.setMembershipsOf(
+ user1, new ListGroupMembership(ImmutableList.of(external_group1)));
+ testGroupBackend.setMembershipsOf(
+ user2, new ListGroupMembership(ImmutableList.of(external_group2)));
+
+ Change change1 = insert(repo, newChange(repo), user1);
+ Change change2 = insert(repo, newChange(repo), user1);
+
+ // post a review with user1 and other_user
+ requestContext.setContext(newRequestContext(user1));
+ gApi.changes()
+ .id(change1.getId().get())
+ .current()
+ .review(new ReviewInput().label("Code-Review", 1));
+ requestContext.setContext(newRequestContext(userId));
+ gApi.changes()
+ .id(change2.getId().get())
+ .current()
+ .review(new ReviewInput().label("Code-Review", 1));
+
+ assertQuery("label:Code-Review=+1," + external_group1.get(), change1);
+ assertQuery("label:Code-Review=+1,group=" + external_group1.get(), change1);
+ assertQuery("label:Code-Review=+1,user=user1", change1);
+ assertQuery("label:Code-Review=+1,user=user2");
+ assertQuery("label:Code-Review=+1,group=" + external_group2.get());
+ }
+
+ @Test
public void limit() throws Exception {
TestRepository<Repo> repo = createProject("repo");
Change last = null;
diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD
index 43b9690..84a65bb 100644
--- a/javatests/com/google/gerrit/server/query/change/BUILD
+++ b/javatests/com/google/gerrit/server/query/change/BUILD
@@ -26,6 +26,7 @@
"//java/com/google/gerrit/index:query_exception",
"//java/com/google/gerrit/lifecycle",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/group/testing",
"//java/com/google/gerrit/server/project/testing:project-test-util",
"//java/com/google/gerrit/server/schema",
"//java/com/google/gerrit/server/util/time",
