Merge changes I8c557145,Iee7c9372,I853673d6,I273fb5d2,I83430861, ... * changes: OnCodeOwnerApproval: Detect if approval is ignored due to self approval OnCodeOwnerOverride: Detect if override is ignored due to self approval Add debug logs when override approval is ignored due to self approval Remove unused methods from Subject classes OnCodeOwnerApprovalIT: Reformat class javadoc Add test for TestPathExpressions TestPathExpressions: Match file types in all subfolders
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java b/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java index 91867cb..d2acaed 100644 --- a/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java +++ b/java/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressions.java
@@ -41,16 +41,20 @@ } /** - * Creates a path expression that matches all files of the given file type in the current folder. + * Creates a path expression that matches all files of the given file type in the current folder + * and all subfolders. * * @param fileType the file type */ - public String matchFileTypeInCurrentFolder(String fileType) { + public String matchFileType(String fileType) { PathExpressionMatcher pathExpressionMatcher = getPathExpressionMatcher(); - if (pathExpressionMatcher instanceof GlobMatcher - || pathExpressionMatcher instanceof FindOwnersGlobMatcher - || pathExpressionMatcher instanceof SimplePathExpressionMatcher) { + if (pathExpressionMatcher instanceof GlobMatcher) { + return "**." + fileType; + } else if (pathExpressionMatcher instanceof FindOwnersGlobMatcher) { return "*." + fileType; + } else if (pathExpressionMatcher instanceof SimplePathExpressionMatcher) { + // '...' (matches any string, including slashes), followed by '.<file-type>' + return "...." + fileType; } throw new IllegalStateException( String.format(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index 937a118..33fd2e4 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -47,6 +47,7 @@ import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; +import com.google.gerrit.server.util.LabelVote; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -222,7 +223,15 @@ codeOwnersPluginConfiguration.getOverrideApproval(changeNotes.getProjectName()); boolean hasOverride = hasOverride(overrideApprovals, changeNotes, patchSetUploader); logger.atFine().log( - "hasOverride = %s (overrideApprovals = %s)", hasOverride, overrideApprovals); + "hasOverride = %s (overrideApprovals = %s)", + hasOverride, + overrideApprovals.stream() + .map( + overrideApproval -> + String.format( + "%s (ignoreSelfApproval = %s)", + overrideApproval, overrideApproval.labelType().isIgnoreSelfApproval())) + .collect(toImmutableList())); BranchNameKey branch = changeNotes.getChange().getDest(); ObjectId revision = getDestBranchRevision(changeNotes.getChange()); @@ -934,6 +943,9 @@ .labelType() .getLabelId() .equals(approval.key().labelId()))) { + logger.atFine().log( + "Filtered out self-override %s of patch set uploader", + LabelVote.create(approval.label(), approval.value())); return false; } return true;
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java index e818163..dff2876 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -124,6 +124,18 @@ return Optional.empty(); } + if (isIgnoredDueToSelfApproval(user, patchSet, requiredApproval)) { + if (isCodeOwnerApprovalNewlyApplied(requiredApproval, oldApprovals, newVote) + || isCodeOwnerApprovalUpOrDowngraded(requiredApproval, oldApprovals, newVote)) { + return Optional.of( + String.format( + "The vote %s is ignored as code-owner approval since the label doesn't allow" + + " self approval of the patch set uploader.", + newVote)); + } + return Optional.empty(); + } + boolean hasImplicitApprovalByUser = codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(changeNotes.getProjectName()) && patchSet.uploader().equals(user.getAccountId()); @@ -222,6 +234,12 @@ pathsToAppend.forEach(path -> message.append(String.format("* %s\n", JgitPath.of(path).get()))); } + private boolean isIgnoredDueToSelfApproval( + IdentifiedUser user, PatchSet patchSet, RequiredApproval requiredApproval) { + return patchSet.uploader().equals(user.getAccountId()) + && requiredApproval.labelType().isIgnoreSelfApproval(); + } + private boolean isCodeOwnerApprovalNewlyApplied( RequiredApproval requiredApproval, Map<String, Short> oldApprovals, LabelVote newVote) { String labelName = requiredApproval.labelType().getName();
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java index df24fbb..c200ae0 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerOverride.java
@@ -98,6 +98,18 @@ RequiredApproval overrideApproval) { LabelVote newVote = getNewVote(overrideApproval, approvals); + if (isIgnoredDueToSelfApproval(user, patchSet, overrideApproval)) { + if (isCodeOwnerOverrideNewlyApplied(overrideApproval, oldApprovals, newVote) + || isCodeOwnerOverrideUpOrDowngraded(overrideApproval, oldApprovals, newVote)) { + return Optional.of( + String.format( + "The vote %s is ignored as code-owner override since the label doesn't allow" + + " self approval of the patch set uploader.", + newVote)); + } + return Optional.empty(); + } + if (isCodeOwnerOverrideNewlyApplied(overrideApproval, oldApprovals, newVote)) { return Optional.of( String.format( @@ -134,6 +146,12 @@ return Optional.empty(); } + private boolean isIgnoredDueToSelfApproval( + IdentifiedUser user, PatchSet patchSet, RequiredApproval requiredApproval) { + return patchSet.uploader().equals(user.getAccountId()) + && requiredApproval.labelType().isIgnoreSelfApproval(); + } + private boolean isCodeOwnerOverrideNewlyApplied( RequiredApproval requiredApproval, Map<String, Short> oldApprovals, LabelVote newVote) { String labelName = requiredApproval.labelType().getName();
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerInfoSubject.java index 29df00b..33a19aa 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerInfoSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerInfoSubject.java
@@ -14,8 +14,6 @@ package com.google.gerrit.plugins.codeowners.testing; -import static com.google.common.truth.Truth.assertAbout; - import com.google.common.truth.ComparableSubject; import com.google.common.truth.Correspondence; import com.google.common.truth.FailureMetadata; @@ -23,9 +21,7 @@ import com.google.common.truth.Subject; import com.google.gerrit.entities.Account; import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo; -import com.google.gerrit.truth.ListSubject; import com.google.gerrit.truth.NullAwareCorrespondence; -import java.util.List; /** {@link Subject} for doing assertions on {@link CodeOwnerInfo}s. */ public class CodeOwnerInfoSubject extends Subject { @@ -44,21 +40,6 @@ codeOwnerInfo -> codeOwnerInfo.account.name, "has account name"); } - /** - * Starts fluent chain to do assertions on a {@link CodeOwnerInfo}. - * - * @param codeOwnerInfo the code owner info on which assertions should be done - * @return the created {@link CodeOwnerInfoSubject} - */ - public static CodeOwnerInfoSubject assertThat(CodeOwnerInfo codeOwnerInfo) { - return assertAbout(codeOwnerInfos()).that(codeOwnerInfo); - } - - public static ListSubject<CodeOwnerInfoSubject, CodeOwnerInfo> assertThatList( - List<CodeOwnerInfo> codeOwnerInfos) { - return ListSubject.assertThat(codeOwnerInfos, codeOwnerInfos()); - } - public static Factory<CodeOwnerInfoSubject, CodeOwnerInfo> codeOwnerInfos() { return CodeOwnerInfoSubject::new; }
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSubject.java index 49852bb..5e46c2c 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerSubject.java
@@ -14,32 +14,19 @@ package com.google.gerrit.plugins.codeowners.testing; -import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.truth.Truth.assertAbout; import com.google.common.truth.ComparableSubject; -import com.google.common.truth.Correspondence; import com.google.common.truth.FailureMetadata; import com.google.common.truth.Subject; import com.google.gerrit.entities.Account; import com.google.gerrit.plugins.codeowners.backend.CodeOwner; -import com.google.gerrit.truth.ListSubject; -import com.google.gerrit.truth.NullAwareCorrespondence; import com.google.gerrit.truth.OptionalSubject; import java.util.Optional; -import java.util.stream.Stream; /** {@link Subject} for doing assertions on {@link CodeOwner}s. */ public class CodeOwnerSubject extends Subject { /** - * Constructs a {@link Correspondence} that maps {@link CodeOwner}s to {@link - * com.google.gerrit.entities.Account.Id}s. - */ - public static final Correspondence<CodeOwner, Account.Id> hasAccountId() { - return NullAwareCorrespondence.transforming(CodeOwner::accountId, "has account ID"); - } - - /** * Starts fluent chain to do assertions on a {@link CodeOwner}. * * @param codeOwner the code owner on which assertions should be done @@ -60,16 +47,6 @@ return OptionalSubject.assertThat(codeOwner, codeOwners()); } - /** - * Starts fluent chain to do assertions on a stream of {@link CodeOwner}s. - * - * @param codeOwners stream of code owners on which assertions should be done - * @return the created {@link ListSubject} - */ - public static ListSubject<CodeOwnerSubject, CodeOwner> assertThat(Stream<CodeOwner> codeOwners) { - return ListSubject.assertThat(codeOwners.collect(toImmutableList()), codeOwners()); - } - /** Creates subject factory for mapping {@link CodeOwner}s to {@link CodeOwnerSubject}s. */ private static Subject.Factory<CodeOwnerSubject, CodeOwner> codeOwners() { return CodeOwnerSubject::new;
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java index a0a9082..8d46152 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/OptionalResultWithMessagesSubject.java
@@ -53,10 +53,6 @@ this.optionalResultWithMessages = optionalResultWithMessages; } - public void isPresent() { - check("result()").about(optionals()).that(optionalResultWithMessages().result()).isPresent(); - } - public void isEmpty() { check("result()").about(optionals()).that(optionalResultWithMessages().result()).isEmpty(); }
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/PathCodeOwnerStatusSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/PathCodeOwnerStatusSubject.java index a99c0e6..5e2f58c 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/PathCodeOwnerStatusSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/PathCodeOwnerStatusSubject.java
@@ -15,7 +15,6 @@ package com.google.gerrit.plugins.codeowners.testing; import static com.google.common.truth.PathSubject.paths; -import static com.google.common.truth.Truth.assertAbout; import com.google.common.truth.ComparableSubject; import com.google.common.truth.FailureMetadata; @@ -27,16 +26,6 @@ /** {@link Subject} for doing assertions on {@link PathCodeOwnerStatus}s. */ public class PathCodeOwnerStatusSubject extends Subject { /** - * Starts fluent chain to do assertions on a {@link PathCodeOwnerStatus}. - * - * @param pathCodeOwnerStatus the {@link PathCodeOwnerStatus} on which assertions should be done - * @return the created {@link PathCodeOwnerStatusSubject} - */ - public static PathCodeOwnerStatusSubject assertThat(PathCodeOwnerStatus pathCodeOwnerStatus) { - return assertAbout(pathCodeOwnerStatuses()).that(pathCodeOwnerStatus); - } - - /** * Creates subject factory for mapping {@link PathCodeOwnerStatus}es to {@link * PathCodeOwnerStatusSubject}s. */
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java index e869632..9e17d9a 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/RequiredApprovalSubject.java
@@ -24,8 +24,6 @@ import com.google.common.truth.Subject; import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval; import com.google.gerrit.truth.ListSubject; -import com.google.gerrit.truth.OptionalSubject; -import java.util.Optional; /** {@link Subject} for doing assertions on {@link RequiredApproval}s. */ public class RequiredApprovalSubject extends Subject { @@ -40,17 +38,6 @@ } /** - * Starts a fluent chain to do assertions on an {@link Optional} {@link RequiredApproval}. - * - * @param requiredApproval optional required approval on which assertions should be done - * @return the created {@link OptionalSubject} - */ - public static OptionalSubject<RequiredApprovalSubject, RequiredApproval> assertThat( - Optional<RequiredApproval> requiredApproval) { - return OptionalSubject.assertThat(requiredApproval, requiredApprovals()); - } - - /** * Starts a fluent chain to do assertions on a list of {@link RequiredApproval}s. * * @param requiredApprovals list of required approvals on which assertions should be done
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/SubmitRecordSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/SubmitRecordSubject.java index 301fc1d..7b45e8b 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/SubmitRecordSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/SubmitRecordSubject.java
@@ -14,7 +14,6 @@ package com.google.gerrit.plugins.codeowners.testing; -import static com.google.common.truth.Truth.assertAbout; import static com.google.gerrit.plugins.codeowners.testing.SubmitRequirementSubject.submitRecordRequirements; import static com.google.gerrit.truth.ListSubject.elements; @@ -30,16 +29,6 @@ /** {@link Subject} for doing assertions on {@link SubmitRecord}s. */ public class SubmitRecordSubject extends Subject { /** - * Starts fluent chain to do assertions on a {@link SubmitRecord}. - * - * @param submitRecord the {@link SubmitRecord} on which assertions should be done - * @return the created {@link SubmitRecordSubject} - */ - public static SubmitRecordSubject assertThat(SubmitRecord submitRecord) { - return assertAbout(submitRecords()).that(submitRecord); - } - - /** * Starts fluent chain to do assertions on an {@link Optional} {@link SubmitRecord}. * * @param submitRecord the {@link SubmitRecord} {@link Optional} on which assertions should be
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 d7d3da3..0826b9e 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -226,12 +226,12 @@ .folderPath("/foo/bar/") .addCodeOwnerSet( CodeOwnerSet.builder() - .addPathExpression(testPathExpressions.matchFileTypeInCurrentFolder("txt")) + .addPathExpression(testPathExpressions.matchFileType("txt")) .addCodeOwnerEmail(admin.email()) .build()) .addCodeOwnerSet( CodeOwnerSet.builder() - .addPathExpression(testPathExpressions.matchFileTypeInCurrentFolder("md")) + .addPathExpression(testPathExpressions.matchFileType("md")) .addCodeOwnerEmail(user.email()) .build()) .create();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java index 97f426a..415dcc6 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -37,9 +37,7 @@ import java.util.regex.Pattern; import org.junit.Test; -/** - * Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.OnCodeOwnerApproval}. - */ +/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.OnCodeOwnerApproval}. */ public class OnCodeOwnerApprovalIT extends AbstractCodeOwnersIT { @Inject private RequestScopeOperations requestScopeOperations; @Inject private ProjectOperations projectOperations; @@ -947,4 +945,115 @@ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review+1"); } + + @Test + public void changeMessageExtendedIfCodeOwnerApprovalIsIgnoredDueToSelfApproval() + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + + String changeId = createChange().getChangeId(); + + recommend(changeId); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + "Patch Set 1: Code-Review+1\n\n" + + "The vote Code-Review+1 is ignored as code-owner approval since the label" + + " doesn't allow self approval of the patch set uploader.\n"); + } + + @Test + public void changeMessageExtendedIfUpgradedCodeOwnerApprovalIsIgnoredDueToSelfApproval() + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + + String changeId = createChange().getChangeId(); + + recommend(changeId); + + // Upgrade the approval from Code-Review+1 to Code-Review+2 + approve(changeId); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + "Patch Set 1: Code-Review+2\n\n" + + "The vote Code-Review+2 is ignored as code-owner approval since the label" + + " doesn't allow self approval of the patch set uploader.\n"); + } + + @Test + public void changeMessageExtendedIfDowngradedCodeOwnerApprovalIsIgnoredDueToSelfApproval() + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + + String changeId = createChange().getChangeId(); + + approve(changeId); + + // Downgrade the approval from Code-Review+2 to Code-Review+1 + recommend(changeId); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + "Patch Set 1: Code-Review+1\n\n" + + "The vote Code-Review+1 is ignored as code-owner approval since the label" + + " doesn't allow self approval of the patch set uploader.\n"); + } + + @Test + public void changeMessageNotExtendedIfIgnoredCodeOwnerApprovalIsRemoved() throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + + String changeId = createChange().getChangeId(); + + recommend(changeId); + + // Remove the code-owner approval + gApi.changes().id(changeId).current().review(new ReviewInput().label("Code-Review", 0)); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: -Code-Review"); + } + + @Test + public void changeMessageExtendedIfNonSelfApprovalCodeOwnerApprovalIsApplied() throws Exception { + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/foo/") + .addCodeOwnerEmail(user.email()) + .create(); + + LabelDefinitionInput input = new LabelDefinitionInput(); + input.ignoreSelfApproval = true; + gApi.projects().name(allProjects.get()).label("Code-Review").update(input); + + String path = "foo/bar.baz"; + String changeId = createChange("Test Change", path, "file content").getChangeId(); + + requestScopeOperations.setApiUser(user.id()); + recommend(changeId); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + String.format( + "Patch Set 1: Code-Review+1\n\n" + + "By voting Code-Review+1 the following files are now code-owner approved by" + + " %s:\n" + + "* %s\n", + user.fullName(), path)); + } }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java index f094f7b..1628c03 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerOverrrideIT.java
@@ -434,4 +434,174 @@ + "* %s\n", admin.fullName(), path)); } + + @Test + @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1") + public void changeMessageExtendedIfCodeOwnerOverrideIsIgnoredDueToSelfApproval() + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.values = ImmutableMap.of("+1", "Override", " 0", "No Override"); + input.ignoreSelfApproval = true; + gApi.projects().name(project.get()).label("Owners-Override").create(input); + + // Allow to vote on the Owners-Override label. + projectOperations + .project(project) + .forUpdate() + .add( + TestProjectUpdate.allowLabel("Owners-Override") + .range(0, 1) + .ref("refs/*") + .group(REGISTERED_USERS) + .build()) + .update(); + + String changeId = createChange().getChangeId(); + + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + "Patch Set 1: Owners-Override+1\n\n" + + "The vote Owners-Override+1 is ignored as code-owner override since the label" + + " doesn't allow self approval of the patch set uploader.\n"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1") + public void changeMessageExtendedIfUpgradedCodeOwnerOverrideIsIgnoredDueToSelfApproval() + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.values = ImmutableMap.of("+2", "Override", "+1", "Override", " 0", "No Override"); + input.ignoreSelfApproval = true; + gApi.projects().name(project.get()).label("Owners-Override").create(input); + + // Allow to vote on the Owners-Override label. + projectOperations + .project(project) + .forUpdate() + .add( + TestProjectUpdate.allowLabel("Owners-Override") + .range(0, 2) + .ref("refs/*") + .group(REGISTERED_USERS) + .build()) + .update(); + + String changeId = createChange().getChangeId(); + + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + // Upgrade the approval from Owners-Override+1 to Owners-Override+2 + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 2)); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + "Patch Set 1: Owners-Override+2\n\n" + + "The vote Owners-Override+2 is ignored as code-owner override since the label" + + " doesn't allow self approval of the patch set uploader.\n"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1") + public void changeMessageExtendedIfDowngradedCodeOwnerOverrideIsIgnoredDueToSelfApproval() + throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.values = ImmutableMap.of("+2", "Override", "+1", "Override", " 0", "No Override"); + input.ignoreSelfApproval = true; + gApi.projects().name(project.get()).label("Owners-Override").create(input); + + // Allow to vote on the Owners-Override label. + projectOperations + .project(project) + .forUpdate() + .add( + TestProjectUpdate.allowLabel("Owners-Override") + .range(0, 2) + .ref("refs/*") + .group(REGISTERED_USERS) + .build()) + .update(); + + String changeId = createChange().getChangeId(); + + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 2)); + + // Downgrade the approval from Owners-Override+2 to Owners-Override+1 + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + "Patch Set 1: Owners-Override+1\n\n" + + "The vote Owners-Override+1 is ignored as code-owner override since the label" + + " doesn't allow self approval of the patch set uploader.\n"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1") + public void changeMessageNotExtendedIfIgnoredCodeOwnerOverrideIsRemoved() throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.values = ImmutableMap.of("+1", "Override", " 0", "No Override"); + input.ignoreSelfApproval = true; + gApi.projects().name(project.get()).label("Owners-Override").create(input); + + // Allow to vote on the Owners-Override label. + projectOperations + .project(project) + .forUpdate() + .add( + TestProjectUpdate.allowLabel("Owners-Override") + .range(0, 1) + .ref("refs/*") + .group(REGISTERED_USERS) + .build()) + .update(); + + String changeId = createChange().getChangeId(); + + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + // Remove the override approval + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0)); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: -Owners-Override"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1") + public void changeMessageExtendedIfNonSelfApprovalCodeOwnerOverrideIsApplied() throws Exception { + LabelDefinitionInput input = new LabelDefinitionInput(); + input.values = ImmutableMap.of("+1", "Override", " 0", "No Override"); + input.ignoreSelfApproval = true; + gApi.projects().name(project.get()).label("Owners-Override").create(input); + + // Allow to vote on the Owners-Override label. + projectOperations + .project(project) + .forUpdate() + .add( + TestProjectUpdate.allowLabel("Owners-Override") + .range(0, 1) + .ref("refs/*") + .group(REGISTERED_USERS) + .build()) + .update(); + + String changeId = createChange().getChangeId(); + + requestScopeOperations.setApiUser(user.id()); + gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1)); + + Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages; + assertThat(Iterables.getLast(messages).message) + .isEqualTo( + String.format( + "Patch Set 1: Owners-Override+1\n\n" + + "By voting Owners-Override+1 the code-owners submit requirement is overridden by %s\n", + user.fullName())); + } }
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java new file mode 100644 index 0000000..0557da3 --- /dev/null +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/testsuite/TestPathExpressionsTest.java
@@ -0,0 +1,225 @@ +// Copyright (C) 2021 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.plugins.codeowners.acceptance.testsuite; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.testing.GerritJUnit.assertThrows; + +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.entities.Project.NameKey; +import com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.extensions.registration.PrivateInternals_DynamicMapImpl; +import com.google.gerrit.extensions.registration.RegistrationHandle; +import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigUpdate; +import com.google.gerrit.plugins.codeowners.backend.GlobMatcher; +import com.google.gerrit.plugins.codeowners.backend.PathExpressionMatcher; +import com.google.gerrit.server.IdentifiedUser; +import com.google.inject.Key; +import com.google.inject.util.Providers; +import java.nio.file.Path; +import java.util.Optional; +import javax.annotation.Nullable; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.revwalk.RevWalk; +import org.junit.Before; +import org.junit.Test; + +/** Tests for {@link TestPathExpressions}. */ +public class TestPathExpressionsTest extends AbstractCodeOwnersTest { + private TestPathExpressions testPathExpressions; + private DynamicMap<CodeOwnerBackend> codeOwnerBackends; + + @Before + public void setUp() { + testPathExpressions = plugin.getSysInjector().getInstance(TestPathExpressions.class); + codeOwnerBackends = + plugin.getSysInjector().getInstance(new Key<DynamicMap<CodeOwnerBackend>>() {}); + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = "find-owners") + public void matchFileType_findOwnersBackend() throws Exception { + assertThat(testPathExpressions.matchFileType("md")).isEqualTo("*.md"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = "proto") + public void matchFileType_protoBackend() throws Exception { + assertThat(testPathExpressions.matchFileType("md")).isEqualTo("....md"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID) + public void matchFileType_backendWithGlobMatcher() throws Exception { + try (AutoCloseable registration = registerTestBackend(GlobMatcher.INSTANCE)) { + assertThat(testPathExpressions.matchFileType("md")).isEqualTo("**.md"); + } + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID) + public void matchFileType_backendWithUnknownMatcher() throws Exception { + PathExpressionMatcher pathExpressionMatcher = + new PathExpressionMatcher() { + @Override + public boolean matches(String pathExpression, Path relativePath) { + return false; + } + }; + try (AutoCloseable registration = registerTestBackend(pathExpressionMatcher)) { + IllegalStateException exception = + assertThrows(IllegalStateException.class, () -> testPathExpressions.matchFileType("md")); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + String.format( + "path expression matcher %s not supported", + pathExpressionMatcher.getClass().getName())); + } + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID) + public void matchFileType_backendWithoutMatcher() throws Exception { + TestCodeOwnerBackend testCodeOwnerBackend = + new TestCodeOwnerBackend(/* pathExpressionMatcher= */ null); + try (AutoCloseable registration = registerTestBackend(testCodeOwnerBackend)) { + IllegalStateException exception = + assertThrows(IllegalStateException.class, () -> testPathExpressions.matchFileType("md")); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + String.format( + "code owner backend %s doesn't support path expressions", + testCodeOwnerBackend.getClass().getName())); + } + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = "find-owners") + public void matchAllFilesInSubfolder_findOwnersBackend() throws Exception { + assertThat(testPathExpressions.matchAllFilesInSubfolder("foo")).isEqualTo("foo/**"); + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = "proto") + public void matchAllFilesInSubfolder_protoBackend() throws Exception { + assertThat(testPathExpressions.matchAllFilesInSubfolder("foo")).isEqualTo("foo/..."); + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID) + public void matchAllFilesInSubfolder_backendWithGlobMatcher() throws Exception { + try (AutoCloseable registration = registerTestBackend(GlobMatcher.INSTANCE)) { + assertThat(testPathExpressions.matchAllFilesInSubfolder("foo")).isEqualTo("foo/**"); + } + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID) + public void matchAllFilesInSubfolder_backendWithUnknownMatcher() throws Exception { + PathExpressionMatcher pathExpressionMatcher = + new PathExpressionMatcher() { + @Override + public boolean matches(String pathExpression, Path relativePath) { + return false; + } + }; + try (AutoCloseable registration = registerTestBackend(pathExpressionMatcher)) { + IllegalStateException exception = + assertThrows( + IllegalStateException.class, + () -> testPathExpressions.matchAllFilesInSubfolder("foo")); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + String.format( + "path expression matcher %s not supported", + pathExpressionMatcher.getClass().getName())); + } + } + + @Test + @GerritConfig(name = "plugin.code-owners.backend", value = TestCodeOwnerBackend.ID) + public void matchAllFilesInSubfolder_backendWithoutMatcher() throws Exception { + TestCodeOwnerBackend testCodeOwnerBackend = + new TestCodeOwnerBackend(/* pathExpressionMatcher= */ null); + try (AutoCloseable registration = registerTestBackend(testCodeOwnerBackend)) { + IllegalStateException exception = + assertThrows( + IllegalStateException.class, + () -> testPathExpressions.matchAllFilesInSubfolder("foo")); + assertThat(exception) + .hasMessageThat() + .isEqualTo( + String.format( + "code owner backend %s doesn't support path expressions", + testCodeOwnerBackend.getClass().getName())); + } + } + + private AutoCloseable registerTestBackend(PathExpressionMatcher pathExpressionMatcher) { + return registerTestBackend(new TestCodeOwnerBackend(pathExpressionMatcher)); + } + + private AutoCloseable registerTestBackend(CodeOwnerBackend codeOwnerBackend) { + RegistrationHandle registrationHandle = + ((PrivateInternals_DynamicMapImpl<CodeOwnerBackend>) codeOwnerBackends) + .put("gerrit", TestCodeOwnerBackend.ID, Providers.of(codeOwnerBackend)); + return registrationHandle::remove; + } + + private static class TestCodeOwnerBackend implements CodeOwnerBackend { + static final String ID = "test-backend"; + + @Nullable private final PathExpressionMatcher pathExpressionMatcher; + + public TestCodeOwnerBackend(@Nullable PathExpressionMatcher pathExpressionMatcher) { + this.pathExpressionMatcher = pathExpressionMatcher; + } + + @Override + public boolean isCodeOwnerConfigFile(NameKey project, String fileName) { + throw new UnsupportedOperationException(); + } + + @Override + public Optional<CodeOwnerConfig> getCodeOwnerConfig( + CodeOwnerConfig.Key codeOwnerConfigKey, RevWalk revWalk, ObjectId revision) { + throw new UnsupportedOperationException(); + } + + @Override + public Path getFilePath(CodeOwnerConfig.Key codeOwnerConfigKey) { + throw new UnsupportedOperationException(); + } + + @Override + public Optional<CodeOwnerConfig> upsertCodeOwnerConfig( + CodeOwnerConfig.Key codeOwnerConfigKey, + CodeOwnerConfigUpdate codeOwnerConfigUpdate, + IdentifiedUser currentUser) { + throw new UnsupportedOperationException(); + } + + @Override + public Optional<PathExpressionMatcher> getPathExpressionMatcher() { + return Optional.ofNullable(pathExpressionMatcher); + } + } +}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java index a99de99..4df0138 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
@@ -540,7 +540,7 @@ .addCodeOwnerSet( CodeOwnerSet.builder() .setIgnoreGlobalAndParentCodeOwners() - .addPathExpression(testPathExpressions.matchFileTypeInCurrentFolder("txt")) + .addPathExpression(testPathExpressions.matchFileType("txt")) .addCodeOwnerEmail(admin.email()) .build()) .create();