Don't filter out service users when listing code owners for path in branch
Currently we have 2 REST endpoints to list code owners for a path that
behave the same:
* GetCodeOwnersForPathInBranch
* GetCodeOwnersForPathInChange
Going on GetCodeOwnersForPathInChange will be used to suggest code
owners for a path on a change, whereas GetCodeOwnersForPathInBranch
lists all code owners for path. The difference is that for
GetCodeOwnersForPathInChange we will filter out code owners that should
not show up in the suggestion (e.g. service users) whereas
GetCodeOwnersForPathInBranch includes all code owners.
The frontend part was already adapted to use
GetCodeOwnersForPathInChange for suggestion code owners (see change
Ie04e60913).
GetCodeOwnersForPathInBranch is not used by the frontend, but is
intended to be used by bots that want to query all code owners.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ifbec40349fa1bde36ea36822e6f4bc2a39fd295e
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 854ffd7..e3c7a1f 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -56,6 +56,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
+import java.util.function.Predicate;
import java.util.stream.Stream;
import org.kohsuke.args4j.Option;
@@ -77,6 +78,7 @@
private final Provider<CodeOwnerResolver> codeOwnerResolver;
private final ServiceUserClassifier serviceUserClassifier;
private final CodeOwnerJson.Factory codeOwnerJsonFactory;
+ private final boolean suggest;
private final EnumSet<ListAccountsOption> options;
private final Set<String> hexOptions;
@@ -115,7 +117,8 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
ServiceUserClassifier serviceUserClassifier,
- CodeOwnerJson.Factory codeOwnerJsonFactory) {
+ CodeOwnerJson.Factory codeOwnerJsonFactory,
+ boolean suggest) {
this.accountVisibility = accountVisibility;
this.accounts = accounts;
this.accountControlFactory = accountControlFactory;
@@ -125,6 +128,7 @@
this.codeOwnerResolver = codeOwnerResolver;
this.serviceUserClassifier = serviceUserClassifier;
this.codeOwnerJsonFactory = codeOwnerJsonFactory;
+ this.suggest = suggest;
this.options = EnumSet.noneOf(ListAccountsOption.class);
this.hexOptions = new HashSet<>();
}
@@ -229,27 +233,36 @@
*/
private ImmutableSet<CodeOwner> filterCodeOwners(
AbstractPathResource rsrc, ImmutableSet<CodeOwner> codeOwners) {
- return codeOwners.stream()
- .filter(
- codeOwner -> {
- if (isVisibleTo(rsrc, codeOwner)) {
- return true;
- }
- logger.atFine().log(
- "Filtering out %s because this code owner cannot see the branch %s",
- codeOwner, rsrc.getBranch().branch());
- return false;
- })
- .filter(
- codeOwner -> {
- if (!isServiceUser(codeOwner)) {
- return true;
- }
- logger.atFine().log(
- "Filtering out %s because this code owner is a service user", codeOwner);
- return false;
- })
- .collect(toImmutableSet());
+ Stream<CodeOwner> filteredCodeOwners =
+ codeOwners.stream().filter(filterOutNonVisibleCodeOwners(rsrc));
+
+ if (suggest) {
+ filteredCodeOwners = filteredCodeOwners.filter(filterOutServiceUsers());
+ }
+
+ return filteredCodeOwners.collect(toImmutableSet());
+ }
+
+ private Predicate<CodeOwner> filterOutNonVisibleCodeOwners(AbstractPathResource rsrc) {
+ return codeOwner -> {
+ if (isVisibleTo(rsrc, codeOwner)) {
+ return true;
+ }
+ logger.atFine().log(
+ "Filtering out %s because this code owner cannot see the branch %s",
+ codeOwner, rsrc.getBranch().branch());
+ return false;
+ };
+ }
+
+ private Predicate<CodeOwner> filterOutServiceUsers() {
+ return codeOwner -> {
+ if (!isServiceUser(codeOwner)) {
+ return true;
+ }
+ logger.atFine().log("Filtering out %s because this code owner is a service user", codeOwner);
+ return false;
+ };
}
/** Whether the given resource is visible to the given code owner. */
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
index c2c22df..a94d848 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
@@ -92,7 +92,9 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
serviceUserClassifier,
- codeOwnerJsonFactory);
+ codeOwnerJsonFactory,
+ /** suggest = */
+ false);
this.repoManager = repoManager;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 3483614..216d264 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -61,7 +61,9 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
serviceUserClassifier,
- codeOwnerJsonFactory);
+ codeOwnerJsonFactory,
+ /** suggest = */
+ true);
}
@Override
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index b9032cb..e6d8955 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -410,39 +410,6 @@
}
@Test
- public void codeOwnersThatAreServiceUsersAreFilteredOut() throws Exception {
- TestAccount serviceUser =
- accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
-
- // Create a code owner config with 2 code owners.
- codeOwnerConfigOperations
- .newCodeOwnerConfig()
- .project(project)
- .branch("master")
- .folderPath("/")
- .addCodeOwnerEmail(admin.email())
- .addCodeOwnerEmail(serviceUser.email())
- .create();
-
- // Check that both code owners are suggested.
- assertThat(queryCodeOwners("/foo/bar/baz.md"))
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id(), serviceUser.id());
-
- // Make 'serviceUser' a service user.
- groupOperations
- .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
- .forUpdate()
- .addMember(serviceUser.id())
- .update();
-
- // Expect that 'serviceUser' is filtered out now.
- assertThat(queryCodeOwners("/foo/bar/baz.md"))
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id());
- }
-
- @Test
public void codeOwnersThatCannotSeeTheBranchAreFilteredOut() throws Exception {
// Create a code owner config with 2 code owners.
codeOwnerConfigOperations
@@ -714,19 +681,6 @@
}
@Test
- @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "service.user@example.com")
- public void globalCodeOwnersThatAreServiceUsersAreFilteredOut() throws Exception {
- TestAccount serviceUser =
- accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
- groupOperations
- .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
- .forUpdate()
- .addMember(serviceUser.id())
- .update();
- assertThat(queryCodeOwners("/foo/bar/baz.md")).isEmpty();
- }
-
- @Test
public void getDefaultCodeOwners() throws Exception {
// Create default code owner config file in refs/meta/config.
codeOwnerConfigOperations
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
index db43d51..341beaf 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
@@ -18,7 +18,11 @@
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.hasAccountId;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -42,6 +46,7 @@
*/
public class GetCodeOwnersForPathInBranchIT extends AbstractGetCodeOwnersForPathIT {
@Inject private ProjectOperations projectOperations;
+ @Inject private GroupOperations groupOperations;
@Override
protected CodeOwners getCodeOwnersApi() throws RestApiException {
@@ -121,4 +126,47 @@
getCodeOwnersApi().query().forRevision(revision.name()), "/foo/bar/baz.md"));
assertThat(exception).hasMessageThat().isEqualTo("unknown revision");
}
+
+ @Test
+ public void codeOwnersThatAreServiceUsersAreIncluded() throws Exception {
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+
+ // Make 'serviceUser' a service user.
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+
+ // Create a code owner config with 2 code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(serviceUser.email())
+ .create();
+
+ // Expect that 'serviceUser' is included.
+ assertThat(queryCodeOwners("/foo/bar/baz.md"))
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), serviceUser.id());
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "service.user@example.com")
+ public void globalCodeOwnersThatAreServiceUsersAreIncluded() throws Exception {
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+ assertThat(queryCodeOwners("/foo/bar/baz.md"))
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(serviceUser.id());
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
index 428cc38..7e82e20 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -21,8 +21,12 @@
import static java.util.stream.Collectors.toMap;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -47,6 +51,7 @@
public class GetCodeOwnersForPathInChangeIT extends AbstractGetCodeOwnersForPathIT {
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
+ @Inject private GroupOperations groupOperations;
private String changeId;
@@ -174,4 +179,50 @@
codeOwnersApiFactory.change(changeId, "current").query().get(TEST_PATHS.get(0));
assertThat(codeOwnerInfos).comparingElementsUsing(hasAccountId()).containsExactly(user.id());
}
+
+ @Test
+ public void codeOwnersThatAreServiceUsersAreFilteredOut() throws Exception {
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+
+ // Create a code owner config with 2 code owners.
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(serviceUser.email())
+ .create();
+
+ // Check that both code owners are suggested.
+ assertThat(queryCodeOwners("/foo/bar/baz.md"))
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), serviceUser.id());
+
+ // Make 'serviceUser' a service user.
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+
+ // Expect that 'serviceUser' is filtered out now.
+ assertThat(queryCodeOwners("/foo/bar/baz.md"))
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id());
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "service.user@example.com")
+ public void globalCodeOwnersThatAreServiceUsersAreFilteredOut() throws Exception {
+ TestAccount serviceUser =
+ accountCreator.create("serviceUser", "service.user@example.com", "Service User", null);
+ groupOperations
+ .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+ .forUpdate()
+ .addMember(serviceUser.id())
+ .update();
+ assertThat(queryCodeOwners("/foo/bar/baz.md")).isEmpty();
+ }
}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index c2aafff..26cf341 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -278,7 +278,6 @@
* are referenced by an email with a disallowed domain (see
[allowedEmailDomain configuration](config.html#pluginCodeOwnersAllowedEmailDomain))
* do not have read access to the branch
-* are service users (members of the `Service Users` group)
are omitted from the result.
@@ -391,16 +390,22 @@
## <a id="revision-endpoints"> Revision Endpoints
-### <a id="list-code-owners-for-path-in-change"> List Code Owners for path in change
+### <a id="list-code-owners-for-path-in-change"> Suggest Code Owners for path in change
_'GET /changes/[\{change-id}](../../../Documentation/rest-api-changes.html#change-id)/revisions/[\{revison-id\}](../../../Documentation/rest-api-changes.html#revision-id)/code_owners/[\{path\}](#path)'_
-Lists the accounts that are code owners of a file in a change revision.
+Suggests accounts that are code owners of a file in a change revision.
The code owners are computed from the owner configuration at the tip of the
change's destination branch.
This REST endpoint has the exact same request and response format as the
-[REST endpoint to list code owners for a path in a branch](#list-code-owners-for-path-in-branch).
+[REST endpoint to list code owners for a path in a branch](#list-code-owners-for-path-in-branch),
+but filters out code owners that which should be omitted from the code owner
+suggestion.
+
+The following code owners are filtered out additionally:
+
+* service users (members of the `Service Users` group)
### <a id="check-code-owner-config-files-in-revision">Check Code Owner Config Files In Revision
_'POST /changes/[\{change-id}](../../../Documentation/rest-api-changes.html#change-id)/revisions/[\{revison-id\}](../../../Documentation/rest-api-changes.html#revision-id)/code_owners.check_config'_
