Respect the suggest.skipServiceUsers config when suggesting code owners

By default Gerrit doesn't suggest service users, but this behavior can
be overriden by setting suggest.skipServiceUsers=false in the gerrit
config. When this setting was added the code-owners plugin was not
adapted to respect it. Fix this now.

Bug: Google b/352467354
Change-Id: I234fddf7386979e0848abd911cca9d85281e15fa
Signed-off-by: Edwin Kempin <ekempin@google.com>
Reviewed-on: https://gerrit-review.googlesource.com/c/plugins/code-owners/+/433118
Reviewed-by: Nitzan Gur-Furman <nitzan@google.com>
Tested-by: Zuul <zuul-63@gerritcodereview-ci.iam.gserviceaccount.com>

java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java

diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index b7149c5..bcd0072 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -39,14 +39,17 @@
 import com.google.gerrit.server.account.AccountControl;
 import com.google.gerrit.server.account.Accounts;
 import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.restapi.change.SuggestReviewers;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import java.util.Optional;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
+import org.eclipse.jgit.lib.Config;
 
 /**
  * REST endpoint that gets the code owners for an arbitrary path in a revision of a change.
@@ -59,10 +62,12 @@
 public class GetCodeOwnersForPathInChange
     extends AbstractGetCodeOwnersForPath<CodeOwnersInChangeCollection.PathResource> {
 
+  private final Config cfg;
   private final ServiceUserClassifier serviceUserClassifier;
 
   @Inject
   GetCodeOwnersForPathInChange(
+      @GerritServerConfig Config cfg,
       AccountVisibility accountVisibility,
       Accounts accounts,
       AccountControl.Factory accountControlFactory,
@@ -87,6 +92,7 @@
         codeOwnerResolver,
         codeOwnerJsonFactory,
         codeOwnerConfigFileJson);
+    this.cfg = cfg;
     this.serviceUserClassifier = serviceUserClassifier;
   }
 
@@ -222,6 +228,13 @@
   }
 
   private Predicate<CodeOwner> filterOutServiceUsers(ImmutableList.Builder<String> debugLogs) {
+    if (!cfg.getBoolean(
+        "suggest", "skipServiceUsers", SuggestReviewers.DEFAULT_SKIP_SERVICE_USERS)) {
+      // Returning true from the Predicate here means that the code owner should not be filtered
+      // out.
+      return codeOwner -> true;
+    }
+
     return codeOwner -> {
       if (!isServiceUser(codeOwner)) {
         // Returning true from the Predicate here means that the code owner should be kept.

javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java

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 a4d69f4..7d60771 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -300,6 +300,37 @@
   }
 
   @Test
+  @GerritConfig(name = "suggest.skipServiceUsers", value = "false")
+  public void codeOwnersThatAreServiceUsersAreNotFilteredOutIfSkippingServiceUsersIsDisabled()
+      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();
+
+    // Make 'serviceUser' a service user.
+    groupOperations
+        .group(groupCache.get(AccountGroup.nameKey("Service Users")).get().getGroupUUID())
+        .forUpdate()
+        .addMember(serviceUser.id())
+        .update();
+
+    // Check that both code owners are suggested, i.e. the service user is not filtered out.
+    assertThat(queryCodeOwners("/foo/bar/baz.md"))
+        .hasCodeOwnersThat()
+        .comparingElementsUsing(hasAccountId())
+        .containsExactly(admin.id(), serviceUser.id());
+  }
+
+  @Test
   @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "service.user@example.com")
   public void globalCodeOwnersThatAreServiceUsersAreFilteredOut() throws Exception {
     TestAccount serviceUser =

resources/Documentation/rest-api.md

diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index c97ba2c..ace28a0 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -742,7 +742,9 @@
 
 The following code owners are filtered out additionally:
 
-* [service users](#serviceUsers) (members of the `Service Users` group)
+* [service users](#serviceUsers) (members of the `Service Users` group), unless
+  Gerrit is [configured](../../../Documentation/config-gerrit.html#suggest.skipServiceUsers)
+  to not skip service users
 * the change owner (since the change owner cannot be added as reviewer)
 * code owners that are annotated with
   [LAST_RESORT_SUGGESTION](backend-find-owners.html#lastResortSuggestion),