Merge changes Id70060a4,Ic092c3db
* changes:
CodeOwnerApprovalCheck: Do not read config params per file
CodeOwnerApprovalCheck: Avoid double instantiation of PathCodeOwners
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 6c36cd1..a582589 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -282,6 +282,9 @@
getApproverAccountIds(requiredApproval, changeNotes, patchSetUploader);
logger.atFine().log("reviewers = %s, approvers = %s", reviewerAccountIds, approverAccountIds);
+ FallbackCodeOwners fallbackCodeOwners =
+ codeOwnersPluginConfiguration.getFallbackCodeOwners(branch.project());
+
return changedFiles
.compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
.stream()
@@ -295,6 +298,7 @@
patchSetUploader,
reviewerAccountIds,
approverAccountIds,
+ fallbackCodeOwners,
hasOverride,
changedFile));
}
@@ -362,6 +366,7 @@
/* reviewerAccountIds= */ ImmutableSet.of(),
// Assume an explicit approval of the given account.
/* approverAccountIds= */ ImmutableSet.of(accountId),
+ fallbackCodeOwners,
/* hasOverride= */ false,
changedFile));
}
@@ -408,6 +413,7 @@
@Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
+ FallbackCodeOwners fallbackCodeOwners,
boolean hasOverride,
ChangedFile changedFile) {
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatus.start()) {
@@ -427,6 +433,7 @@
patchSetUploader,
reviewerAccountIds,
approverAccountIds,
+ fallbackCodeOwners,
hasOverride,
newPath));
@@ -448,6 +455,7 @@
patchSetUploader,
reviewerAccountIds,
approverAccountIds,
+ fallbackCodeOwners,
hasOverride,
changedFile.oldPath().get()));
}
@@ -467,6 +475,7 @@
@Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
+ FallbackCodeOwners fallbackCodeOwners,
boolean hasOverride,
Path absolutePath) {
logger.atFine().log("computing path status for %s", absolutePath);
@@ -503,38 +512,39 @@
branch,
revision,
absolutePath,
- codeOwnerConfig -> {
- CodeOwnerResolverResult codeOwners = getCodeOwners(codeOwnerConfig, absolutePath);
- logger.atFine().log(
- "code owners = %s (code owner config folder path = %s, file name = %s)",
- codeOwners,
- codeOwnerConfig.key().folderPath(),
- codeOwnerConfig.key().fileName().orElse("<default>"));
+ (PathCodeOwnersVisitor)
+ pathCodeOwners -> {
+ CodeOwnerResolverResult codeOwners = resolveCodeOwners(pathCodeOwners);
+ logger.atFine().log(
+ "code owners = %s (code owner config folder path = %s, file name = %s)",
+ codeOwners,
+ pathCodeOwners.getCodeOwnerConfig().key().folderPath(),
+ pathCodeOwners.getCodeOwnerConfig().key().fileName().orElse("<default>"));
- if (codeOwners.hasRevelantCodeOwnerDefinitions()) {
- hasRevelantCodeOwnerDefinitions.set(true);
- }
+ if (codeOwners.hasRevelantCodeOwnerDefinitions()) {
+ hasRevelantCodeOwnerDefinitions.set(true);
+ }
- if (isApproved(
- absolutePath,
- codeOwners,
- approverAccountIds,
- enableImplicitApprovalFromUploader,
- patchSetUploader)) {
- codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
- return false;
- } else if (isPending(absolutePath, codeOwners, reviewerAccountIds)) {
- codeOwnerStatus.set(CodeOwnerStatus.PENDING);
+ if (isApproved(
+ absolutePath,
+ codeOwners,
+ approverAccountIds,
+ enableImplicitApprovalFromUploader,
+ patchSetUploader)) {
+ codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
+ return false;
+ } else if (isPending(absolutePath, codeOwners, reviewerAccountIds)) {
+ codeOwnerStatus.set(CodeOwnerStatus.PENDING);
- // We need to continue to check if any of the higher-level code owners approved the
- // change.
- return true;
- }
+ // We need to continue to check if any of the higher-level code owners approved
+ // the change.
+ return true;
+ }
- // We need to continue to check if any of the higher-level code owners approved the
- // change or is a reviewer.
- return true;
- },
+ // We need to continue to check if any of the higher-level code owners approved the
+ // change or is a reviewer.
+ return true;
+ },
codeOwnerConfigKey -> {
logger.atFine().log(
"code owner config %s ignores parent code owners for %s",
@@ -557,6 +567,7 @@
patchSetUploader,
reviewerAccountIds,
approverAccountIds,
+ fallbackCodeOwners,
absolutePath));
}
}
@@ -699,9 +710,8 @@
@Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
+ FallbackCodeOwners fallbackCodeOwners,
Path absolutePath) {
- FallbackCodeOwners fallbackCodeOwners =
- codeOwnersPluginConfiguration.getFallbackCodeOwners(branch.project());
logger.atFine().log(
"getting code owner status for fallback code owners (fallback code owners = %s)",
fallbackCodeOwners);
@@ -822,17 +832,12 @@
}
/**
- * Gets the code owners that own the given path according to the given code owner config.
+ * Resolves the given path code owners.
*
- * @param codeOwnerConfig the code owner config from which the code owners should be retrieved
- * @param absolutePath the path for which the code owners should be retrieved
+ * @param pathCodeOwners the path code owners that should be resolved
*/
- private CodeOwnerResolverResult getCodeOwners(
- CodeOwnerConfig codeOwnerConfig, Path absolutePath) {
- return codeOwnerResolver
- .get()
- .enforceVisibility(false)
- .resolvePathCodeOwners(codeOwnerConfig, absolutePath);
+ private CodeOwnerResolverResult resolveCodeOwners(PathCodeOwners pathCodeOwners) {
+ return codeOwnerResolver.get().enforceVisibility(false).resolvePathCodeOwners(pathCodeOwners);
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index 473cdc5..cecd46d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -101,10 +101,39 @@
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
+ requireNonNull(codeOwnerConfigVisitor, "codeOwnerConfigVisitor");
+ PathCodeOwnersVisitor pathCodeOwnersVisitor =
+ pathCodeOwners -> codeOwnerConfigVisitor.visit(pathCodeOwners.getCodeOwnerConfig());
+ visit(
+ branchNameKey,
+ revision,
+ absolutePath,
+ pathCodeOwnersVisitor,
+ parentCodeOwnersIgnoredCallback);
+ }
+
+ /**
+ * Visits the path code owners in the given branch that apply for the given path by following the
+ * path hierarchy from the given path up to the root folder.
+ *
+ * @param branchNameKey project and branch from which the code owner configs should be visited
+ * @param revision the branch revision from which the code owner configs should be loaded
+ * @param absolutePath the path for which the code owner configs should be visited; the path must
+ * be absolute; can be the path of a file or folder; the path may or may not exist
+ * @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
+ * @param parentCodeOwnersIgnoredCallback callback that is invoked for the first visited code
+ * owner config that ignores parent code owners
+ */
+ public void visit(
+ BranchNameKey branchNameKey,
+ ObjectId revision,
+ Path absolutePath,
+ PathCodeOwnersVisitor pathCodeOwnersVisitor,
+ Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
requireNonNull(branchNameKey, "branch");
requireNonNull(revision, "revision");
requireNonNull(absolutePath, "absolutePath");
- requireNonNull(codeOwnerConfigVisitor, "codeOwnerConfigVisitor");
+ requireNonNull(pathCodeOwnersVisitor, "pathCodeOwnersVisitor");
requireNonNull(parentCodeOwnersIgnoredCallback, "parentCodeOwnersIgnoredCallback");
checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath);
@@ -127,8 +156,7 @@
pathCodeOwnersFactory.create(codeOwnerConfigKey, revision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
- boolean visitFurtherCodeOwnerConfigs =
- codeOwnerConfigVisitor.visit(pathCodeOwners.get().getCodeOwnerConfig());
+ boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
boolean ignoreParentCodeOwners =
pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
if (ignoreParentCodeOwners) {
@@ -155,7 +183,7 @@
if (!RefNames.REFS_CONFIG.equals(branchNameKey.branch())) {
visitCodeOwnerConfigInRefsMetaConfig(
- branchNameKey.project(), absolutePath, codeOwnerConfigVisitor);
+ branchNameKey.project(), absolutePath, pathCodeOwnersVisitor);
}
}
@@ -174,11 +202,10 @@
* the {@code refs/meta/config} branch
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
- * @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
- * configs
+ * @param pathCodeOwnersVisitor visitor that should be invoked
*/
private void visitCodeOwnerConfigInRefsMetaConfig(
- Project.NameKey project, Path absolutePath, CodeOwnerConfigVisitor codeOwnerConfigVisitor) {
+ Project.NameKey project, Path absolutePath, PathCodeOwnersVisitor pathCodeOwnersVisitor) {
CodeOwnerConfig.Key metaCodeOwnerConfigKey =
CodeOwnerConfig.Key.create(project, RefNames.REFS_CONFIG, "/");
logger.atFine().log("visiting code owner config %s", metaCodeOwnerConfigKey);
@@ -194,7 +221,7 @@
pathCodeOwnersFactory.create(metaCodeOwnerConfigKey, metaRevision, absolutePath);
if (pathCodeOwners.isPresent()) {
logger.atFine().log("visit code owner config %s", metaCodeOwnerConfigKey);
- codeOwnerConfigVisitor.visit(pathCodeOwners.get().getCodeOwnerConfig());
+ pathCodeOwnersVisitor.visit(pathCodeOwners.get());
} else {
logger.atFine().log("code owner config %s not found", metaCodeOwnerConfigKey);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index cd874ee..a16714c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -138,6 +138,10 @@
* Resolves the code owners from the given code owner config for the given path from {@link
* CodeOwnerReference}s to a {@link CodeOwner}s.
*
+ * <p>If the code owner config has already been resolved to {@link PathCodeOwners}, prefer calling
+ * {@link #resolvePathCodeOwners(PathCodeOwners)} instead, so that {@link PathCodeOwners} do not
+ * need to be created again.
+ *
* <p>Non-resolvable code owners are filtered out.
*
* @param codeOwnerConfig the code owner config for which the local owners for the given path
@@ -151,13 +155,27 @@
requireNonNull(codeOwnerConfig, "codeOwnerConfig");
requireNonNull(absolutePath, "absolutePath");
checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath);
+ return resolvePathCodeOwners(pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath));
+ }
+
+ /**
+ * Resolves the the given path code owners from {@link CodeOwnerReference}s to a {@link
+ * CodeOwner}s.
+ *
+ * <p>Non-resolvable code owners are filtered out.
+ *
+ * @param pathCodeOwners the path code owners that should be resolved
+ * @return the resolved code owners
+ */
+ public CodeOwnerResolverResult resolvePathCodeOwners(PathCodeOwners pathCodeOwners) {
+ requireNonNull(pathCodeOwners, "pathCodeOwners");
try (Timer0.Context ctx = codeOwnerMetrics.resolvePathCodeOwners.start()) {
logger.atFine().log(
"resolve path code owners (code owner config = %s, path = %s)",
- codeOwnerConfig.key(), absolutePath);
+ pathCodeOwners.getCodeOwnerConfig().key(), pathCodeOwners.getPath());
OptionalResultWithMessages<PathCodeOwnersResult> pathCodeOwnersResult =
- pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig();
+ pathCodeOwners.resolveCodeOwnerConfig();
return resolve(
pathCodeOwnersResult.get().getPathCodeOwners(),
pathCodeOwnersResult.get().unresolvedImports(),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index cdf0f84..78e315a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -156,6 +156,11 @@
return codeOwnerConfig;
}
+ /** Returns the absolute path for which code owners were computed. */
+ public Path getPath() {
+ return path;
+ }
+
/**
* Resolves the {@link #codeOwnerConfig}.
*
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersVisitor.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersVisitor.java
new file mode 100644
index 0000000..2d69ca2
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwnersVisitor.java
@@ -0,0 +1,27 @@
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+/** Callback interface to visit {@link PathCodeOwners}. */
+@FunctionalInterface
+public interface PathCodeOwnersVisitor {
+ /**
+ * Callback for {@link PathCodeOwners}.
+ *
+ * @param pathCodeOwners the path code owners
+ * @return whether further path code owner configs should be visited
+ */
+ boolean visit(PathCodeOwners pathCodeOwners);
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 6a4ea64..0787e9a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -422,6 +422,15 @@
}
@Test
+ public void cannotResolvePathCodeOwnersOfNullPathCodeOwners() throws Exception {
+ NullPointerException npe =
+ assertThrows(
+ NullPointerException.class,
+ () -> codeOwnerResolver.get().resolvePathCodeOwners(/* pathCodeOwners= */ null));
+ assertThat(npe).hasMessageThat().isEqualTo("pathCodeOwners");
+ }
+
+ @Test
public void cannotResolvePathCodeOwnersForRelativePath() throws Exception {
String relativePath = "foo/bar.md";
CodeOwnerConfig codeOwnerConfig =
