Suggest global code owners if they are not service users When suggesting code owners, global code owners come always last. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I4104fb154c397ff4b75e04d0c80078df1416a890
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java index 6a0c27c..dd4fd6a 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -21,6 +21,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.flogger.FluentLogger; +import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.client.ListAccountsOption; import com.google.gerrit.extensions.client.ListOption; import com.google.gerrit.extensions.restapi.AuthException; @@ -32,6 +33,7 @@ import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring; +import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; import com.google.gerrit.server.account.AccountDirectory.FillOptions; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.account.ServiceUserClassifier; @@ -59,6 +61,7 @@ @VisibleForTesting public static final int DEFAULT_LIMIT = 10; private final PermissionBackend permissionBackend; + private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration; private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy; private final Provider<CodeOwnerResolver> codeOwnerResolver; private final ServiceUserClassifier serviceUserClassifier; @@ -94,11 +97,13 @@ protected AbstractGetCodeOwnersForPath( PermissionBackend permissionBackend, + CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, Provider<CodeOwnerResolver> codeOwnerResolver, ServiceUserClassifier serviceUserClassifier, CodeOwnerJson.Factory codeOwnerJsonFactory) { this.permissionBackend = permissionBackend; + this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy; this.codeOwnerResolver = codeOwnerResolver; this.serviceUserClassifier = serviceUserClassifier; @@ -112,9 +117,14 @@ parseHexOptions(); validateLimit(); - // The maximal possible distance. This is the distance that applies to code owners that are - // defined in the root code owner configuration. - int maxDistance = rsrc.getPath().getNameCount(); + // The distance that applies to code owners that are defined in the root code owner + // configuration. + int rootDistance = rsrc.getPath().getNameCount(); + + // The maximal possible distance. This is the distance that applies to global code owners and is + // by 1 greater than the distance that applies to code owners that are defined in the root code + // owner configuration. + int maxDistance = rootDistance + 1; CodeOwnerScoring.Builder distanceScoring = CodeOwnerScore.DISTANCE.createScoring(maxDistance); @@ -127,7 +137,7 @@ ImmutableSet<CodeOwner> pathCodeOwners = codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath()); codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners)); - int distance = maxDistance - codeOwnerConfig.key().folderPath().getNameCount(); + int distance = rootDistance - codeOwnerConfig.key().folderPath().getNameCount(); pathCodeOwners.forEach( localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance)); @@ -139,12 +149,29 @@ return codeOwners.size() < limit; }); + if (codeOwners.size() < limit) { + ImmutableSet<CodeOwner> globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project()); + globalCodeOwners.forEach( + codeOwner -> distanceScoring.putValueForCodeOwner(codeOwner, maxDistance)); + codeOwners.addAll(filterCodeOwners(rsrc, globalCodeOwners)); + } + return Response.ok( codeOwnerJsonFactory .create(getFillOptions()) .format(sortAndLimit(distanceScoring.build(), ImmutableSet.copyOf(codeOwners)))); } + private ImmutableSet<CodeOwner> getGlobalCodeOwners(Project.NameKey projectName) { + ImmutableSet<CodeOwner> globalCodeOwners = + codeOwnerResolver + .get() + .resolve(codeOwnersPluginConfiguration.getGlobalCodeOwners(projectName)) + .collect(toImmutableSet()); + logger.atFine().log("including global code owners = %s", globalCodeOwners); + return globalCodeOwners; + } + /** * Filters out code owners that should not be suggested. *
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java index 6c5a3a6..baf260d 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
@@ -25,6 +25,7 @@ import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver; +import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; import com.google.gerrit.server.account.ServiceUserClassifier; import com.google.gerrit.server.change.IncludedInResolver; import com.google.gerrit.server.git.GitRepositoryManager; @@ -65,6 +66,7 @@ @Inject GetCodeOwnersForPathInBranch( PermissionBackend permissionBackend, + CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, Provider<CodeOwnerResolver> codeOwnerResolver, ServiceUserClassifier serviceUserClassifier, @@ -72,6 +74,7 @@ GitRepositoryManager repoManager) { super( permissionBackend, + codeOwnersPluginConfiguration, codeOwnerConfigHierarchy, codeOwnerResolver, serviceUserClassifier,
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java index 4ad31aa..43a5d21 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -20,6 +20,7 @@ import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver; +import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration; import com.google.gerrit.server.account.ServiceUserClassifier; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -40,12 +41,14 @@ @Inject GetCodeOwnersForPathInChange( PermissionBackend permissionBackend, + CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, Provider<CodeOwnerResolver> codeOwnerResolver, ServiceUserClassifier serviceUserClassifier, CodeOwnerJson.Factory codeOwnerJsonFactory) { super( permissionBackend, + codeOwnersPluginConfiguration, codeOwnerConfigHierarchy, codeOwnerResolver, serviceUserClassifier,
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 d0951ac..360e2a7 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -630,4 +630,95 @@ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar/baz.md"); assertThat(codeOwnerInfos).comparingElementsUsing(hasAccountId()).containsExactly(admin.id()); } + + @Test + @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "global.owner@example.com") + public void getGlobalCodeOwners() throws Exception { + TestAccount globalOwner = + accountCreator.create("global_owner", "global.owner@example.com", "Global Owner", null); + assertThat(queryCodeOwners("/foo/bar/baz.md")) + .comparingElementsUsing(hasAccountId()) + .containsExactly(globalOwner.id()); + } + + @Test + @GerritConfig( + name = "plugin.code-owners.globalCodeOwner", + values = {"global.owner1@example.com", "global.owner2@example.com"}) + public void getWithGlobalCodeOwnersAndLimit() throws Exception { + TestAccount globalOwner1 = + accountCreator.create( + "global_owner_1", "global.owner1@example.com", "Global Owner 1", null); + TestAccount globalOwner2 = + accountCreator.create( + "global_owner_2", "global.owner2@example.com", "Global Owner 2", null); + + TestAccount user2 = accountCreator.user2(); + + // create some code owner configs + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(admin.email()) + .create(); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/bar/") + .addCodeOwnerEmail(user.email()) + .addCodeOwnerEmail(user2.email()) + .create(); + + // get code owners with different limits + List<CodeOwnerInfo> codeOwnerInfos = + queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md"); + assertThat(codeOwnerInfos).hasSize(1); + // the first 2 code owners have the same scoring, so their order is random and we don't know + // which of them we get when the limit is 1 + assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isAnyOf(user.id(), user2.id()); + + codeOwnerInfos = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md"); + assertThat(codeOwnerInfos) + .comparingElementsUsing(hasAccountId()) + .containsExactly(user.id(), user2.id()); + + codeOwnerInfos = getCodeOwnersApi().query().withLimit(3).get("/foo/bar/baz.md"); + assertThat(codeOwnerInfos) + .comparingElementsUsing(hasAccountId()) + .containsExactly(admin.id(), user.id(), user2.id()); + + codeOwnerInfos = getCodeOwnersApi().query().withLimit(4).get("/foo/bar/baz.md"); + assertThat(codeOwnerInfos).hasSize(4); + // the order of the first 2 code owners is random + assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isAnyOf(user.id(), user2.id()); + assertThatList(codeOwnerInfos).element(1).hasAccountIdThat().isAnyOf(user.id(), user2.id()); + assertThatList(codeOwnerInfos).element(2).hasAccountIdThat().isEqualTo(admin.id()); + // the order of the global code owners is random + assertThatList(codeOwnerInfos) + .element(3) + .hasAccountIdThat() + .isAnyOf(globalOwner1.id(), globalOwner2.id()); + + codeOwnerInfos = getCodeOwnersApi().query().withLimit(5).get("/foo/bar/baz.md"); + assertThat(codeOwnerInfos) + .comparingElementsUsing(hasAccountId()) + .containsExactly(admin.id(), user.id(), user2.id(), globalOwner1.id(), globalOwner2.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(); + } }