Fix transitive per-file imports
Transitive per-file imports were imported regardless of whether the path
expressions of the per-file rules matched.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Idd0982e5c95408e34e3c17754a6df1cf3b912278
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 4f67254..036c298 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -21,6 +21,7 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
@@ -344,10 +345,11 @@
.forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet);
}
+ ImmutableSet<CodeOwnerSet> matchingPerFileCodeOwnerSets =
+ getMatchingPerFileCodeOwnerSets(importedCodeOwnerConfig).collect(toImmutableSet());
if (importMode.importPerFileCodeOwnerSets()) {
logger.atFine().log("import per-file code owners");
- getMatchingPerFileCodeOwnerSets(importedCodeOwnerConfig)
- .forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet);
+ matchingPerFileCodeOwnerSets.forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet);
}
if (importMode.resolveImportsOfImport()
@@ -356,7 +358,7 @@
Set<CodeOwnerConfigReference> transitiveImports = new HashSet<>();
transitiveImports.addAll(importedCodeOwnerConfig.imports());
transitiveImports.addAll(
- importedCodeOwnerConfig.codeOwnerSets().stream()
+ matchingPerFileCodeOwnerSets.stream()
.flatMap(codeOwnerSet -> codeOwnerSet.imports().stream())
.collect(toImmutableSet()));
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index 066fcbb..341b3a3 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -1661,6 +1661,96 @@
}
@Test
+ public void onlyMatchingTransitivePerFileImportsAreImported() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // create importing config with global import
+ CodeOwnerConfig.Key rootCodeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addImport(
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/bar/OWNERS"))
+ .create();
+
+ // create imported config with 2 per file imports, one for *.md files and one for *.txt
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/bar/")
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("*.md")
+ .addImport(
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/md/OWNERS"))
+ .build())
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression("*.txt")
+ .addImport(
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/txt/OWNERS"))
+ .build())
+ .create();
+
+ // create config with global code owner that is imported by the imported config for *.md files
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/md/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // create config with global code owner that is imported by the imported config for *.txt files
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/txt/")
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ // Expectation for foo.xyz file: code owners is empty since foo.xyz neither matches *.md nor
+ // *.txt
+ Optional<PathCodeOwners> pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ rootCodeOwnerConfigKey,
+ projectOperations.project(project).getHead("master"),
+ Paths.get("/foo.xyz"));
+ assertThat(pathCodeOwners).isPresent();
+ assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners()).isEmpty();
+
+ // Expectation for foo.md file: code owners contains only user since foo.md only matches *.md
+ pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ rootCodeOwnerConfigKey,
+ projectOperations.project(project).getHead("master"),
+ Paths.get("/foo.md"));
+ assertThat(pathCodeOwners).isPresent();
+ assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(user.email());
+
+ // Expectation for foo.txt file: code owners contains only user2 since foo.txt only matches
+ // *.txt
+ pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ rootCodeOwnerConfigKey,
+ projectOperations.project(project).getHead("master"),
+ Paths.get("/foo.txt"));
+ assertThat(pathCodeOwners).isPresent();
+ assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().getPathCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(user2.email());
+ }
+
+ @Test
public void cannotMatchAgainstNullCodeOwnerSet() throws Exception {
NullPointerException npe =
assertThrows(
