Use change number as seed for sorting when suggesting code owners for change
For suggesting code owners for paths in changes all requests should use
the same seed so that the sort order is stable across requests and
grouping of files with the same code owners works.
The initial idea was that the frontend generates a seed and then uses
the same seed for all requests that retrieve code owners for the change.
A disadvantage with this would be that the suggested code owners may
change whenever the change screen is reloaded (since reloading would
result in generating a new seed). To avoid this we will simply use the
change number as seed, if no seed was specified. This can be done on
server-side and there is also no need to adapt the frontend.
When listing code owners for a path in a branch we continue to use a
random seed if none was specified.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I1279e47666cca08c8d8ad616c4b9f073575c1abe
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 3dd3ab9..7a010e7 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -142,6 +142,10 @@
parseHexOptions();
validateLimit();
+ if (!seed.isPresent()) {
+ seed = getDefaultSeed(rsrc);
+ }
+
// The distance that applies to code owners that are defined in the root code owner
// configuration.
int rootDistance = rsrc.getPath().getNameCount();
@@ -250,6 +254,18 @@
return codeOwners;
}
+ /**
+ * Returns the seed that should by default be used for sorting, if none was specified on the
+ * request.
+ *
+ * <p>If {@link Optional#empty()} is returned, a random seed will be used.
+ *
+ * @param rsrc resource on which the request is being performed
+ */
+ protected Optional<Long> getDefaultSeed(R rsrc) {
+ return Optional.empty();
+ }
+
private Stream<CodeOwner> getVisibleCodeOwners(R rsrc, ImmutableSet<CodeOwner> allCodeOwners) {
return allCodeOwners.stream()
.filter(
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index 0e6e2fa..784d42b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -31,6 +31,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.List;
+import java.util.Optional;
import java.util.function.Predicate;
import java.util.stream.Stream;
@@ -78,6 +79,12 @@
}
@Override
+ protected Optional<Long> getDefaultSeed(CodeOwnersInChangeCollection.PathResource rsrc) {
+ // use the change number as seed so that the sort order for a change is always stable
+ return Optional.of(Long.valueOf(rsrc.getRevisionResource().getChange().getId().get()));
+ }
+
+ @Override
protected Stream<CodeOwner> filterCodeOwners(
CodeOwnersInChangeCollection.PathResource rsrc, Stream<CodeOwner> codeOwners) {
return codeOwners.filter(filterOutChangeOwner(rsrc)).filter(filterOutServiceUsers());
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 6e14d21..b5d874f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -23,7 +23,6 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.stream.Collectors.toList;
-import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.TestAccount;
@@ -478,63 +477,6 @@
}
@Test
- public void getCodeOwnersOrderNotDefinedIfCodeOwnersHaveTheSameScoring() throws Exception {
- TestAccount user2 = accountCreator.user2();
- TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
- TestAccount user4 = accountCreator.create("user4", "user4@example.com", "User4", null);
-
- codeOwnerConfigOperations
- .newCodeOwnerConfig()
- .project(project)
- .branch("master")
- .folderPath("/")
- .addCodeOwnerEmail(admin.email())
- .addCodeOwnerEmail(user2.email())
- .addCodeOwnerEmail(user3.email())
- .addCodeOwnerEmail(user4.email())
- .create();
-
- codeOwnerConfigOperations
- .newCodeOwnerConfig()
- .project(project)
- .branch("master")
- .folderPath("/foo/")
- .addCodeOwnerEmail(user.email())
- .create();
-
- List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar.md");
- assertThat(codeOwnerInfos)
- .comparingElementsUsing(hasAccountId())
- .containsExactly(admin.id(), user.id(), user2.id(), user3.id(), user4.id());
-
- // The first code owner in the result should be user as user has the best distance score.
- assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isEqualTo(user.id());
-
- // The order of the other code owners is random since they have the same score.
- // Check that the order of the code owners with the same score is different for further requests
- // at least once.
- List<Account.Id> accountIdsInRetrievedOrder1 =
- codeOwnerInfos.stream().map(info -> Account.id(info.account._accountId)).collect(toList());
- boolean foundOtherOrder = false;
- for (int i = 0; i < 10; i++) {
- codeOwnerInfos = queryCodeOwners("/foo/bar.md");
- List<Account.Id> accountIdsInRetrievedOrder2 =
- codeOwnerInfos.stream()
- .map(info -> Account.id(info.account._accountId))
- .collect(toList());
- if (!accountIdsInRetrievedOrder1.equals(accountIdsInRetrievedOrder2)) {
- foundOtherOrder = true;
- break;
- }
- }
- if (!foundOtherOrder) {
- fail(
- String.format(
- "expected different order, but order was always %s", accountIdsInRetrievedOrder1));
- }
- }
-
- @Test
public void getCodeOwnersIsLimitedByDefault() throws Exception {
// Create a code owner config that has more code owners than the number of code owners which are
// returned by default.
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
index 341beaf..2e14b98 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
@@ -15,13 +15,17 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.assertThatList;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.hasAccountId;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.stream.Collectors.toList;
+import static org.junit.Assert.fail;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -54,6 +58,63 @@
}
@Test
+ public void getCodeOwnersOrderNotDefinedIfCodeOwnersHaveTheSameScoring() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+ TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
+ TestAccount user4 = accountCreator.create("user4", "user4@example.com", "User4", null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user2.email())
+ .addCodeOwnerEmail(user3.email())
+ .addCodeOwnerEmail(user4.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id(), user3.id(), user4.id());
+
+ // The first code owner in the result should be user as user has the best distance score.
+ assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isEqualTo(user.id());
+
+ // The order of the other code owners is random since they have the same score.
+ // Check that the order of the code owners with the same score is different for further requests
+ // at least once.
+ List<Account.Id> accountIdsInRetrievedOrder1 =
+ codeOwnerInfos.stream().map(info -> Account.id(info.account._accountId)).collect(toList());
+ boolean foundOtherOrder = false;
+ for (int i = 0; i < 10; i++) {
+ codeOwnerInfos = queryCodeOwners("/foo/bar.md");
+ List<Account.Id> accountIdsInRetrievedOrder2 =
+ codeOwnerInfos.stream()
+ .map(info -> Account.id(info.account._accountId))
+ .collect(toList());
+ if (!accountIdsInRetrievedOrder1.equals(accountIdsInRetrievedOrder2)) {
+ foundOtherOrder = true;
+ break;
+ }
+ }
+ if (!foundOtherOrder) {
+ fail(
+ String.format(
+ "expected different order, but order was always %s", accountIdsInRetrievedOrder1));
+ }
+ }
+
+ @Test
public void getCodeOwnersForRevision() throws Exception {
// Create an initial code owner config that only has 'admin' as code owner.
CodeOwnerConfig.Key codeOwnerConfigKey =
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 27587e2..b74df75 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -16,9 +16,12 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.assertThatList;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerInfoSubject.hasAccountId;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toMap;
+import static org.junit.Assert.fail;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
@@ -26,6 +29,7 @@
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -97,6 +101,58 @@
}
@Test
+ public void getCodeOwnersOrderIsAlwaysTheSameIfCodeOwnersHaveTheSameScoring() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+ TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
+ TestAccount user4 = accountCreator.create("user4", "user4@example.com", "User4", null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user2.email())
+ .addCodeOwnerEmail(user3.email())
+ .addCodeOwnerEmail(user4.email())
+ .create();
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ List<CodeOwnerInfo> codeOwnerInfos = queryCodeOwners("/foo/bar.md");
+ assertThat(codeOwnerInfos)
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(admin.id(), user.id(), user2.id(), user3.id(), user4.id());
+
+ // The first code owner in the result should be user as user has the best distance score.
+ assertThatList(codeOwnerInfos).element(0).hasAccountIdThat().isEqualTo(user.id());
+
+ // The order of the other code owners is random since they have the same score.
+ // Check that the order of the code owners with the same score is the same for further requests.
+ List<Account.Id> accountIdsInRetrievedOrder1 =
+ codeOwnerInfos.stream().map(info -> Account.id(info.account._accountId)).collect(toList());
+ for (int i = 0; i < 10; i++) {
+ codeOwnerInfos = queryCodeOwners("/foo/bar.md");
+ List<Account.Id> accountIdsInRetrievedOrder2 =
+ codeOwnerInfos.stream()
+ .map(info -> Account.id(info.account._accountId))
+ .collect(toList());
+ if (!accountIdsInRetrievedOrder1.equals(accountIdsInRetrievedOrder2)) {
+ fail(
+ String.format(
+ "expected always the same order %s, but order was different %s",
+ accountIdsInRetrievedOrder1, accountIdsInRetrievedOrder2));
+ }
+ }
+ }
+
+ @Test
public void getCodeOwnersForDeletedFile() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index c93adc0..f7b536a 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -568,6 +568,10 @@
* service users (members of the `Service Users` group)
* the change owner (since the change owner cannot be added as reviewer)
+In addition, by default the change number is used as seed if none was specified.
+This way the sort order on a change is always the same for files that have the
+exact same code owners (requires that the limit is the same on all requests).
+
### <a id="check-code-owner-config-files-in-revision">Check Code Owner Config Files In Revision
_'POST /changes/[\{change-id}](../../../Documentation/rest-api-changes.html#change-id)/revisions/[\{revison-id\}](../../../Documentation/rest-api-changes.html#revision-id)/code_owners.check_config'_
