Expose auto-approved files in the owners REST API Extend the owners~files-owners response with a new files_auto_approved section so the UI can tell when the label was copied even if owned files were modified between patchsets, due to the auto-approval logic. Keep the new field aligned with the existing payload shape by returning the same owner details as files and files_approved. Make files_auto_approved and files_approved mutually exclusive: a file that is auto-approved is returned only in files_auto_approved. Files that also have a sufficient explicit owner vote on the current patch set are unchanged and returned as before. Bug: Issue 498512068 Change-Id: I0c252e21391daaf2e884519c8d27133641635e3a
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java b/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java index 3f6c937..184686b 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/entities/FilesOwnersResponse.java
@@ -25,14 +25,17 @@ public final Map<String, Set<GroupOwner>> files; public final Map<Integer, Map<String, Integer>> ownersLabels; public final Map<String, Set<GroupOwner>> filesApproved; + public final Map<String, Set<GroupOwner>> filesAutoApproved; public FilesOwnersResponse( Map<Integer, Map<String, Integer>> ownersLabels, Map<String, Set<GroupOwner>> files, - Map<String, Set<GroupOwner>> filesApproved) { + Map<String, Set<GroupOwner>> filesApproved, + Map<String, Set<GroupOwner>> filesAutoApproved) { this.ownersLabels = ownersLabels; this.files = files; this.filesApproved = filesApproved; + this.filesAutoApproved = filesAutoApproved; } @Override @@ -42,12 +45,13 @@ FilesOwnersResponse that = (FilesOwnersResponse) o; return Objects.equal(files, that.files) && Objects.equal(ownersLabels, that.ownersLabels) - && Objects.equal(filesApproved, that.filesApproved); + && Objects.equal(filesApproved, that.filesApproved) + && Objects.equal(filesAutoApproved, that.filesAutoApproved); } @Override public int hashCode() { - return Objects.hashCode(files, ownersLabels, filesApproved); + return Objects.hashCode(files, ownersLabels, filesApproved, filesAutoApproved); } @Override @@ -59,6 +63,8 @@ + ownersLabels + ", filesApproved=" + filesApproved + + ", filesAutoApproved=" + + filesAutoApproved + '}'; } }
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 e34eb89..c61c901 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
@@ -15,11 +15,18 @@ package com.googlesource.gerrit.owners.restapi; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.allowsAutoApprovalOnPatch; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.modifiedFilesBetweenPatchSets; +import static com.googlesource.gerrit.owners.AutoOwnersApprovalFunctions.touchedPaths; + import com.google.common.collect.Maps; +import com.google.common.collect.Sets; 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.PatchSet; +import com.google.gerrit.entities.PatchSetApproval; import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.api.GerritApi; import com.google.gerrit.extensions.client.ListChangesOption; @@ -34,6 +41,8 @@ import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.patch.DiffNotAvailableException; +import com.google.gerrit.server.patch.DiffOperations; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.query.change.ChangeData; import com.google.inject.Inject; @@ -53,8 +62,10 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.jgit.lib.Repository; @@ -65,6 +76,7 @@ private final AccountCache accountCache; private final ProjectCache projectCache; private final GitRepositoryManager repositoryManager; + private final DiffOperations diffOperations; private final PluginSettings pluginSettings; private final GerritApi gerritApi; private final PathOwnersEntriesCache cache; @@ -79,6 +91,7 @@ AccountCache accountCache, ProjectCache projectCache, GitRepositoryManager repositoryManager, + DiffOperations diffOperations, PluginSettings pluginSettings, GerritApi gerritApi, PathOwnersEntriesCache cache) { @@ -86,6 +99,7 @@ this.accountCache = accountCache; this.projectCache = projectCache; this.repositoryManager = repositoryManager; + this.diffOperations = diffOperations; this.pluginSettings = pluginSettings; this.gerritApi = gerritApi; this.cache = cache; @@ -165,8 +179,29 @@ isApprovedByOwner( fileExpandedOwners.get(fileOwnerEntry.getKey()), ownersLabels, label)); + Map<String, Set<GroupOwner>> filesAutoApprovedByOwners; + Map<String, Set<GroupOwner>> filesExplicitlyApprovedByOwners; + Set<String> filesAllowedAutoApproval = owners.getFileOwnersAllowedAutoApproval(); + + if (Sets.intersection(filesAllowedAutoApproval, filesApprovedByOwners.keySet()).isEmpty()) { + filesAutoApprovedByOwners = Map.of(); + filesExplicitlyApprovedByOwners = filesApprovedByOwners; + } else { + Set<String> filesAutoApproved = + getFilesAutoApproved(revision, changeData, filesApprovedByOwners); + filesAutoApprovedByOwners = + Maps.filterKeys(filesApprovedByOwners, filesAutoApproved::contains); + filesExplicitlyApprovedByOwners = + Maps.filterKeys( + filesApprovedByOwners, filePath -> !filesAutoApproved.contains(filePath)); + } + return Response.ok( - new FilesOwnersResponse(ownersLabels, filesWithPendingOwners, filesApprovedByOwners)); + new FilesOwnersResponse( + ownersLabels, + filesWithPendingOwners, + filesExplicitlyApprovedByOwners, + filesAutoApprovedByOwners)); } catch (InvalidOwnersFileException e) { logger.atSevere().withCause(e).log("Reading/parsing OWNERS file error."); throw new ResourceConflictException(e.getMessage(), e); @@ -240,6 +275,98 @@ .anyMatch(value -> value >= label.getScore()); } + private Set<String> getFilesAutoApproved( + RevisionResource revision, + ChangeData changeData, + Map<String, Set<GroupOwner>> filesApprovedByOwners) + throws IOException, InvalidOwnersFileException, DiffNotAvailableException { + PatchSet sourcePatchSet = getPreviousPatchSet(revision); + if (sourcePatchSet == null) { + return Set.of(); + } + Account.Id ownerId = revision.getChange().getOwner(); + String branch = changeData.change().getDest().branch(); + Project.NameKey project = changeData.project(); + + Set<String> allFilesTouchedInTheLastPatchSet = + touchedPaths( + modifiedFilesBetweenPatchSets( + diffOperations, project, sourcePatchSet, revision.getPatchSet())); + + Map<Account.Id, List<PatchSetApproval>> approvalsByAccount = + changeData.currentApprovals().stream() + .collect(Collectors.groupingBy(PatchSetApproval::accountId)); + + List<PatchSetApproval> changeOwnerApprovals = approvalsByAccount.get(ownerId); + + // If the change owner didn't approve the label or the label was not copied from the previous + // patch set then auto-owners-approvals cannot qualify for this patch-set. + if (changeOwnerApprovals == null + || changeOwnerApprovals.stream().anyMatch(isNotCopiedApproval())) { + return Set.of(); + } + + // Otherwise we check if the change owner was eligible for auto-owners-approved + Set<String> filesOwnedByChangeOwnerInTheLastPatchSet = + filterFilesOwnedBy(ownerId, allFilesTouchedInTheLastPatchSet, project, branch); + + if (!allowsAutoApprovalOnPatch( + ownerId, + ownerId, + revision.getPatchSet().uploader(), + filesOwnedByChangeOwnerInTheLastPatchSet, + allFilesTouchedInTheLastPatchSet, + this, + project, + branch)) { + return Set.of(); + } + + return getAutoApprovedFiles(filesApprovedByOwners, approvalsByAccount); + } + + private Set<String> getAutoApprovedFiles( + Map<String, Set<GroupOwner>> filesApprovedByOwners, + Map<Account.Id, List<PatchSetApproval>> currentApprovalsByAccount) { + return filesApprovedByOwners.keySet().stream() + .filter(notExplicitlyApprovedByAnOwner(filesApprovedByOwners, currentApprovalsByAccount)) + .collect(Collectors.toSet()); + } + + private Predicate<String> notExplicitlyApprovedByAnOwner( + Map<String, Set<GroupOwner>> fileExpandedOwners, + Map<Account.Id, List<PatchSetApproval>> currentApprovalsByAccount) { + return filePath -> + ownerIds(fileExpandedOwners.get(filePath)) + .map(currentApprovalsByAccount::get) + .filter(Objects::nonNull) + .flatMap(List::stream) + .noneMatch(isNotCopiedApproval()); + } + + private PatchSet getPreviousPatchSet(RevisionResource revision) { + int sourcePatchSetNumber = revision.getPatchSet().id().get() - 1; + if (sourcePatchSetNumber < 1) { + return null; + } + + return revision + .getNotes() + .getPatchSets() + .get(PatchSet.id(revision.getChange().getId(), sourcePatchSetNumber)); + } + + private static Predicate<PatchSetApproval> isNotCopiedApproval() { + Predicate<PatchSetApproval> func = PatchSetApproval::copied; + return func.negate(); + } + + private Stream<Account.Id> ownerIds(Set<GroupOwner> fileOwners) { + return fileOwners.stream() + .filter(owner -> owner instanceof Owner) + .map(owner -> Account.id(((Owner) owner).getId())); + } + private Stream<Integer> codeReviewLabelValue( Map<Integer, Map<String, Integer>> ownersLabels, int ownerId, String labelId) { return Stream.ofNullable(ownersLabels.get(ownerId))
diff --git a/owners/src/main/resources/Documentation/rest-api.md b/owners/src/main/resources/Documentation/rest-api.md index 3505d6c..6aca821 100644 --- a/owners/src/main/resources/Documentation/rest-api.md +++ b/owners/src/main/resources/Documentation/rest-api.md
@@ -1,8 +1,9 @@ # Rest API The @PLUGIN@ exposes a Rest API endpoint to list the owners associated with each file that -needs approval (`file` field), is approved (`files_approved`) and, for each owner, -its current labels and votes (`owners_labels`): +needs approval (`files`), is approved (`files_approved`), is approved through +`auto-owners-approved` (`files_auto_approved`) and, for each owner, its current labels and +votes (`owners_labels`): ```bash GET /changes/{change-id}/revisions/{revision-id}/owners~files-owners @@ -25,6 +26,12 @@ { "name":"Release Engineer", "id": 1000001 } ] }, + "files_auto_approved": { + "Repository.java":[ + { "name":"John", "id": 1000004 }, + { "name":"Release Engineer", "id": 1000001 } + ] + }, "owners_labels" : { "1000002": { "Verified": 1, @@ -38,5 +45,13 @@ ``` +`files_auto_approved` contains the files whose approval on the current patch set comes from a vote +that was copied forward because the `auto-owners-approved` logic applies. + +`files_auto_approved` and `files_approved` are mutually exclusive. A file that is auto-approved is +returned only in `files_auto_approved`, and both sections return the full owner set for each file. +If a file also has a sufficient explicit owner vote on the current patch set, it is treated as +explicitly approved and returned only in `files_approved`. + > __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/GetFilesOwnersITAbstract.java b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java index ad46da6..e8b3919 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/restapi/GetFilesOwnersITAbstract.java
@@ -16,7 +16,10 @@ package com.googlesource.gerrit.owners.restapi; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import static com.googlesource.gerrit.owners.AlreadyApprovedByOperand.FULL_OPERAND_WITH_PLUGIN_NAME; import com.google.common.collect.Sets; import com.google.gerrit.acceptance.GitUtil; @@ -25,11 +28,16 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.UseLocalDisk; import com.google.gerrit.acceptance.config.GlobalPluginConfig; +import com.google.gerrit.acceptance.testsuite.change.ChangeOperations; +import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; +import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +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.entities.Project.NameKey; import com.google.gerrit.entities.RefNames; +import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -37,13 +45,17 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.server.project.testing.TestLabels; +import com.google.inject.Inject; import com.googlesource.gerrit.owners.common.InvalidOwnersFileException; import com.googlesource.gerrit.owners.common.LabelDefinition; import com.googlesource.gerrit.owners.entities.FilesOwnersResponse; import com.googlesource.gerrit.owners.entities.GroupOwner; import com.googlesource.gerrit.owners.entities.Owner; import com.googlesource.gerrit.owners.restapi.GetFilesOwners.LabelNotFoundException; +import java.util.Arrays; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import javax.servlet.http.HttpServletResponse; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; @@ -53,6 +65,12 @@ public abstract class GetFilesOwnersITAbstract extends LightweightPluginDaemonTest { private static final String REFS_META_CONFIG = RefNames.REFS_META + "config"; + private static final String UNOWNED_TXT_FILE = "a.txt"; + private static final String OWNED_JAVA_FILE = "foo.java"; + private static final Set<GroupOwner> NO_AUTO_APPROVED_OWNERS = Set.of(); + @Inject protected ProjectOperations projectOperations; + @Inject protected RequestScopeOperations requestScopeOperations; + @Inject protected ChangeOperations changeOperations; protected GetFilesOwners ownersApi; private Owner rootOwner; private Owner projectOwner; @@ -142,6 +160,133 @@ } @Test + public void shouldReturnFilesAutoApprovedWhenOwnerVoteIsCopied() throws Exception { + setupAutoApprovalFor(admin); + + Change.Id changeId = createChangeWithCopiedOwnerVote(admin); + + assertFilesApproval( + changeId.toString(), OWNED_JAVA_FILE, NO_AUTO_APPROVED_OWNERS, owners(admin)); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenOwnerExplicitlyVotesOnCurrentPatchSet() + throws Exception { + setupAutoApprovalFor(admin); + + Change.Id changeId = createChangeWithCopiedOwnerVote(admin); + vote(admin, changeId.toString(), 2); + + assertFilesApproval( + changeId.toString(), OWNED_JAVA_FILE, owners(admin), NO_AUTO_APPROVED_OWNERS); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenAnotherOwnerExplicitlyVotesOnCurrentPatchSet() + throws Exception { + TestAccount explicitVoteOwner = accountCreator.create("user-backend"); + allowCodeReviewForRegisteredUsers(); + setupAutoApprovalFor(admin, explicitVoteOwner); + + Change.Id changeId = createChangeWithCopiedOwnerVote(admin); + vote(explicitVoteOwner, changeId.toString(), 2); + + assertFilesApproval( + changeId.toString(), + OWNED_JAVA_FILE, + owners(admin, explicitVoteOwner), + NO_AUTO_APPROVED_OWNERS); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenOwnedAndUnownedFilesAreModifiedTogether() + throws Exception { + setupAutoApprovalForJavaMatcher(admin); + + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(admin.id()) + .file(OWNED_JAVA_FILE) + .content("v1") + .create(); + vote(admin, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(admin.id()) + .file(OWNED_JAVA_FILE) + .content("v2") + .file(UNOWNED_TXT_FILE) + .content("unowned") + .create(); + + Response<FilesOwnersResponse> response = + assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId.toString()))); + assertThat(response.value().files).containsExactly(OWNED_JAVA_FILE, owners(admin)); + assertThat(response.value().filesApproved).isEmpty(); + assertThat(response.value().filesAutoApproved).isEmpty(); + } + + @Test + public void shouldNotReturnFilesAutoApprovedWhenOnlyUnownedFileIsAdded() throws Exception { + setupAutoApprovalForJavaMatcher(admin); + + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(admin.id()) + .file(OWNED_JAVA_FILE) + .content("v1") + .create(); + vote(admin, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(admin.id()) + .file(UNOWNED_TXT_FILE) + .content("unowned") + .create(); + + assertFilesApproval( + changeId.toString(), OWNED_JAVA_FILE, owners(admin), NO_AUTO_APPROVED_OWNERS); + } + + @Test + public void shouldReturnFilesAutoApprovedWhenNextPatchSetAddsOnlyOwnedFile() throws Exception { + setupAutoApprovalForJavaMatcher(admin); + String ANOTHER_OWNED_JAVA_FILE = "another-" + OWNED_JAVA_FILE; + + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(admin.id()) + .file(OWNED_JAVA_FILE) + .content("owned") + .file(UNOWNED_TXT_FILE) + .content("unowned") + .create(); + vote(admin, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(admin.id()) + .file(ANOTHER_OWNED_JAVA_FILE) + .content("owned") + .create(); + + Response<FilesOwnersResponse> response = + assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId.toString()))); + assertThat(response.value().files).isEmpty(); + assertThat(response.value().filesApproved).isEmpty(); + assertThat(response.value().filesAutoApproved) + .containsExactly(OWNED_JAVA_FILE, owners(admin), ANOTHER_OWNED_JAVA_FILE, owners(admin)); + } + + @Test @GlobalPluginConfig(pluginName = "owners", name = "owners.expandGroups", value = "false") public void shouldReturnResponseWithUnexpandedFileOwners() throws Exception { addOwnerFileToRoot(true); @@ -319,6 +464,79 @@ merge(createChange(testRepo, "master", "Add OWNER file", "OWNERS", "{foo", "")); } + private Change.Id createChangeWithCopiedOwnerVote(TestAccount owner) throws Exception { + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(owner.id()) + .file(OWNED_JAVA_FILE) + .content("v1") + .create(); + vote(owner, changeId.toString(), 2); + changeOperations + .change(changeId) + .newPatchset() + .uploader(owner.id()) + .file(OWNED_JAVA_FILE) + .content("v2") + .create(); + return changeId; + } + + private void setupAutoApprovalFor(TestAccount... owners) throws Exception { + String ownersYaml = + Arrays.stream(owners) + .map(owner -> String.format("- %s\n", owner.username())) + .collect(Collectors.joining()); + setupAutoApproval( + String.format("inherited: true\nauto-owners-approved: true\nowners:\n%s", ownersYaml)); + } + + private void setupAutoApprovalForJavaMatcher(TestAccount owner) throws Exception { + setupAutoApproval( + String.format( + "inherited: true\nmatchers:\n" + + "- suffix: .java\n" + + " auto-owners-approved: true\n" + + " owners:\n" + + " - %s\n", + owner.username())); + } + + private void setupAutoApproval(String ownersContent) throws Exception { + updateLabel(b -> b.setCopyCondition("approverin:" + FULL_OPERAND_WITH_PLUGIN_NAME)); + merge(createChange(testRepo, "master", "Add OWNER file", "OWNERS", ownersContent, "")); + } + + private void assertFilesApproval( + String changeId, + String filePath, + java.util.Set<GroupOwner> explicitlyApprovedOwners, + java.util.Set<GroupOwner> autoApprovedOwners) + throws Exception { + Response<FilesOwnersResponse> response = + assertResponseOk(ownersApi.apply(parseCurrentRevisionResource(changeId))); + assertThat(response.value().files).isEmpty(); + if (explicitlyApprovedOwners.isEmpty()) { + assertThat(response.value().filesApproved).isEmpty(); + } else { + assertThat(response.value().filesApproved) + .containsExactly(filePath, explicitlyApprovedOwners); + } + if (autoApprovedOwners.isEmpty()) { + assertThat(response.value().filesAutoApproved).isEmpty(); + } else { + assertThat(response.value().filesAutoApproved).containsExactly(filePath, autoApprovedOwners); + } + } + + private Set<GroupOwner> owners(TestAccount... accounts) { + return java.util.Arrays.stream(accounts) + .map(account -> new Owner(account.fullName(), account.id().get())) + .collect(Collectors.toSet()); + } + private void addOwnerFileToProjectConfig(Project.NameKey projectNameKey, boolean inherit) throws Exception { addOwnerFileToProjectConfig(projectNameKey, inherit, user); @@ -422,4 +640,27 @@ } return clonedProject; } + + private void vote(TestAccount user, String changeId, int vote) throws Exception { + requestScopeOperations.setApiUser(user.id()); + gApi.changes() + .id(changeId) + .current() + .review(new ReviewInput().label(LabelId.CODE_REVIEW, vote)); + } + + private void updateLabel(java.util.function.Consumer<LabelType.Builder> update) throws Exception { + try (ProjectConfigUpdate u = updateProject(allProjects)) { + u.getConfig().updateLabelType(LabelId.CODE_REVIEW, update); + u.save(); + } + } + + private void allowCodeReviewForRegisteredUsers() throws Exception { + projectOperations + .project(allProjects) + .forUpdate() + .add(allowLabel(LabelId.CODE_REVIEW).ref("refs/*").group(REGISTERED_USERS).range(-2, 2)) + .update(); + } }