Fix transitive import of global owners via per-file rule that ignores global owners
Example:
OWNERS:
per-file *.md=set noparent
per-file *.md=file:/foo/OWNERS
/foo/OWNERS:
file:/bar/OWNERS
/bar/OWNERS:
code-owner@example.com
For a file that matches the per-file rule in 'OWNERS' we would expect
that 'code-owner@example.com' is a code owner. 'code-owner@example.com'
is a global code owner in '/bar/OWNERS' but since it's imported for a
per-file rule it becomes a per-file code owner. However what happens is
that we wrongly treat 'code-owner@example.com' as a global code owner
and then it gets ignored due to the "set noparent" on the matching
per-file rule.
In PathCodeOwners.resolveImports we already had logic to treat global
code owners that are imported via a per-file rule as per-file code
owners, however this worked only for global code owners that were
directly imported by a per-file rule, but not for global code owners
that were transitively imported by a per-file rule.
This got broken by the refactoring that was done in change I6f7723c0a,
but we didn't notice the breakage because we only tested this case
without transitive imports (see
PathCodeOwnersTest.perFileRuleThatIgnoresGlobalCodeOwnersCanImportGlobalCodeOwnersFromOtherFile).
To fix this we need to know for an import if any previous import in the
import chain was a per-file import. If yes, we should treat the imported
global code owners as per-file code owners.
The check whether an import imports code owners as global code owners or
as per-file code owners is done by
PathCodeOwners.CodeOwnerImport#isGlobal. So far it was only looking at
the current import but not at the previous imports. By passing in the
previous import (if there is one) we can fix this.
Bug: Google b/363000012
Change-Id: I7c872175fb198603f24a95819877ee1ff98651ec
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 585c3e7..3a31808 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -255,12 +255,13 @@
}
// Resolve global imports.
- ImmutableSet<CodeOwnerImport> globalImports = getGlobalImports(0, codeOwnerConfig);
+ ImmutableSet<CodeOwnerImport> globalImports =
+ CodeOwnerImport.createGlobalImports(codeOwnerConfig);
resolveImports(codeOwnerConfig.key(), globalImports, pathCodeOwnersResultBuilder);
// Resolve per-file imports.
ImmutableSet<CodeOwnerImport> perFileImports =
- getPerFileImports(0, codeOwnerConfig, matchingPerFileCodeOwnerSets);
+ CodeOwnerImport.createPerFileImports(codeOwnerConfig, matchingPerFileCodeOwnerSets);
resolveImports(codeOwnerConfig.key(), perFileImports, pathCodeOwnersResultBuilder);
this.pathCodeOwnersResult = pathCodeOwnersResultBuilder.build();
@@ -391,16 +392,16 @@
}
if (importMode.importGlobalCodeOwnerSets()) {
- if (codeOwnerConfigImport.isGlobalImport()) {
- logger.atFine().log("add possibly ignored imported global code owners");
- getGlobalCodeOwnerSets(importedCodeOwnerConfig)
- .forEach(pathCodeOwnersResultBuilder::addGlobalCodeOwnerSet);
- } else {
+ if (codeOwnerConfigImport.importGlobalCodeOwnersAsPerFileCodeOwners()) {
// global code owners which are being imported by a per-file rule become per-file code
// owners
logger.atFine().log("add imported global code owners as per-file code owners");
getGlobalCodeOwnerSets(importedCodeOwnerConfig)
.forEach(pathCodeOwnersResultBuilder::addPerFileCodeOwnerSet);
+ } else {
+ logger.atFine().log("add possibly ignored imported global code owners");
+ getGlobalCodeOwnerSets(importedCodeOwnerConfig)
+ .forEach(pathCodeOwnersResultBuilder::addGlobalCodeOwnerSet);
}
}
@@ -424,12 +425,11 @@
logger.atFine().log("resolve imports of imported code owner config");
Set<CodeOwnerImport> transitiveImports = new HashSet<>();
transitiveImports.addAll(
- getGlobalImports(codeOwnerConfigImport.importLevel() + 1, importedCodeOwnerConfig));
+ CodeOwnerImport.createTransitiveGlobalImports(
+ codeOwnerConfigImport, importedCodeOwnerConfig));
transitiveImports.addAll(
- getPerFileImports(
- codeOwnerConfigImport.importLevel() + 1,
- importedCodeOwnerConfig,
- matchingPerFileCodeOwnerSets));
+ CodeOwnerImport.createTransitivePerFileImports(
+ codeOwnerConfigImport, importedCodeOwnerConfig, matchingPerFileCodeOwnerSets));
if (importMode == CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY) {
// If only global code owners should be imported, transitive imports should also only
@@ -443,7 +443,7 @@
.map(
codeOwnerCfgImport ->
CodeOwnerImport.create(
- codeOwnerCfgImport.importLevel(),
+ codeOwnerCfgImport.prevImport(),
codeOwnerCfgImport.importingCodeOwnerConfig(),
CodeOwnerConfigReference.copyWithNewImportMode(
codeOwnerCfgImport.referenceToImportedCodeOwnerConfig(),
@@ -467,32 +467,6 @@
}
}
- private ImmutableSet<CodeOwnerImport> getGlobalImports(
- int importLevel, CodeOwnerConfig codeOwnerConfig) {
- return codeOwnerConfig.imports().stream()
- .map(
- codeOwnerConfigReference ->
- CodeOwnerImport.create(importLevel, codeOwnerConfig, codeOwnerConfigReference))
- .collect(toImmutableSet());
- }
-
- private ImmutableSet<CodeOwnerImport> getPerFileImports(
- int importLevel, CodeOwnerConfig importingCodeOwnerConfig, Set<CodeOwnerSet> codeOwnerSets) {
- ImmutableSet.Builder<CodeOwnerImport> codeOwnerConfigImports = ImmutableSet.builder();
- for (CodeOwnerSet codeOwnerSet : codeOwnerSets) {
- codeOwnerSet.imports().stream()
- .forEach(
- codeOwnerConfigReference ->
- codeOwnerConfigImports.add(
- CodeOwnerImport.create(
- importLevel,
- importingCodeOwnerConfig,
- codeOwnerConfigReference,
- codeOwnerSet)));
- }
- return codeOwnerConfigImports.build();
- }
-
public static CodeOwnerConfig.Key createKeyForImportedCodeOwnerConfig(
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
CodeOwnerConfigReference codeOwnerConfigReference) {
@@ -573,13 +547,8 @@
@AutoValue
abstract static class CodeOwnerImport {
- /**
- * The import level.
- *
- * <p>{@code 0} for direct import, {@code 1} if imported by a directly imported file, {@code 2},
- * if imported by a file that was imported by an directly imported file, etc.
- */
- public abstract int importLevel();
+ /** The import that imported the {@link #importingCodeOwnerConfig()}. */
+ public abstract Optional<CodeOwnerImport> prevImport();
/** The code owner config that contains the import. */
public abstract CodeOwnerConfig importingCodeOwnerConfig();
@@ -590,12 +559,35 @@
/** The code owner set that specified the import, empty if it is a global import. */
public abstract Optional<CodeOwnerSet> codeOwnerSet();
+ /**
+ * The import level.
+ *
+ * <p>{@code 0} for direct import, {@code 1} if imported by a directly imported file, {@code 2},
+ * if imported by a file that was imported by a directly imported file, etc.
+ */
+ int importLevel() {
+ return prevImport().isPresent() ? prevImport().get().importLevel() + 1 : 0;
+ }
+
+ boolean importGlobalCodeOwnersAsPerFileCodeOwners() {
+ return !isGlobalImport()
+ || (!prevImport().isEmpty()
+ && prevImport().get().importGlobalCodeOwnersAsPerFileCodeOwners());
+ }
+
boolean isGlobalImport() {
return codeOwnerSet().isEmpty();
}
public String format() {
if (isGlobalImport()) {
+ if (importGlobalCodeOwnersAsPerFileCodeOwners()) {
+ return getPrefix()
+ + String.format(
+ "* %s (global import of per-file code owners, import mode = %s)\n",
+ referenceToImportedCodeOwnerConfig().format(),
+ referenceToImportedCodeOwnerConfig().importMode());
+ }
return getPrefix()
+ String.format(
"* %s (global import, import mode = %s)\n",
@@ -636,33 +628,75 @@
return levels > 0 ? String.format("%" + (levels * 2) + "s", "") : "";
}
- public static CodeOwnerImport create(
- int importLevel,
+ static ImmutableSet<CodeOwnerImport> createGlobalImports(
+ CodeOwnerConfig importingCodeOwnerConfig) {
+ return importingCodeOwnerConfig.imports().stream()
+ .map(
+ codeOwnerConfigReference ->
+ create(
+ Optional.empty(),
+ importingCodeOwnerConfig,
+ codeOwnerConfigReference,
+ Optional.empty()))
+ .collect(toImmutableSet());
+ }
+
+ static ImmutableSet<CodeOwnerImport> createTransitiveGlobalImports(
+ CodeOwnerImport prevCodeOwnerImport, CodeOwnerConfig importingCodeOwnerConfig) {
+ return importingCodeOwnerConfig.imports().stream()
+ .map(
+ codeOwnerConfigReference ->
+ create(
+ Optional.of(prevCodeOwnerImport),
+ importingCodeOwnerConfig,
+ codeOwnerConfigReference,
+ Optional.empty()))
+ .collect(toImmutableSet());
+ }
+
+ static ImmutableSet<CodeOwnerImport> createPerFileImports(
+ CodeOwnerConfig importingCodeOwnerConfig, Set<CodeOwnerSet> codeOwnerSets) {
+ ImmutableSet.Builder<CodeOwnerImport> codeOwnerConfigImports = ImmutableSet.builder();
+ for (CodeOwnerSet codeOwnerSet : codeOwnerSets) {
+ codeOwnerSet.imports().stream()
+ .forEach(
+ codeOwnerConfigReference ->
+ codeOwnerConfigImports.add(
+ create(
+ Optional.empty(),
+ importingCodeOwnerConfig,
+ codeOwnerConfigReference,
+ Optional.of(codeOwnerSet))));
+ }
+ return codeOwnerConfigImports.build();
+ }
+
+ static ImmutableSet<CodeOwnerImport> createTransitivePerFileImports(
+ CodeOwnerImport prevCodeOwnerImport,
CodeOwnerConfig importingCodeOwnerConfig,
- CodeOwnerConfigReference codeOwnerConfigReference) {
- return create(
- importLevel, importingCodeOwnerConfig, codeOwnerConfigReference, Optional.empty());
+ Set<CodeOwnerSet> codeOwnerSets) {
+ ImmutableSet.Builder<CodeOwnerImport> codeOwnerConfigImports = ImmutableSet.builder();
+ for (CodeOwnerSet codeOwnerSet : codeOwnerSets) {
+ codeOwnerSet.imports().stream()
+ .forEach(
+ codeOwnerConfigReference ->
+ codeOwnerConfigImports.add(
+ create(
+ Optional.of(prevCodeOwnerImport),
+ importingCodeOwnerConfig,
+ codeOwnerConfigReference,
+ Optional.of(codeOwnerSet))));
+ }
+ return codeOwnerConfigImports.build();
}
public static CodeOwnerImport create(
- int importLevel,
- CodeOwnerConfig importingCodeOwnerConfig,
- CodeOwnerConfigReference codeOwnerConfigReference,
- CodeOwnerSet codeOwnerSet) {
- return create(
- importLevel,
- importingCodeOwnerConfig,
- codeOwnerConfigReference,
- Optional.of(codeOwnerSet));
- }
-
- public static CodeOwnerImport create(
- int importLevel,
+ Optional<CodeOwnerImport> prevImport,
CodeOwnerConfig importingCodeOwnerConfig,
CodeOwnerConfigReference codeOwnerConfigReference,
Optional<CodeOwnerSet> codeOwnerSet) {
return new AutoValue_PathCodeOwners_CodeOwnerImport(
- importLevel, importingCodeOwnerConfig, codeOwnerConfigReference, codeOwnerSet);
+ prevImport, importingCodeOwnerConfig, codeOwnerConfigReference, codeOwnerSet);
}
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index f0f53c3..b9defba 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
@@ -2614,6 +2614,89 @@
@Test
public void
+ perFileRuleThatIgnoresGlobalCodeOwnersCanTransitivelyImportGlobalCodeOwnersFromOtherFile()
+ throws Exception {
+ // create transitively imported config with a global code owner
+ CodeOwnerConfig.Key keyOfBarCodeOwnerConfig =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/bar/")
+ .fileName("OWNERS")
+ .addCodeOwnerEmail(user.email())
+ .create();
+ CodeOwnerConfigReference barCodeOwnerConfigReference =
+ createCodeOwnerConfigReference(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, keyOfBarCodeOwnerConfig);
+
+ // create imported config the imports the another config
+ CodeOwnerConfig.Key keyOfFooCodeOwnerConfig =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .fileName("OWNERS")
+ .addImport(barCodeOwnerConfigReference)
+ .create();
+ CodeOwnerConfigReference fooCodeOwnerConfigReference =
+ createCodeOwnerConfigReference(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, keyOfFooCodeOwnerConfig);
+
+ // create importing config that:
+ // * has a global code owner
+ // * has a per-file import for md files
+ // * ignores global and parent code owners for md files
+ CodeOwnerConfig.Key keyOfRootCodeOwnerConfig =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .setIgnoreGlobalAndParentCodeOwners()
+ .addImport(fooCodeOwnerConfigReference)
+ .build())
+ .create();
+
+ CodeOwnerConfig rootCodeOwnerConfig =
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfRootCodeOwnerConfig).get();
+ CodeOwnerConfig fooCodeOwnerConfig =
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfFooCodeOwnerConfig).get();
+ CodeOwnerConfig barCodeOwnerConfig =
+ codeOwnerConfigOperations.codeOwnerConfig(keyOfBarCodeOwnerConfig).get();
+
+ Optional<PathCodeOwners> pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
+ keyOfRootCodeOwnerConfig,
+ projectOperations.project(project).getHead("master"),
+ Path.of("/foo.md"));
+ assertThat(pathCodeOwners).isPresent();
+
+ // Expectation: we get the global code owner from the transitively imported code owner config
+ // (since it is imported via a matching per-file rule), the global code owner from the importing
+ // code owner config is ignored (since the matching per-file rule ignores parent and global code
+ // owners)
+ PathCodeOwnersResult pathCodeOwnersResult = pathCodeOwners.get().resolveCodeOwnerConfig();
+ assertThat(pathCodeOwnersResult.getPathCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(user.email());
+ assertThat(pathCodeOwnersResult.resolvedImports())
+ .containsExactly(
+ CodeOwnerConfigImport.createResolvedImport(
+ rootCodeOwnerConfig, fooCodeOwnerConfig, fooCodeOwnerConfigReference),
+ CodeOwnerConfigImport.createResolvedImport(
+ fooCodeOwnerConfig, barCodeOwnerConfig, barCodeOwnerConfigReference));
+ assertThat(pathCodeOwnersResult.unresolvedImports()).isEmpty();
+ }
+
+ @Test
+ public void
perFileRuleThatIsImportedByAGlobalImportIsRespectedIfALocalPerFileRuleIgnoresGlobalCodeOwners()
throws Exception {
TestAccount user2 = accountCreator.user2();
