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
