When listing code owners for a path let callers know if all users own it Adds a ownedByAllUsers boolean field to CodeOwnersInfo which is returned by the get code owners REST endpoints (GetCodeOwnersForPathInChange and GetCodeOwnersForPathInBranch). This allows callers to know if a path is owned by all users. E.g. the Gerrit frontend should make this transparent to users when suggesting code owners. In order to know whether a path is owned by all users we need to continue inspecting code owner configs in parent folders even if we have already found enough code owners to satisfy the requested limit, as parent code owner configs may assign code ownership to all users, which we need to know in order to populate the ownedByAllUsers field in the response. We can still abort the iteration over code owner config files if we have already found that the path is owned by all users. Needing to check all relevant code owner config files if paths are not owned by all users means that the get code owners calls become a bit more expensive, but it shouldn't be significantly slower. If performance becomes an issue we need to implement caching for code owner configs. FWIW if we ever add further scores for code owners (besides the distance score), which we likely will do, we also need to iterate over all relevant code owner configs. So iterating over all code owner configs already now, shouldn't be an issue. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I8c564c959850ad21822bef0b1e58d23b7ac0ff24
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java index 5ce7ac2..2d3a84c 100644 --- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java +++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java
@@ -26,4 +26,11 @@ public class CodeOwnersInfo { /** List of code owners. */ public List<CodeOwnerInfo> codeOwners; + + /** + * Whether the path is owned by all users. + * + * <p>Not set if {@code false}. + */ + public Boolean ownedByAllUsers; }
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java index a0ab229..1464ecb 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -58,6 +58,7 @@ import java.util.Optional; import java.util.Random; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Stream; import org.kohsuke.args4j.Option; @@ -157,6 +158,7 @@ CodeOwnerScoring.Builder distanceScoring = CodeOwnerScore.DISTANCE.createScoring(maxDistance); Set<CodeOwner> codeOwners = new HashSet<>(); + AtomicBoolean ownedByAllUsers = new AtomicBoolean(false); codeOwnerConfigHierarchy.visit( rsrc.getBranch(), rsrc.getRevision(), @@ -167,6 +169,7 @@ codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners.codeOwners())); if (pathCodeOwners.ownedByAllUsers()) { + ownedByAllUsers.set(true); fillUpWithRandomUsers(rsrc, codeOwners, limit); if (codeOwners.size() < limit) { @@ -191,15 +194,13 @@ .forEach( localCodeOwner -> distanceScoring.putValueForCodeOwner(localCodeOwner, distance)); - // If codeOwners.size() >= limit we have gathered enough code owners and do not need to - // look at further code owner configs. - // We can abort here, since all further code owners will have a lower distance scoring - // and hence they would appear at the end of the sorted code owners list and be dropped - // due to the limit. - return codeOwners.size() < limit; + // If codeOwners.size() >= limit we have gathered enough code owners, but we still need to + // inspect the further code owner configs to know if the path is maybe owned by all user + // (in which case we need to set the ownedByAllUsers flag in the response). + return true; }); - if (codeOwners.size() < limit) { + if (codeOwners.size() < limit || !ownedByAllUsers.get()) { CodeOwnerResolverResult globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project()); globalCodeOwners .codeOwners() @@ -208,6 +209,7 @@ codeOwners.addAll(filterCodeOwners(rsrc, globalCodeOwners.codeOwners())); if (globalCodeOwners.ownedByAllUsers()) { + ownedByAllUsers.set(true); fillUpWithRandomUsers(rsrc, codeOwners, limit); } } @@ -217,6 +219,7 @@ codeOwnerJsonFactory .create(getFillOptions()) .format(sortAndLimit(distanceScoring.build(), ImmutableSet.copyOf(codeOwners))); + codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null; return Response.ok(codeOwnersInfo); }
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java index 5e30c97..28ea59e 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.codeOwnerInfos; import static com.google.gerrit.truth.ListSubject.elements; +import com.google.common.truth.BooleanSubject; import com.google.common.truth.FailureMetadata; import com.google.common.truth.Subject; import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo; @@ -53,6 +54,10 @@ .thatCustom(codeOwnersInfo().codeOwners, codeOwnerInfos()); } + public BooleanSubject hasOwnedByAllUsersThat() { + return check("ownedByAllUsers").that(codeOwnersInfo().ownedByAllUsers); + } + private CodeOwnersInfo codeOwnersInfo() { isNotNull(); return codeOwnersInfo;
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 9138ef0..cd57b24 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -45,6 +45,7 @@ import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions; import com.google.gerrit.plugins.codeowners.api.CodeOwners; import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet; import com.google.gerrit.plugins.codeowners.restapi.GetCodeOwnersForPathInBranch; import com.google.inject.Inject; @@ -104,8 +105,11 @@ .folderPath("/abc/def/") .addCodeOwnerEmail(admin.email()) .addCodeOwnerEmail(user.email()) + .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD) .create(); - assertThat(queryCodeOwners("/foo/bar/baz.md")).hasCodeOwnersThat().isEmpty(); + CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md"); + assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty(); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isNull(); } @Test @@ -156,6 +160,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountName()) .containsExactly(null, null, null); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isNull(); } @Test @@ -186,15 +191,16 @@ .addCodeOwnerEmail(user.email()) .create(); - // 3. code owner config that makes "admin" a code owner, but for this test this code owner - // config is ignored, since the 2. code owner config ignores code owners from parent code owner - // configs + // 3. code owner config that makes "admin" a code owner and assigns code ownership to all users, + // but for this test this code owner config is ignored, since the 2. code owner config ignores + // code owners from parent code owner configs codeOwnerConfigOperations .newCodeOwnerConfig() .project(project) .branch("master") .folderPath("/") .addCodeOwnerEmail(admin.email()) + .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD) .create(); // Assert the code owners for "/foo/bar/baz.md". This evaluates the code owner configs in the @@ -208,6 +214,7 @@ .comparingElementsUsing(hasAccountId()) .containsExactly(user2.id(), user.id()) .inOrder(); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isNull(); } @Test @@ -629,6 +636,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(user.id(), user2.id(), admin.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // Query code owners with a limit. requestScopeOperations.setApiUser(user.id()); @@ -644,6 +652,7 @@ .element(1) .hasAccountIdThat() .isAnyOf(user.id(), user2.id(), admin.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); } @Test @@ -922,6 +931,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(user.id(), user2.id(), admin.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // Query code owners with a limit. requestScopeOperations.setApiUser(user.id()); @@ -937,6 +947,7 @@ .element(1) .hasAccountIdThat() .isAnyOf(user.id(), user2.id(), admin.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); } @Test @@ -963,6 +974,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(user.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // user2 can see user3 and itself requestScopeOperations.setApiUser(user2.id()); @@ -971,6 +983,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // admin can see all users requestScopeOperations.setApiUser(admin.id()); @@ -979,6 +992,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(admin.id(), user.id(), user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // Query code owners with a limit, user2 can see user3 and itself requestScopeOperations.setApiUser(user2.id()); @@ -989,6 +1003,7 @@ .element(0) .hasAccountIdThat() .isAnyOf(user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); } @Test @@ -1023,6 +1038,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(user.id(), user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // user2 can see user3 and itself requestScopeOperations.setApiUser(user2.id()); @@ -1031,6 +1047,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // admin can see all users requestScopeOperations.setApiUser(admin.id()); @@ -1039,6 +1056,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(admin.id(), user.id(), user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); // Query code owners with a limit, user2 can see user3 and itself requestScopeOperations.setApiUser(user2.id()); @@ -1049,6 +1067,7 @@ .element(0) .hasAccountIdThat() .isAnyOf(user2.id(), user3.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); } @Test @@ -1069,6 +1088,7 @@ requestScopeOperations.setApiUser(user.id()); CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md"); assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty(); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); } @Test @@ -1096,6 +1116,7 @@ .hasCodeOwnersThat() .comparingElementsUsing(hasAccountId()) .containsExactly(admin.id(), user.id(), user2.id()); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); } @Test @@ -1205,4 +1226,92 @@ .inOrder(); } } + + @Test + public void allUsersOwnershipThatIsAssignedInParentIsConsideredEvenIfLimitWasAlreadyReached() + throws Exception { + // Create some code owner configs. + // The order below reflects the order in which the code owner configs are evaluated. + + // 1. code owner config that makes "user" a code owner, inheriting code owners from parent code + // owner configs is enabled by default + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/bar/") + .addCodeOwnerEmail(user.email()) + .create(); + + // 2. code owner config that makes "admin" a code owner, inheriting code owners from parent code + // owner configs is enabled by default + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(admin.email()) + .create(); + + // 3. code owner config that makes all users code owners + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(CodeOwnerResolver.ALL_USERS_WILDCARD) + .create(); + + // Query code owners for "/foo/bar/baz.md" with limit 2. After inspecting code owner config 1. + // and 2. enough code owners are found to satisfy the limit, but code owner config 3. is still + // inspected so that the ownedByAllUsers field in the response gets set. + CodeOwnersInfo codeOwnersInfo = + queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md"); + assertThat(codeOwnersInfo) + .hasCodeOwnersThat() + .comparingElementsUsing(hasAccountId()) + .containsExactly(user.id(), admin.id()) + .inOrder(); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); + } + + @Test + @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "*") + public void allUsersOwnershipThatIsAssignedAsGlobalOwnerIsConsideredEvenIfLimitWasAlreadyReached() + throws Exception { + // Create some code owner configs. + // The order below reflects the order in which the code owner configs are evaluated. + + // 1. code owner config that makes "user" a code owner, inheriting code owners from parent code + // owner configs is enabled by default + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/bar/") + .addCodeOwnerEmail(user.email()) + .create(); + + // 2. code owner config that makes "admin" a code owner, inheriting code owners from parent code + // owner configs is enabled by default + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(admin.email()) + .create(); + + // Query code owners for "/foo/bar/baz.md" with limit 2. After inspecting code owner config 1. + // and 2. enough code owners are found to satisfy the limit, but global code owners are still + // inspected so that the ownedByAllUsers field in the response gets set. + CodeOwnersInfo codeOwnersInfo = + queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md"); + assertThat(codeOwnersInfo) + .hasCodeOwnersThat() + .comparingElementsUsing(hasAccountId()) + .containsExactly(user.id(), admin.id()) + .inOrder(); + assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue(); + } }
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md index 6656020..df7d90e 100644 --- a/resources/Documentation/rest-api.md +++ b/resources/Documentation/rest-api.md
@@ -788,9 +788,10 @@ ### <a id="code-owners-info"> CodeOwnersInfo The `CodeOwnersInfo` entity contains information about a list of code owners. -| Field Name | Description | -| ------------- | ----------- | -| `code_owners` | List of code owners as [CodeOwnerInfo](#code-owner-info) entities. The code owners are sorted by [score](#scores). +| Field Name | | Description | +| ------------- | -------- | ----------- | +| `code_owners` | | List of code owners as [CodeOwnerInfo](#code-owner-info) entities. The code owners are sorted by [score](#scores). +| `owned_by_all_users` | optional | Whether the path is owned by all users. Not set if `false`. ### <a id="file-code-owner-status-info"> FileCodeOwnerStatusInfo The `FileCodeOwnerStatusInfo` entity describes the code owner statuses for a