Merge branch 'origin/stable-3.4' into stable-3.5
* origin/stable-3.4:
Filter out the files owners based on the owners' code-review value
Change-Id: I0f590874bdbfac256daa2642543e5a723952e202
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
index 42c6586..02581de 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -16,8 +16,11 @@
package com.googlesource.gerrit.owners.restapi;
import com.google.common.collect.Maps;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.client.ListChangesOption;
@@ -25,6 +28,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
@@ -64,6 +68,12 @@
private final ChangeData.Factory changeDataFactory;
private final PathOwnersEntriesCache cache;
+ static final String MISSING_CODE_REVIEW_LABEL =
+ String.format(
+ "Cannot calculate file onwers state when %s label is not configured",
+ LabelId.CODE_REVIEW);
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
@Inject
GetFilesOwners(
Accounts accounts,
@@ -88,6 +98,20 @@
public Response<FilesOwnersResponse> apply(RevisionResource revision)
throws AuthException, BadRequestException, ResourceConflictException, Exception {
Change change = revision.getChange();
+ Short codeReviewMaxValue =
+ revision
+ .getChangeResource()
+ .getChangeData()
+ .getLabelTypes()
+ .byLabel(LabelId.CODE_REVIEW)
+ .map(LabelType::getMaxPositive)
+ .orElseThrow(
+ () -> {
+ logger.atInfo().log(
+ "Project %s has no Code-Review label configured hence getting file owners is not possible.",
+ revision.getProject());
+ return new ResourceNotFoundException(MISSING_CODE_REVIEW_LABEL);
+ });
int id = revision.getChangeResource().getChange().getChangeId();
Project.NameKey project = change.getProject();
@@ -110,24 +134,55 @@
project.get(),
cache);
+ Map<String, Set<GroupOwner>> fileExpandedOwners =
+ Maps.transformValues(
+ owners.getFileOwners(),
+ ids ->
+ ids.stream()
+ .map(this::getOwnerFromAccountId)
+ .flatMap(Optional::stream)
+ .collect(Collectors.toSet()));
+
Map<String, Set<GroupOwner>> fileToOwners =
pluginSettings.expandGroups()
- ? Maps.transformValues(
- owners.getFileOwners(),
- ids ->
- ids.stream()
- .map(this::getOwnerFromAccountId)
- .flatMap(Optional::stream)
- .collect(Collectors.toSet()))
+ ? fileExpandedOwners
: Maps.transformValues(
owners.getFileGroupOwners(),
groupNames ->
groupNames.stream().map(GroupOwner::new).collect(Collectors.toSet()));
- return Response.ok(new FilesOwnersResponse(getLabels(id), fileToOwners));
+ Map<Integer, Map<String, Integer>> ownersLabels = getLabels(id);
+
+ Map<String, Set<GroupOwner>> filesWithPendingOwners =
+ Maps.filterEntries(
+ fileToOwners,
+ (fileOwnerEntry) ->
+ !isApprovedByOwner(
+ fileExpandedOwners.get(fileOwnerEntry.getKey()),
+ ownersLabels,
+ codeReviewMaxValue));
+
+ return Response.ok(new FilesOwnersResponse(ownersLabels, filesWithPendingOwners));
}
}
+ private boolean isApprovedByOwner(
+ Set<GroupOwner> fileOwners,
+ Map<Integer, Map<String, Integer>> ownersLabels,
+ short codeReviewMaxValue) {
+ return fileOwners.stream()
+ .filter(owner -> owner instanceof Owner)
+ .map(owner -> ((Owner) owner).getId())
+ .map(ownerId -> codeReviewLabelValue(ownersLabels, ownerId))
+ .anyMatch(value -> value.filter(v -> v == codeReviewMaxValue).isPresent());
+ }
+
+ private Optional<Integer> codeReviewLabelValue(
+ Map<Integer, Map<String, Integer>> ownersLabels, int ownerId) {
+ return Optional.ofNullable(ownersLabels.get(ownerId))
+ .flatMap(m -> Optional.ofNullable(m.get(LabelId.CODE_REVIEW)));
+ }
+
/**
* This method returns ta Map representing the "owners_labels" object of the response. When
* serialized the Map, has to to return the following JSON: the following JSON:
diff --git a/owners/src/main/resources/Documentation/rest-api.md b/owners/src/main/resources/Documentation/rest-api.md
index 4fb4285..68fb463 100644
--- a/owners/src/main/resources/Documentation/rest-api.md
+++ b/owners/src/main/resources/Documentation/rest-api.md
@@ -1,7 +1,7 @@
# Rest API
-The @PLUGIN@ exposes a Rest API endpoint to list the owners associated to each file and, for each owner,
-its associated labels and votes:
+The @PLUGIN@ exposes a Rest API endpoint to list the owners associated to each file that
+needs a review, and, for each owner, its current labels and votes:
```bash
GET /changes/{change-id}/revisions/{revision-id}/owners~files-owners
@@ -29,4 +29,7 @@
}
}
-```
\ No newline at end of file
+```
+
+> __NOTE__: The API does not work in the case when custom label is in
+> rules.pl configuration as described in [the config.md docs](https://gerrit.googlesource.com/plugins/owners/+/refs/heads/stable-3.4/owners/src/main/resources/Documentation/config.md#example-3-owners-file-without-matchers-and-custom-owner_approves-label)
\ No newline at end of file
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
index 53d13c9..1c551ec 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersIT.java
@@ -16,8 +16,8 @@
package com.googlesource.gerrit.owners.restapi;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
-import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit.Result;
@@ -25,6 +25,8 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
import com.google.gerrit.acceptance.config.GlobalPluginConfig;
+import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.entities.RefNames;
@@ -32,7 +34,9 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.server.project.testing.TestLabels;
import com.googlesource.gerrit.owners.entities.FilesOwnersResponse;
import com.googlesource.gerrit.owners.entities.GroupOwner;
import com.googlesource.gerrit.owners.entities.Owner;
@@ -99,7 +103,21 @@
}
@Test
- public void shouldReturnOwnersLabels() throws Exception {
+ public void shouldReturnOwnersLabelsWhenNotApprovedByOwners() throws Exception {
+ addOwnerFileToRoot(true);
+ String changeId = createChange().getChangeId();
+
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+
+ assertThat(resp.value().files)
+ .containsExactly("a.txt", Sets.newHashSet(new Owner(admin.fullName(), admin.id().get())));
+
+ assertThat(resp.value().ownersLabels).isEmpty();
+ }
+
+ @Test
+ public void shouldReturnEmptyResponseWhenApprovedByOwners() throws Exception {
addOwnerFileToRoot(true);
String changeId = createChange().getChangeId();
approve(changeId);
@@ -107,8 +125,7 @@
Response<FilesOwnersResponse> resp =
assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
- assertThat(resp.value().ownersLabels)
- .containsExactly(admin.id().get(), ImmutableMap.builder().put("Code-Review", 2).build());
+ assertThat(resp.value().files).isEmpty();
}
@Test
@@ -126,6 +143,20 @@
@Test
@GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false")
+ public void shouldReturnEmptyResponseWhenApprovedByOwnersWithUnexpandedFileOwners()
+ throws Exception {
+ addOwnerFileToRoot(true);
+ String changeId = createChange().getChangeId();
+ approve(changeId);
+
+ Response<FilesOwnersResponse> resp =
+ assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId)));
+
+ assertThat(resp.value().files).isEmpty();
+ }
+
+ @Test
+ @GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false")
public void shouldReturnResponseWithUnexpandedFileMatchersOwners() throws Exception {
addOwnerFileWithMatchersToRoot(true);
String changeId = createChange().getChangeId();
@@ -177,6 +208,30 @@
assertNotInheritFromProject(allProjects);
}
+ @Test
+ @UseLocalDisk
+ public void shouldThrowExceptionWhenCodeReviewLabelIsNotConfigured() throws Exception {
+ addOwnerFileToProjectConfig(project, false);
+ replaceCodeReviewWithLabel(
+ TestLabels.label(
+ "Foo", TestLabels.value(1, "Foo is fine"), TestLabels.value(-1, "Foo is not fine")));
+ String changeId = createChange().getChangeId();
+
+ ResourceNotFoundException thrown =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> ownersApi.apply(parseCurrentRevisionResource(changeId)));
+ assertThat(thrown).hasMessageThat().isEqualTo(GetFilesOwners.MISSING_CODE_REVIEW_LABEL);
+ }
+
+ private void replaceCodeReviewWithLabel(LabelType label) throws Exception {
+ try (ProjectConfigUpdate u = updateProject(allProjects)) {
+ u.getConfig().getLabelSections().remove(LabelId.CODE_REVIEW);
+ u.getConfig().upsertLabelType(label);
+ u.save();
+ }
+ }
+
private static <T> Response<T> assertResponseOk(Response<T> response) {
assertThat(response.statusCode()).isEqualTo(HttpServletResponse.SC_OK);
return response;
