Support annotations on include and file lines Allow annotations (such as LAST_RESORT_SUGGESTION) on include directives and file: lines. Bug: Google b/223862325 Change-Id: Ib7ffa7223bddda3d65ac157646af7d4c1b74cac1
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java index 45a1c3d..636f098 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigReference.java
@@ -19,11 +19,13 @@ import static java.util.Objects.requireNonNull; import com.google.auto.value.AutoValue; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; import java.nio.file.Path; import java.util.Optional; +import java.util.Set; /** * A reference to a {@link CodeOwnerConfig}. @@ -61,6 +63,9 @@ */ public abstract Path filePath(); + /** Gets the annotations on this code owner config reference. */ + public abstract ImmutableSet<CodeOwnerAnnotation> annotations(); + /** * The path of the folder that contains the code owner config. * @@ -132,7 +137,8 @@ public static Builder builder(CodeOwnerConfigImportMode importMode, Path filePath) { return new AutoValue_CodeOwnerConfigReference.Builder() .setImportMode(importMode) - .setFilePath(filePath); + .setFilePath(filePath) + .setAnnotations(ImmutableSet.of()); } /** Returns a copy of the given code owner config reference with the given import mode. */ @@ -191,6 +197,42 @@ */ abstract Builder setFilePath(Path filePath); + abstract ImmutableSet.Builder<CodeOwnerAnnotation> annotationsBuilder(); + + /** + * Adds an annotation for this code owner config reference. + * + * @param annotation annotation that should be added + * @return the Builder instance for chaining calls + */ + @CanIgnoreReturnValue + public Builder addAnnotation(CodeOwnerAnnotation annotation) { + requireNonNull(annotation, "annotation"); + annotationsBuilder().add(annotation); + return this; + } + + /** + * Adds annotations for this code owner config reference. + * + * @param annotations annotations that should be added + * @return the Builder instance for chaining calls + */ + @CanIgnoreReturnValue + public Builder addAnnotations(Set<CodeOwnerAnnotation> annotations) { + requireNonNull(annotations, "annotations"); + annotations.forEach(this::addAnnotation); + return this; + } + + /** + * Sets the annotations of this code owner config reference. + * + * @param annotations the annotations + * @return the Builder instance for chaining calls + */ + public abstract Builder setAnnotations(ImmutableSet<CodeOwnerAnnotation> annotations); + /** * Builds the {@link CodeOwnerConfigReference} instance without validation. *
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java index 3a31808..c0cdcb9 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -397,10 +397,16 @@ // owners logger.atFine().log("add imported global code owners as per-file code owners"); getGlobalCodeOwnerSets(importedCodeOwnerConfig) + .map( + codeOwnerSet -> + applyAnnotations(codeOwnerSet, codeOwnerConfigImport.annotations())) .forEach(pathCodeOwnersResultBuilder::addPerFileCodeOwnerSet); } else { logger.atFine().log("add possibly ignored imported global code owners"); getGlobalCodeOwnerSets(importedCodeOwnerConfig) + .map( + codeOwnerSet -> + applyAnnotations(codeOwnerSet, codeOwnerConfigImport.annotations())) .forEach(pathCodeOwnersResultBuilder::addGlobalCodeOwnerSet); } } @@ -416,7 +422,8 @@ String.format( "per-file code owner set with path expressions %s matches\n", codeOwnerSet.pathExpressions()))); - pathCodeOwnersResultBuilder.addPerFileCodeOwnerSet(codeOwnerSet); + pathCodeOwnersResultBuilder.addPerFileCodeOwnerSet( + applyAnnotations(codeOwnerSet, codeOwnerConfigImport.annotations())); }); } @@ -545,6 +552,18 @@ .anyMatch(pathExpression -> matcher.matches(pathExpression, relativePath)); } + private static CodeOwnerSet applyAnnotations( + CodeOwnerSet codeOwnerSet, ImmutableSet<CodeOwnerAnnotation> annotations) { + if (annotations.isEmpty() || codeOwnerSet.codeOwners().isEmpty()) { + return codeOwnerSet; + } + CodeOwnerSet.Builder builder = codeOwnerSet.toBuilder(); + for (CodeOwnerReference codeOwner : codeOwnerSet.codeOwners()) { + builder.addAnnotations(codeOwner, annotations); + } + return builder.build(); + } + @AutoValue abstract static class CodeOwnerImport { /** The import that imported the {@link #importingCodeOwnerConfig()}. */ @@ -560,6 +579,21 @@ public abstract Optional<CodeOwnerSet> codeOwnerSet(); /** + * Gets all annotations that should be applied to code owners from this import. + * + * <p>Includes annotations specified on this import reference as well as annotations from parent + * import references. + */ + ImmutableSet<CodeOwnerAnnotation> annotations() { + ImmutableSet.Builder<CodeOwnerAnnotation> annotationsBuilder = ImmutableSet.builder(); + if (prevImport().isPresent()) { + annotationsBuilder.addAll(prevImport().get().annotations()); + } + annotationsBuilder.addAll(referenceToImportedCodeOwnerConfig().annotations()); + return annotationsBuilder.build(); + } + + /** * The import level. * * <p>{@code 0} for direct import, {@code 1} if imported by a directly imported file, {@code 2},
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java index 5f35099..2250fa0 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
@@ -273,16 +273,6 @@ .build(); } - CodeOwnerConfigReference codeOwnerConfigReference; - if ((codeOwnerConfigReference = parseInclude(directive)) != null) { - return CodeOwnerSet.builder() - .addImport(codeOwnerConfigReference) - .setPathExpressions(dirGlobs) - .build(); - } - - List<String> ownerEmails = Arrays.asList(directive.split(COMMA, -1)); - // Get the comment part of the line (the first '#' and everything that follows). String comment = perFileMatcher.group(3); Set<CodeOwnerAnnotation> annotations = new HashSet<>(); @@ -294,6 +284,20 @@ } } + CodeOwnerConfigReference codeOwnerConfigReference; + if ((codeOwnerConfigReference = parseInclude(directive)) != null) { + if (!annotations.isEmpty()) { + codeOwnerConfigReference = + codeOwnerConfigReference.toBuilder().addAnnotations(annotations).build(); + } + return CodeOwnerSet.builder() + .addImport(codeOwnerConfigReference) + .setPathExpressions(dirGlobs) + .build(); + } + + List<String> ownerEmails = Arrays.asList(directive.split(COMMA, -1)); + CodeOwnerSet.Builder codeOwnerSet = CodeOwnerSet.builder() .setPathExpressions(dirGlobs) @@ -415,6 +419,15 @@ } } + String comment = m.group(5); + if (comment != null) { + Matcher annotationMatcher = PAT_ANNOTATION.matcher(comment); + while (annotationMatcher.find()) { + String annotation = annotationMatcher.group(1); + builder.addAnnotation(CodeOwnerAnnotation.create(annotation)); + } + } + return builder.build(); } @@ -620,6 +633,14 @@ // write the file path b.append(codeOwnerConfigReference.filePath()); + if (!codeOwnerConfigReference.annotations().isEmpty()) { + b.append( + formatAnnotations( + codeOwnerConfigReference.annotations().stream() + .map(CodeOwnerAnnotation::key) + .collect(toImmutableSortedSet(naturalOrder())))); + } + return b.toString(); } }
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerConfigReferenceSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerConfigReferenceSubject.java index 7f7706d..f3007db 100644 --- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerConfigReferenceSubject.java +++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerConfigReferenceSubject.java
@@ -14,12 +14,15 @@ package com.google.gerrit.plugins.codeowners.testing; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.gerrit.truth.OptionalSubject.optionals; import com.google.common.truth.FailureMetadata; +import com.google.common.truth.IterableSubject; import com.google.common.truth.PathSubject; import com.google.common.truth.Subject; import com.google.gerrit.entities.Project; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference; import com.google.gerrit.truth.OptionalSubject; @@ -60,6 +63,15 @@ return check("filePath()").that(codeOwnerConfigReference().filePath()); } + /** Returns a subject for the annotations on this code owner config reference. */ + public IterableSubject hasAnnotationsThat() { + return check("annotations()") + .that( + codeOwnerConfigReference().annotations().stream() + .map(CodeOwnerAnnotation::key) + .collect(toImmutableSet())); + } + private CodeOwnerConfigReference codeOwnerConfigReference() { isNotNull(); return codeOwnerConfigReference;
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 b3d0242..18363d1 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInChangeIT.java
@@ -40,6 +40,8 @@ import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo; import com.google.gerrit.plugins.codeowners.backend.CodeOwner; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet; @@ -513,6 +515,40 @@ } @Test + public void codeOwnersWithLastResortSuggestionAnnotation_annotationSetOnInclude() + throws Exception { + skipTestIfAnnotationsNotSupportedByCodeOwnersBackend(); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/bar/") + .addCodeOwnerEmail(user.email()) + .create(); + + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/") + .addCodeOwnerEmail(admin.email()) + .addImport( + CodeOwnerConfigReference.builder(CodeOwnerConfigImportMode.ALL, "/bar/OWNERS") + .addAnnotation(CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION) + .build()) + .create(); + + // Expectation: admin is suggested, user gets filtered out due to the LAST_RESORT_SUGGESTION + // annotation on the include line + CodeOwnersInfo codeOwnersInfo = queryCodeOwners("foo/bar/baz.md"); + assertThat(codeOwnersInfo) + .hasCodeOwnersThat() + .comparingElementsUsing(hasAccountId()) + .containsExactly(admin.id()); + } + + @Test public void perFileCodeOwnersWithLastResortSuggestionAnnotationAreFilteredOut() throws Exception { skipTestIfAnnotationsNotSupportedByCodeOwnersBackend();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java index b9defba..45bc2f1 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -38,6 +38,7 @@ import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest; import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations; import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotations; import com.google.gerrit.server.IdentifiedUser; import com.google.inject.Inject; import com.google.inject.Key; @@ -1533,6 +1534,53 @@ } @Test + public void importWithAnnotations() throws Exception { + // create imported config with global code owner + CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig = + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/bar/") + .fileName("OWNERS") + .addCodeOwnerEmail(user.email()) + .create(); + CodeOwnerConfigReference codeOwnerConfigReference = + createCodeOwnerConfigReference(CodeOwnerConfigImportMode.ALL, keyOfImportedCodeOwnerConfig) + .toBuilder() + .addAnnotation(CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION) + .build(); + + // create importing config with global code owner and annotated import + CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig = + codeOwnerConfigOperations + .newCodeOwnerConfig() + .project(project) + .branch("master") + .folderPath("/") + .fileName("OWNERS") + .addCodeOwnerEmail(admin.email()) + .addImport(codeOwnerConfigReference) + .create(); + + Optional<PathCodeOwners> pathCodeOwners = + pathCodeOwnersFactory.create( + transientCodeOwnerConfigCacheProvider.get(), + keyOfImportingCodeOwnerConfig, + projectOperations.project(project).getHead("master"), + Path.of("/foo.md")); + assertThat(pathCodeOwners).isPresent(); + + PathCodeOwnersResult pathCodeOwnersResult = pathCodeOwners.get().resolveCodeOwnerConfig(); + assertThat(pathCodeOwnersResult.getPathCodeOwners()) + .comparingElementsUsing(hasEmail()) + .containsExactly(admin.email(), user.email()); + assertThat(pathCodeOwnersResult.getAnnotationsFor(user.email())) + .containsExactly(CodeOwnerAnnotations.LAST_RESORT_SUGGESTION_ANNOTATION.key()); + assertThat(pathCodeOwnersResult.getAnnotationsFor(admin.email())).isEmpty(); + } + + @Test public void importsAreResolvedFromSameRevision() throws Exception { TestAccount user2 = accountCreator.user2();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java index 199dfc1..6f250eb 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -26,6 +26,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; import com.google.gerrit.plugins.codeowners.backend.AbstractCodeOwnerConfigParserTest; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerAnnotation; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode; import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigParseException; @@ -36,6 +37,7 @@ import com.google.gerrit.plugins.codeowners.testing.CodeOwnerConfigReferenceSubject; import com.google.gerrit.plugins.codeowners.testing.CodeOwnerSetSubject; import java.nio.file.Path; +import java.util.Set; import java.util.regex.Pattern; import org.junit.Test; @@ -65,7 +67,7 @@ } b.append( String.format( - "%s %s%s%s\n", + "%s %s%s%s%s\n", keyword, codeOwnerConfigReference .project() @@ -73,7 +75,8 @@ .map(projectName -> projectName + ":") .orElse(""), codeOwnerConfigReference.branch().map(branch -> branch + ":").orElse(""), - codeOwnerConfigReference.filePath())); + codeOwnerConfigReference.filePath(), + formatAnnotations(codeOwnerConfigReference.annotations()))); }); // global code owners @@ -102,7 +105,7 @@ for (CodeOwnerConfigReference codeOwnerConfigReference : codeOwnerSet.imports()) { b.append( String.format( - "per-file %s=file: %s%s%s\n", + "per-file %s=file: %s%s%s%s\n", codeOwnerSet.pathExpressions().stream().sorted().collect(joining(",")), codeOwnerConfigReference .project() @@ -110,7 +113,8 @@ .map(projectName -> projectName + ":") .orElse(""), codeOwnerConfigReference.branch().map(branch -> branch + ":").orElse(""), - codeOwnerConfigReference.filePath())); + codeOwnerConfigReference.filePath(), + formatAnnotations(codeOwnerConfigReference.annotations()))); } if (!codeOwnerSet.codeOwners().isEmpty()) { b.append( @@ -127,6 +131,17 @@ return b.toString(); } + private static String formatAnnotations(Set<CodeOwnerAnnotation> annotations) { + if (annotations.isEmpty()) { + return ""; + } + return annotations.stream() + .map(CodeOwnerAnnotation::key) + .sorted() + .map(annotation -> "#{" + annotation + "}") + .collect(joining(" ", " ", "")); + } + @Test public void cannotParseCodeOwnerConfigWithInvalidEmails() throws Exception { CodeOwnerConfigParseException exception = @@ -237,6 +252,58 @@ } @Test + public void importCodeOwnerConfigWithAnnotations() throws Exception { + Path path = Path.of("/foo/bar/OWNERS"); + CodeOwnerConfigReference codeOwnerConfigReference = + CodeOwnerConfigReference.builder(CodeOwnerConfigImportMode.ALL, path) + .addAnnotation(CodeOwnerAnnotation.create("LAST_RESORT_SUGGESTION")) + .addAnnotation(CodeOwnerAnnotation.create("FOO")) + .build(); + assertParseAndFormat( + "include " + path + " #{LAST_RESORT_SUGGESTION} #{FOO} # some comment", + codeOwnerConfig -> { + CodeOwnerConfigReferenceSubject codeOwnerConfigReferenceSubject = + assertThat(codeOwnerConfig).hasImportsThat().onlyElement(); + codeOwnerConfigReferenceSubject.hasProjectThat().isEmpty(); + codeOwnerConfigReferenceSubject.hasBranchThat().isEmpty(); + codeOwnerConfigReferenceSubject.hasFilePathThat().isEqualTo(path); + codeOwnerConfigReferenceSubject + .hasAnnotationsThat() + .containsExactly("LAST_RESORT_SUGGESTION", "FOO"); + }, + getCodeOwnerConfig(codeOwnerConfigReference)); + } + + @Test + public void perFileImportCodeOwnerConfigWithAnnotations() throws Exception { + Path path = Path.of("/foo/bar/OWNERS"); + CodeOwnerConfigReference codeOwnerConfigReference = + CodeOwnerConfigReference.builder( + CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, path) + .addAnnotation(CodeOwnerAnnotation.create("LAST_RESORT_SUGGESTION")) + .build(); + assertParseAndFormat( + "per-file *.md=file: " + path + " #{LAST_RESORT_SUGGESTION} # some comment", + codeOwnerConfig -> { + CodeOwnerSetSubject codeOwnerSetSubject = + assertThat(codeOwnerConfig).hasCodeOwnerSetsThat().onlyElement(); + codeOwnerSetSubject.hasPathExpressionsThat().containsExactly("*.md"); + CodeOwnerConfigReferenceSubject codeOwnerConfigReferenceSubject = + codeOwnerSetSubject.hasImportsThat().onlyElement(); + codeOwnerConfigReferenceSubject.hasFilePathThat().isEqualTo(path); + codeOwnerConfigReferenceSubject + .hasAnnotationsThat() + .containsExactly("LAST_RESORT_SUGGESTION"); + }, + getCodeOwnerConfig( + false, + CodeOwnerSet.builder() + .setPathExpressions(ImmutableSet.of("*.md")) + .addImport(codeOwnerConfigReference) + .build())); + } + + @Test public void codeOwnerConfigWithAnnotations() throws Exception { assertParseAndFormat( getCodeOwnerConfig(
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md index 5e428e5..0d786a2 100644 --- a/resources/Documentation/backend-find-owners.md +++ b/resources/Documentation/backend-find-owners.md
@@ -371,12 +371,9 @@ annotation, this annotation applies to all these users. E.g. if an annotation is set for the all users wildcard (aka `*`) it applies to all users. -**NOTE:** Only [email lines](#userEmails) and [per-file lines](#perFile) that -assign code ownership directly to users support annotations, for other lines -(e.g. [file lines](#fileKeyword), [include lines](#includeKeyword) and -[per-file lines](#perFile) that reference other `OWNERS` files via the -[file](#fileKeyword) keyword) annotations are interpreted as -[comments](#comments) and are silently ignored. +**NOTE:** Annotations on [include lines](#includeKeyword) and [file lines](#fileKeyword) +apply to all imported code owners. This includes code owners imported through +[per-file lines](#perFile) that reference another `OWNERS` file. ### <a id="comments">Comments