Merge "Allow to set a max size for the in-memory code owner config cache"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 91b0f6f..ed65114 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toSet;
@@ -221,37 +222,43 @@
logger.atFine().log(
"resolve code owners for %s from code owner config %s", path, codeOwnerConfig.key());
+ List<String> messages = new ArrayList<>();
+
+ // Create a code owner config builder to create the resolved code owner config (= code owner
+ // config that is scoped to the path and which has imports resolved)
CodeOwnerConfig.Builder resolvedCodeOwnerConfigBuilder =
CodeOwnerConfig.builder(codeOwnerConfig.key(), codeOwnerConfig.revision());
- List<String> messages = new ArrayList<>();
-
- // Add all data from the importing code owner config.
+ // Add all data from the original code owner config that is relevant for the path
+ // (ignoreParentCodeOwners flag, global code owner sets and matching per-file code owner
+ // sets). Effectively this means we are dropping all non-matching per-file rules.
resolvedCodeOwnerConfigBuilder.setIgnoreParentCodeOwners(
codeOwnerConfig.ignoreParentCodeOwners());
getGlobalCodeOwnerSets(codeOwnerConfig)
.forEach(resolvedCodeOwnerConfigBuilder::addCodeOwnerSet);
- getMatchingPerFileCodeOwnerSets(codeOwnerConfig)
- .forEach(
- codeOwnerSet -> {
- messages.add(
- String.format(
- "per-file code owner set with path expressions %s matches",
- codeOwnerSet.pathExpressions()));
- resolvedCodeOwnerConfigBuilder.addCodeOwnerSet(codeOwnerSet);
- });
+ for (CodeOwnerSet codeOwnerSet :
+ getMatchingPerFileCodeOwnerSets(codeOwnerConfig).collect(toImmutableSet())) {
+ messages.add(
+ String.format(
+ "per-file code owner set with path expressions %s matches",
+ codeOwnerSet.pathExpressions()));
+ resolvedCodeOwnerConfigBuilder.addCodeOwnerSet(codeOwnerSet);
+ }
- OptionalResultWithMessages<List<UnresolvedImport>> unresolvedImports =
- resolveImports(codeOwnerConfig, resolvedCodeOwnerConfigBuilder);
+ // Resolve all global imports.
+ Set<CodeOwnerConfigImport> globalImports = getGlobalImports(0, codeOwnerConfig);
+ OptionalResultWithMessages<List<UnresolvedImport>> unresolvedGlobalImports =
+ resolveImports(codeOwnerConfig.key(), globalImports, resolvedCodeOwnerConfigBuilder);
+ messages.addAll(unresolvedGlobalImports.messages());
- CodeOwnerConfig resolvedCodeOwnerConfig = resolvedCodeOwnerConfigBuilder.build();
-
- // Remove global code owner sets if any per-file code owner set has the
- // ignoreGlobalAndParentCodeOwners flag set to true.
+ // Remove all global code owner sets if any per-file code owner set has the
+ // ignoreGlobalAndParentCodeOwners flag set to true (as in this case they are ignored and
+ // hence not relevant).
// In this case also set ignoreParentCodeOwners to true, so that we do not need to inspect the
- // ignoreGlobalAndParentCodeOwners flags again.
+ // ignoreGlobalAndParentCodeOwners flags on per-file code owner sets again, but can just rely
+ // on the global ignoreParentCodeOwners flag.
Optional<CodeOwnerSet> matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners =
- getMatchingPerFileCodeOwnerSets(resolvedCodeOwnerConfig)
+ getMatchingPerFileCodeOwnerSets(resolvedCodeOwnerConfigBuilder.build())
.filter(CodeOwnerSet::ignoreGlobalAndParentCodeOwners)
.findAny();
if (matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners.isPresent()) {
@@ -263,21 +270,44 @@
matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners
.get()
.pathExpressions()));
- resolvedCodeOwnerConfig =
- resolvedCodeOwnerConfig
+ // We use resolvedCodeOwnerConfigBuilder to build up a code owner config that is scoped to
+ // the path and which has imports resolved. When resolving imports the relevant code owner
+ // sets from the imported code owner configs are added to the builder.
+ // If a per-file rule ignores global and parent code owners we have to drop all global code
+ // owner sets. The problem is that AutoValue doesn't allow us to remove/override code owner
+ // sets that have previously been added to the builder (we cannot call setCodeOwnerSets(...)
+ // after addCodeOwnerSet(...) or codeOwnerSetsBuilder() has been invoked). To override the
+ // code owner sets we build the code owner config and then create a fresh builder from it.
+ // Since the builder is fresh addCodeOwnerSet(...) and codeOwnerSetsBuilder() haven't been
+ // invoked on it yet we can now call setCodeOwnerSets(...).
+ resolvedCodeOwnerConfigBuilder =
+ resolvedCodeOwnerConfigBuilder
+ .build()
.toBuilder()
.setIgnoreParentCodeOwners()
.setCodeOwnerSets(
- resolvedCodeOwnerConfig.codeOwnerSets().stream()
+ resolvedCodeOwnerConfigBuilder.codeOwnerSets().stream()
.filter(codeOwnerSet -> !codeOwnerSet.pathExpressions().isEmpty())
- .collect(toImmutableSet()))
- .build();
+ .collect(toImmutableSet()));
}
- messages.addAll(unresolvedImports.messages());
+ // Resolve per-file imports.
+ Set<CodeOwnerConfigImport> perFileImports =
+ getPerFileImports(
+ 0, codeOwnerConfig.key(), resolvedCodeOwnerConfigBuilder.codeOwnerSets());
+ OptionalResultWithMessages<List<UnresolvedImport>> unresolvedPerFileImports =
+ resolveImports(codeOwnerConfig.key(), perFileImports, resolvedCodeOwnerConfigBuilder);
+ messages.addAll(unresolvedPerFileImports.messages());
+
this.pathCodeOwnersResult =
OptionalResultWithMessages.create(
- PathCodeOwnersResult.create(path, resolvedCodeOwnerConfig, unresolvedImports.get()),
+ PathCodeOwnersResult.create(
+ path,
+ resolvedCodeOwnerConfigBuilder.build(),
+ Stream.concat(
+ unresolvedGlobalImports.get().stream(),
+ unresolvedPerFileImports.get().stream())
+ .collect(toImmutableList())),
messages);
logger.atFine().log("path code owners result = %s", pathCodeOwnersResult);
return this.pathCodeOwnersResult;
@@ -287,17 +317,19 @@
/**
* Resolve the imports of the given code owner config.
*
- * @param importingCodeOwnerConfig the code owner config for which imports should be resolved
+ * @param keyOfImportingCodeOwnerConfig the key of the importing code owner config
+ * @param codeOwnerConfigImports the code owner configs that should be imported
* @param resolvedCodeOwnerConfigBuilder the builder for the resolved code owner config
* @return list of unresolved imports, empty list if all imports were successfully resolved
*/
private OptionalResultWithMessages<List<UnresolvedImport>> resolveImports(
- CodeOwnerConfig importingCodeOwnerConfig,
+ CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
+ Set<CodeOwnerConfigImport> codeOwnerConfigImports,
CodeOwnerConfig.Builder resolvedCodeOwnerConfigBuilder) {
ImmutableList.Builder<UnresolvedImport> unresolvedImports = ImmutableList.builder();
StringBuilder messageBuilder = new StringBuilder();
try (Timer0.Context ctx = codeOwnerMetrics.resolveCodeOwnerConfigImports.start()) {
- logger.atFine().log("resolve imports of codeOwnerConfig %s", importingCodeOwnerConfig.key());
+ logger.atFine().log("resolve imports of codeOwnerConfig %s", keyOfImportingCodeOwnerConfig);
// To detect cyclic dependencies we keep track of all seen code owner configs.
Set<CodeOwnerConfig.Key> seenCodeOwnerConfigs = new HashSet<>();
@@ -309,16 +341,12 @@
revisionMap.put(codeOwnerConfig.key().branchNameKey(), codeOwnerConfig.revision());
Queue<CodeOwnerConfigImport> codeOwnerConfigsToImport = new ArrayDeque<>();
- codeOwnerConfigsToImport.addAll(getGlobalImports(0, importingCodeOwnerConfig));
- codeOwnerConfigsToImport.addAll(
- getPerFileImports(
- 0, codeOwnerConfig.key(), resolvedCodeOwnerConfigBuilder.codeOwnerSets()));
-
+ codeOwnerConfigsToImport.addAll(codeOwnerConfigImports);
if (!codeOwnerConfigsToImport.isEmpty()) {
messageBuilder.append(
String.format(
"Code owner config %s imports:\n",
- importingCodeOwnerConfig.key().format(codeOwners)));
+ keyOfImportingCodeOwnerConfig.format(codeOwners)));
}
while (!codeOwnerConfigsToImport.isEmpty()) {
CodeOwnerConfigImport codeOwnerConfigImport = codeOwnerConfigsToImport.poll();
@@ -328,7 +356,7 @@
codeOwnerConfigImport.referenceToImportedCodeOwnerConfig();
CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig =
createKeyForImportedCodeOwnerConfig(
- importingCodeOwnerConfig.key(), codeOwnerConfigReference);
+ keyOfImportingCodeOwnerConfig, codeOwnerConfigReference);
try (Timer0.Context ctx2 = codeOwnerMetrics.resolveCodeOwnerConfigImport.start()) {
logger.atFine().log(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersTest.java
index 8929a72..d4ec968 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.extensions.registration.RegistrationHandle;
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.server.IdentifiedUser;
import com.google.inject.Inject;
import com.google.inject.Key;
@@ -60,6 +61,7 @@
private PathCodeOwners.Factory pathCodeOwnersFactory;
private DynamicMap<CodeOwnerBackend> codeOwnerBackends;
private Provider<TransientCodeOwnerConfigCache> transientCodeOwnerConfigCacheProvider;
+ private TestPathExpressions testPathExpressions;
@Before
public void setUpCodeOwnersPlugin() throws Exception {
@@ -70,6 +72,7 @@
plugin.getSysInjector().getInstance(new Key<DynamicMap<CodeOwnerBackend>>() {});
transientCodeOwnerConfigCacheProvider =
plugin.getSysInjector().getInstance(new Key<Provider<TransientCodeOwnerConfigCache>>() {});
+ testPathExpressions = plugin.getSysInjector().getInstance(TestPathExpressions.class);
}
@Test
@@ -1952,6 +1955,57 @@
.isTrue();
}
+ @Test
+ public void perFileRuleThatIgnoresGlobalCodeOwnersCanImportGlobalCodeOwnersFromOtherFile()
+ throws Exception {
+ // 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 importingCodeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .setIgnoreGlobalAndParentCodeOwners()
+ .addImport(
+ CodeOwnerConfigReference.create(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY, "/bar/OWNERS"))
+ .build())
+ .create();
+
+ // create imported config with a global code owner
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/bar/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ Optional<PathCodeOwners> pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCacheProvider.get(),
+ importingCodeOwnerConfigKey,
+ projectOperations.project(project).getHead("master"),
+ Paths.get("/foo.md"));
+ assertThat(pathCodeOwners).isPresent();
+
+ // Expectation: we get the global code owner from the imported code owner config (since it is
+ // imported by 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)
+ assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().get().getPathCodeOwners())
+ .comparingElementsUsing(hasEmail())
+ .containsExactly(user.email());
+ assertThat(pathCodeOwners.get().resolveCodeOwnerConfig().get().hasUnresolvedImports())
+ .isFalse();
+ }
+
private CodeOwnerConfig.Builder createCodeOwnerBuilder() {
return CodeOwnerConfig.builder(
CodeOwnerConfig.Key.create(BranchNameKey.create(project, "master"), Paths.get("/")),
diff --git a/ui/code-owners-service.js b/ui/code-owners-service.js
index 13f18d7..5f6bc92 100644
--- a/ui/code-owners-service.js
+++ b/ui/code-owners-service.js
@@ -109,7 +109,7 @@
listOwnersForPath(changeId, path) {
return this.restApi.get(
`/changes/${changeId}/revisions/current/code_owners` +
- `/${encodeURIComponent(path)}?limit=5&o=DETAILS`
+ `/${encodeURIComponent(path)}?limit=5&o=DETAILS&resolve-all-users=true`
);
}
