PathCodeOwners: Flatten the return type (step 3/3)
Add a AutoValue builder for PathCodeOwnersResult.
This allows us to get rid of local vars to collect the result data.
Having an AutoValue builder for PathCodeOwnersResult also allows us to
unify how results within PathCodeOwners are passed between methods. At
the moment the resolveImports method has a return value to return
resolved/unresolved imports and the messages, and in addition it gets a
CodeOwnerConfig.Builder passed in to write out relevant parts of the
imported CodeOwnerConfig. Using 2 different approaches to return results
is confusing. If we have an AutoValue builder for PathCodeOwnersResult
we can just use this builder to collect all the results.
Change-Id: I69c13959dc08517d9dbc8e144d7a28b0fe388582
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 07e6e94..dc6da3e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -21,7 +21,6 @@
import com.google.auto.value.AutoValue;
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.common.Nullable;
@@ -36,10 +35,8 @@
import com.google.inject.Singleton;
import java.nio.file.Path;
import java.util.ArrayDeque;
-import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
-import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Queue;
@@ -219,6 +216,11 @@
try (Timer0.Context ctx = codeOwnerMetrics.resolveCodeOwnerConfig.start()) {
Path codeOwnerConfigFilePath = codeOwners.getFilePath(codeOwnerConfig.key());
+
+ PathCodeOwnersResult.Builder pathCodeOwnersResultBuilder =
+ PathCodeOwnersResult.builder(
+ path, codeOwnerConfig.key(), codeOwnerConfig.ignoreParentCodeOwners());
+
logger.atFine().log(
"resolve code owners for %s from code owner config %s:%s:%s",
path,
@@ -226,8 +228,7 @@
codeOwnerConfig.key().shortBranchName(),
codeOwnerConfigFilePath);
- List<String> messages = new ArrayList<>();
- messages.add(
+ pathCodeOwnersResultBuilder.addMessage(
String.format(
"resolve code owners for %s from code owner config %s:%s:%s",
path,
@@ -235,25 +236,19 @@
codeOwnerConfig.key().shortBranchName(),
codeOwnerConfigFilePath));
- // Create vars to collect result data.
- boolean ignoreParentCodeOwners = codeOwnerConfig.ignoreParentCodeOwners();
- ImmutableSet.Builder<CodeOwnerSet> codeOwnerSets = ImmutableSet.builder();
- ImmutableList.Builder<CodeOwnerConfigImport> resolvedImports = ImmutableList.builder();
- ImmutableList.Builder<CodeOwnerConfigImport> unresolvedImports = ImmutableList.builder();
-
// 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.
- getGlobalCodeOwnerSets(codeOwnerConfig).forEach(codeOwnerSets::add);
+ getGlobalCodeOwnerSets(codeOwnerConfig).forEach(pathCodeOwnersResultBuilder::addCodeOwnerSet);
boolean globalCodeOwnersIgnored = false;
ImmutableSet<CodeOwnerSet> matchingPerFileCodeOwnerSets =
getMatchingPerFileCodeOwnerSets(codeOwnerConfig).collect(toImmutableSet());
for (CodeOwnerSet codeOwnerSet : matchingPerFileCodeOwnerSets) {
- messages.add(
+ pathCodeOwnersResultBuilder.addMessage(
String.format(
"per-file code owner set with path expressions %s matches",
codeOwnerSet.pathExpressions()));
- codeOwnerSets.add(codeOwnerSet);
+ pathCodeOwnersResultBuilder.addCodeOwnerSet(codeOwnerSet);
if (codeOwnerSet.ignoreGlobalAndParentCodeOwners()) {
globalCodeOwnersIgnored = true;
}
@@ -261,30 +256,22 @@
// Resolve global imports.
ImmutableSet<CodeOwnerImport> globalImports = getGlobalImports(0, codeOwnerConfig);
- OptionalResultWithMessages<CodeOwnerConfigImports> globalImportedCodeOwnerConfigs;
if (!globalCodeOwnersIgnored) {
- globalImportedCodeOwnerConfigs = resolveImports(codeOwnerConfig.key(), globalImports);
+ resolveImports(codeOwnerConfig.key(), globalImports, pathCodeOwnersResultBuilder);
} else {
// skip global import with mode GLOBAL_CODE_OWNER_SETS_ONLY,
// since we already know that global code owners will be ignored, we do not need to resolve
// these imports
- globalImportedCodeOwnerConfigs =
- resolveImports(
- codeOwnerConfig.key(),
- globalImports.stream()
- .filter(
- codeOwnerConfigImport ->
- codeOwnerConfigImport.referenceToImportedCodeOwnerConfig().importMode()
- != CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY)
- .collect(toImmutableSet()));
+ resolveImports(
+ codeOwnerConfig.key(),
+ globalImports.stream()
+ .filter(
+ codeOwnerConfigImport ->
+ codeOwnerConfigImport.referenceToImportedCodeOwnerConfig().importMode()
+ != CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY)
+ .collect(toImmutableSet()),
+ pathCodeOwnersResultBuilder);
}
- messages.addAll(globalImportedCodeOwnerConfigs.messages());
- if (globalImportedCodeOwnerConfigs.get().ignoreParentCodeOwners()) {
- ignoreParentCodeOwners = true;
- }
- codeOwnerSets.addAll(globalImportedCodeOwnerConfigs.get().codeOwnerSets());
- resolvedImports.addAll(globalImportedCodeOwnerConfigs.get().resolved());
- unresolvedImports.addAll(globalImportedCodeOwnerConfigs.get().unresolved());
// 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
@@ -293,12 +280,12 @@
// ignoreGlobalAndParentCodeOwners flags on per-file code owner sets again, but can just rely
// on the global ignoreParentCodeOwners flag.
Optional<CodeOwnerSet> matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners =
- getMatchingPerFileCodeOwnerSets(codeOwnerSets.build())
+ getMatchingPerFileCodeOwnerSets(pathCodeOwnersResultBuilder.build().codeOwnerSets())
.filter(CodeOwnerSet::ignoreGlobalAndParentCodeOwners)
.findAny();
if (matchingPerFileCodeOwnerSetThatIgnoresGlobalAndParentCodeOwners.isPresent()) {
logger.atFine().log("remove folder code owner sets and set ignoreParentCodeOwners to true");
- messages.add(
+ pathCodeOwnersResultBuilder.addMessage(
String.format(
"found matching per-file code owner set (with path expressions = %s) that ignores"
+ " parent code owners, hence ignoring the folder code owners",
@@ -307,38 +294,29 @@
.pathExpressions()));
// If a per-file rule ignores global and parent code owners we have to drop all global code
// owner sets.
- ignoreParentCodeOwners = true;
- codeOwnerSets =
- ImmutableSet.<CodeOwnerSet>builder()
- .addAll(
- codeOwnerSets.build().stream()
+ pathCodeOwnersResultBuilder.ignoreParentCodeOwners();
+ PathCodeOwnersResult intermediatePathCodeOwnersResult = pathCodeOwnersResultBuilder.build();
+ pathCodeOwnersResultBuilder =
+ PathCodeOwnersResult.builder(
+ intermediatePathCodeOwnersResult.path(),
+ intermediatePathCodeOwnersResult.codeOwnerConfigKey(),
+ intermediatePathCodeOwnersResult.ignoreParentCodeOwners())
+ .addAllCodeOwnerSets(
+ intermediatePathCodeOwnersResult.codeOwnerSets().stream()
.filter(codeOwnerSet -> !codeOwnerSet.pathExpressions().isEmpty())
- .collect(toImmutableSet()));
+ .collect(toImmutableSet()))
+ .addAllResolvedImports(intermediatePathCodeOwnersResult.resolvedImports())
+ .addAllUnresolvedImports(intermediatePathCodeOwnersResult.unresolvedImports())
+ .addAllMessages(intermediatePathCodeOwnersResult.messages());
}
// Resolve per-file imports.
ImmutableSet<CodeOwnerImport> perFileImports =
getPerFileImports(0, codeOwnerConfig, matchingPerFileCodeOwnerSets);
- OptionalResultWithMessages<CodeOwnerConfigImports> perFileImportedCodeOwnerConfigs =
- resolveImports(codeOwnerConfig.key(), perFileImports);
- messages.addAll(perFileImportedCodeOwnerConfigs.messages());
- if (perFileImportedCodeOwnerConfigs.get().ignoreParentCodeOwners()) {
- ignoreParentCodeOwners = true;
- }
- codeOwnerSets.addAll(perFileImportedCodeOwnerConfigs.get().codeOwnerSets());
- resolvedImports.addAll(perFileImportedCodeOwnerConfigs.get().resolved());
- unresolvedImports.addAll(perFileImportedCodeOwnerConfigs.get().unresolved());
+ resolveImports(codeOwnerConfig.key(), perFileImports, pathCodeOwnersResultBuilder);
- this.pathCodeOwnersResult =
- PathCodeOwnersResult.create(
- path,
- codeOwnerConfig.key(),
- ignoreParentCodeOwners,
- codeOwnerSets.build(),
- resolvedImports.build(),
- unresolvedImports.build(),
- messages);
- logger.atFine().log("path code owners result = %s", pathCodeOwnersResult);
+ this.pathCodeOwnersResult = pathCodeOwnersResultBuilder.build();
+ logger.atFine().log("path code owners result = %s", this.pathCodeOwnersResult);
return this.pathCodeOwnersResult;
}
}
@@ -348,16 +326,11 @@
*
* @param keyOfImportingCodeOwnerConfig the key of the importing code owner config
* @param codeOwnerConfigImports the code owner configs that should be imported
- * @return list of unresolved imports, empty list if all imports were successfully resolved
*/
- private OptionalResultWithMessages<CodeOwnerConfigImports> resolveImports(
+ private void resolveImports(
CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig,
- Set<CodeOwnerImport> codeOwnerConfigImports) {
- // Create vars to collect result data.
- boolean ignoreParentCodeOwners = false;
- ImmutableSet.Builder<CodeOwnerSet> codeOwnerSets = ImmutableSet.builder();
- ImmutableList.Builder<CodeOwnerConfigImport> resolvedImports = ImmutableList.builder();
- ImmutableList.Builder<CodeOwnerConfigImport> unresolvedImports = ImmutableList.builder();
+ Set<CodeOwnerImport> codeOwnerConfigImports,
+ PathCodeOwnersResult.Builder pathCodeOwnersResultBuilder) {
StringBuilder messageBuilder = new StringBuilder();
try (Timer0.Context ctx = codeOwnerMetrics.resolveCodeOwnerConfigImports.start()) {
@@ -397,7 +370,7 @@
Optional<ProjectState> projectState =
projectCache.get(keyOfImportedCodeOwnerConfig.project());
if (!projectState.isPresent()) {
- unresolvedImports.add(
+ pathCodeOwnersResultBuilder.addUnresolvedImport(
CodeOwnerConfigImport.createUnresolvedImport(
codeOwnerConfigImport.importingCodeOwnerConfig(),
keyOfImportedCodeOwnerConfig,
@@ -409,7 +382,7 @@
continue;
}
if (!projectState.get().statePermitsRead()) {
- unresolvedImports.add(
+ pathCodeOwnersResultBuilder.addUnresolvedImport(
CodeOwnerConfigImport.createUnresolvedImport(
codeOwnerConfigImport.importingCodeOwnerConfig(),
keyOfImportedCodeOwnerConfig,
@@ -435,7 +408,7 @@
: codeOwnerConfigLoader.getFromCurrentRevision(keyOfImportedCodeOwnerConfig);
if (!mayBeImportedCodeOwnerConfig.isPresent()) {
- unresolvedImports.add(
+ pathCodeOwnersResultBuilder.addUnresolvedImport(
CodeOwnerConfigImport.createUnresolvedImport(
codeOwnerConfigImport.importingCodeOwnerConfig(),
keyOfImportedCodeOwnerConfig,
@@ -451,7 +424,7 @@
CodeOwnerConfig importedCodeOwnerConfig = mayBeImportedCodeOwnerConfig.get();
- resolvedImports.add(
+ pathCodeOwnersResultBuilder.addResolvedImport(
CodeOwnerConfigImport.createResolvedImport(
codeOwnerConfigImport.importingCodeOwnerConfig(),
importedCodeOwnerConfig,
@@ -466,12 +439,13 @@
if (importMode.importIgnoreParentCodeOwners()
&& importedCodeOwnerConfig.ignoreParentCodeOwners()) {
logger.atFine().log("import ignoreParentCodeOwners flag");
- ignoreParentCodeOwners = true;
+ pathCodeOwnersResultBuilder.ignoreParentCodeOwners();
}
if (importMode.importGlobalCodeOwnerSets()) {
logger.atFine().log("import global code owners");
- getGlobalCodeOwnerSets(importedCodeOwnerConfig).forEach(codeOwnerSets::add);
+ getGlobalCodeOwnerSets(importedCodeOwnerConfig)
+ .forEach(pathCodeOwnersResultBuilder::addCodeOwnerSet);
}
ImmutableSet<CodeOwnerSet> matchingPerFileCodeOwnerSets =
@@ -485,7 +459,7 @@
String.format(
"per-file code owner set with path expressions %s matches\n",
codeOwnerSet.pathExpressions())));
- codeOwnerSets.add(codeOwnerSet);
+ pathCodeOwnersResultBuilder.addCodeOwnerSet(codeOwnerSet);
});
}
@@ -532,13 +506,9 @@
if (message.endsWith("\n")) {
message = message.substring(0, message.length() - 1);
}
- return OptionalResultWithMessages.create(
- CodeOwnerConfigImports.create(
- ignoreParentCodeOwners,
- codeOwnerSets.build(),
- resolvedImports.build(),
- unresolvedImports.build()),
- !message.isEmpty() ? ImmutableList.of(message) : ImmutableList.of());
+ if (!message.isEmpty()) {
+ pathCodeOwnersResultBuilder.addMessage(message);
+ }
}
private ImmutableSet<CodeOwnerImport> getGlobalImports(
@@ -735,26 +705,4 @@
importLevel, importingCodeOwnerConfig, codeOwnerConfigReference, codeOwnerSet);
}
}
-
- @AutoValue
- abstract static class CodeOwnerConfigImports {
- abstract boolean ignoreParentCodeOwners();
-
- abstract ImmutableSet<CodeOwnerSet> codeOwnerSets();
-
- /** Imported code owner configs the could be resolved. */
- abstract ImmutableList<CodeOwnerConfigImport> resolved();
-
- /** Imported code owner configs the could not be resolved. */
- abstract ImmutableList<CodeOwnerConfigImport> unresolved();
-
- static CodeOwnerConfigImports create(
- boolean ignoreParentCodeOwners,
- ImmutableSet<CodeOwnerSet> codeOwnerSets,
- ImmutableList<CodeOwnerConfigImport> resolved,
- ImmutableList<CodeOwnerConfigImport> unresolved) {
- return new AutoValue_PathCodeOwners_CodeOwnerConfigImports(
- ignoreParentCodeOwners, codeOwnerSets, resolved, unresolved);
- }
- }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
index 85aa14e..6a45425 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResult.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
@@ -23,8 +24,6 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
import java.nio.file.Path;
-import java.util.List;
-import java.util.Set;
/** The result of resolving path code owners via {@link PathCodeOwners}. */
@AutoValue
@@ -112,22 +111,83 @@
.toString();
}
- /** Creates a {@link PathCodeOwnersResult} instance. */
- public static PathCodeOwnersResult create(
- Path path,
- CodeOwnerConfig.Key codeOwnerConfigKey,
- boolean ignoreParentCodeOwners,
- Set<CodeOwnerSet> codeOwnerSets,
- List<CodeOwnerConfigImport> resolvedImports,
- List<CodeOwnerConfigImport> unresolvedImports,
- List<String> messages) {
- return new AutoValue_PathCodeOwnersResult(
- path,
- codeOwnerConfigKey,
- ignoreParentCodeOwners,
- ImmutableSet.copyOf(codeOwnerSets),
- ImmutableList.copyOf(resolvedImports),
- ImmutableList.copyOf(unresolvedImports),
- ImmutableList.copyOf(messages));
+ /** Creates a builder for a {@link PathCodeOwnersResult} instance. */
+ public static Builder builder(
+ Path path, CodeOwnerConfig.Key codeOwnerConfigKey, boolean ignoreParentCodeOwners) {
+ return new AutoValue_PathCodeOwnersResult.Builder()
+ .path(path)
+ .codeOwnerConfigKey(codeOwnerConfigKey)
+ .ignoreParentCodeOwners(ignoreParentCodeOwners);
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+ abstract Builder path(Path path);
+
+ abstract Builder codeOwnerConfigKey(CodeOwnerConfig.Key codeOwnerConfigKey);
+
+ abstract Builder ignoreParentCodeOwners(boolean ignoreParentCodeOwners);
+
+ Builder ignoreParentCodeOwners() {
+ return ignoreParentCodeOwners(true);
+ }
+
+ abstract ImmutableSet.Builder<CodeOwnerSet> codeOwnerSetsBuilder();
+
+ Builder addCodeOwnerSet(CodeOwnerSet codeOwnerSet) {
+ requireNonNull(codeOwnerSet, "codeOwnerSet");
+ codeOwnerSetsBuilder().add(codeOwnerSet);
+ return this;
+ }
+
+ Builder addAllCodeOwnerSets(ImmutableSet<CodeOwnerSet> codeOwnerSets) {
+ requireNonNull(codeOwnerSets, "codeOwnerSet2");
+ codeOwnerSetsBuilder().addAll(codeOwnerSets);
+ return this;
+ }
+
+ abstract ImmutableList.Builder<CodeOwnerConfigImport> resolvedImportsBuilder();
+
+ Builder addResolvedImport(CodeOwnerConfigImport codeOwnerConfigImport) {
+ requireNonNull(codeOwnerConfigImport, "codeOwnerConfigImport");
+ resolvedImportsBuilder().add(codeOwnerConfigImport);
+ return this;
+ }
+
+ Builder addAllResolvedImports(ImmutableList<CodeOwnerConfigImport> codeOwnerConfigImports) {
+ requireNonNull(codeOwnerConfigImports, "codeOwnerConfigImports");
+ resolvedImportsBuilder().addAll(codeOwnerConfigImports);
+ return this;
+ }
+
+ abstract ImmutableList.Builder<CodeOwnerConfigImport> unresolvedImportsBuilder();
+
+ Builder addUnresolvedImport(CodeOwnerConfigImport codeOwnerConfigImport) {
+ requireNonNull(codeOwnerConfigImport, "codeOwnerConfigImport");
+ unresolvedImportsBuilder().add(codeOwnerConfigImport);
+ return this;
+ }
+
+ Builder addAllUnresolvedImports(ImmutableList<CodeOwnerConfigImport> codeOwnerConfigImports) {
+ requireNonNull(codeOwnerConfigImports, "codeOwnerConfigImports");
+ unresolvedImportsBuilder().addAll(codeOwnerConfigImports);
+ return this;
+ }
+
+ abstract ImmutableList.Builder<String> messagesBuilder();
+
+ Builder addMessage(String message) {
+ requireNonNull(message, "message");
+ messagesBuilder().add(message);
+ return this;
+ }
+
+ Builder addAllMessages(ImmutableList<String> messages) {
+ requireNonNull(messages, "messages");
+ messagesBuilder().addAll(messages);
+ return this;
+ }
+
+ abstract PathCodeOwnersResult build();
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResultTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResultTest.java
index d6a431e..932a91b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResultTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersResultTest.java
@@ -14,7 +14,6 @@
package com.google.gerrit.plugins.codeowners.backend;
-import com.google.common.collect.ImmutableList;
import java.nio.file.Path;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
@@ -34,25 +33,25 @@
CodeOwnerConfigReference unresolvableCodeOwnerConfigReference =
CodeOwnerConfigReference.create(CodeOwnerConfigImportMode.ALL, "/baz/OWNERS");
PathCodeOwnersResult pathCodeOwnersResult =
- PathCodeOwnersResult.create(
- Path.of("/foo/bar/baz.md"),
- codeOwnerConfig.key(),
- codeOwnerConfig.ignoreParentCodeOwners(),
- codeOwnerConfig.codeOwnerSets(),
- ImmutableList.of(
+ PathCodeOwnersResult.builder(
+ Path.of("/foo/bar/baz.md"),
+ codeOwnerConfig.key(),
+ codeOwnerConfig.ignoreParentCodeOwners())
+ .addAllCodeOwnerSets(codeOwnerConfig.codeOwnerSets())
+ .addResolvedImport(
CodeOwnerConfigImport.createResolvedImport(
codeOwnerConfig,
CodeOwnerConfig.builder(
CodeOwnerConfig.Key.create(project, "master", "/bar/"), TEST_REVISION)
.build(),
- resolvableCodeOwnerConfigReference)),
- ImmutableList.of(
+ resolvableCodeOwnerConfigReference))
+ .addUnresolvedImport(
CodeOwnerConfigImport.createUnresolvedImport(
codeOwnerConfig,
CodeOwnerConfig.Key.create(project, "master", "/baz/"),
unresolvableCodeOwnerConfigReference,
- "test message")),
- ImmutableList.of());
+ "test message"))
+ .build();
assertThatToStringIncludesAllData(pathCodeOwnersResult, PathCodeOwnersResult.class);
}
}
