Stop resolving all users by default when listing code owners
The "resolve-all-users" option on the list code owners REST endpoints
resolves "*" to random users until the requested limit is reached. So
far this was the default behaviour. Since we are returning a flag that
tells the caller whether the file is owned by all users (see change
I8c564c959), callers can handle this case explicitly now and resolving
"*" to random users is not longer required in most cases. Hence we are
flipping the default for this option to "false".
There are still some cases were the old behaviour is wanted (e.g. if a
tool wants to automatically assign reviews to a code owner), which is
why we still support this option.
Another reason to flip the default is that Gerrit core actually doesn't
properly support boolean options that are true by default so that this
option is currently broken (it cannot be set to false since Gerrit core
does not forward false values to the REST endpoint, because it assumes
that false is the default value for all boolean options). By flipping
the default we avoid this problem (but it's not the main reason why we
are flipping the default).
Known callers (Android AyeAye and the Gerrit UI) have been adapted to
explicitly set "resolve-all-users=true" so that they are not affected by
the flip of the default value. The Gerrit UI intends to use
"resolve-all-users=false", but it still requires some work (so setting
"resolve-all-users=true" in the Gerrit UI is only temporarily).
Bug: Issue 14120
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ic887a27feab2935f8fcd97c221fa012876cb5e18
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 6fb8c15..0644b4c 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -86,7 +86,7 @@
private int limit = DEFAULT_LIMIT;
private Optional<Long> seed = Optional.empty();
- private boolean resolveAllUsers = true;
+ private boolean resolveAllUsers;
@Option(
name = "-o",
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 0826b9e..743bef0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -631,7 +631,8 @@
public void getAllUsersAsGlobalCodeOwners() throws Exception {
TestAccount user2 = accountCreator.user2();
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -640,7 +641,9 @@
// Query code owners with a limit.
requestScopeOperations.setApiUser(user.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -932,7 +935,8 @@
.addCodeOwnerEmail("*")
.create();
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -941,7 +945,9 @@
// Query code owners with a limit.
requestScopeOperations.setApiUser(user.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -975,7 +981,8 @@
// user can only see itself
requestScopeOperations.setApiUser(user.id());
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -984,7 +991,8 @@
// user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -993,7 +1001,8 @@
// admin can see all users
requestScopeOperations.setApiUser(admin.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1002,7 +1011,9 @@
// Query code owners with a limit, user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(1), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(1);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -1039,7 +1050,8 @@
// user can only see itself, user2 (because user is owner of a group that contains user2) and
// user3 (because user3 is member of a group that is visible to all users)
requestScopeOperations.setApiUser(user.id());
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1048,7 +1060,8 @@
// user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1057,7 +1070,8 @@
// admin can see all users
requestScopeOperations.setApiUser(admin.id());
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1066,7 +1080,9 @@
// Query code owners with a limit, user2 can see user3 and itself
requestScopeOperations.setApiUser(user2.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(1), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(1);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -1115,7 +1131,8 @@
// Since accounts.visibility = NONE, no account is visible and hence the list of code owners is
// empty.
- CodeOwnersInfo codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isTrue();
@@ -1126,7 +1143,8 @@
.update();
// If VIEW_ALL_ACCOUNTS is assigned, all accounts are visible now.
- codeOwnersInfo = queryCodeOwners("/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().setResolveAllUsers(true), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1182,14 +1200,17 @@
// Query code owners with limits, "*" is resolved to random users, but user2 should always be
// included since this user is set explicitly as code owner
CodeOwnersInfo codeOwnersInfo =
- queryCodeOwners(getCodeOwnersApi().query().withLimit(2), "/foo/bar/baz.md");
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(2), "/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().hasSize(2);
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
.contains(user2.id());
- codeOwnersInfo = queryCodeOwners(getCodeOwnersApi().query().withLimit(1), "/foo/bar/baz.md");
+ codeOwnersInfo =
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withLimit(1), "/foo/bar/baz.md");
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
@@ -1252,7 +1273,8 @@
long seed = (new Random()).nextLong();
CodeOwnersInfo codeOwnersInfo =
- queryCodeOwners(getCodeOwnersApi().query().withSeed(seed), "/foo/bar/baz.md");
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withSeed(seed), "/foo/bar/baz.md");
// all code owners have the same score, hence their order is random
assertThat(codeOwnersInfo)
.hasCodeOwnersThat()
@@ -1265,7 +1287,10 @@
.map(info -> Account.id(info.account._accountId))
.collect(toList());
for (int i = 0; i < 10; i++) {
- assertThat(queryCodeOwners(getCodeOwnersApi().query().withSeed(seed), "/foo/bar/baz.md"))
+ assertThat(
+ queryCodeOwners(
+ getCodeOwnersApi().query().setResolveAllUsers(true).withSeed(seed),
+ "/foo/bar/baz.md"))
.hasCodeOwnersThat()
.comparingElementsUsing(hasAccountId())
.containsExactlyElementsIn(expectedAccountIds)
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index b977f3c..9d4d3a8 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -458,7 +458,7 @@
| `O` | optional | [Account option](../../../Documentation/rest-api-accounts.html#query-options) in hex. For the explanation see `o` parameter.
| `limit`\|`n` | optional | Limit defining how many code owners should be returned at most. By default 10.
| `seed` | optional | Seed, as a long value, that should be used to shuffle code owners that have the same score. Can be used to make the sort order stable across several requests, e.g. to get the same set of random code owners for different file paths that have the same code owners. Important: the sort order is only stable if the requests use the same seed **and** the same limit. In addition, the sort order is not guaranteed to be stable if new accounts are created in between the requests, or if the account visibility is changed.
-| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `true` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
+| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `false` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
| `revision` | optional | Revision from which the code owner configs should be read as commit SHA1. Can be used to read historic code owners from this branch, but imports from other branches or repositories as well as default and global code owners from `refs/meta/config` are still read from the current revisions. If not specified the code owner configs are read from the HEAD revision of the branch. Not supported for getting code owners for a path in a change.
As a response a [CodeOwnersInfo](#code-owners-info) entity is returned that
