Merge "CodeOwnerApprovalCheck: Factor out computation of input data"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
index 1309f8f..89f493f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/BackendModule.java
@@ -36,6 +36,7 @@
public class BackendModule extends FactoryModule {
@Override
protected void configure() {
+ factory(CodeOwnerApprovalCheckInput.Loader.Factory.class);
factory(CodeOwnersUpdate.Factory.class);
factory(CodeOwnerConfigScanner.Factory.class);
factory(CodeOwnersPluginGlobalConfigSnapshot.Factory.class);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 9045c90..792f641 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -41,14 +41,12 @@
import com.google.gerrit.plugins.codeowners.common.ChangedFile;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
-import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.PureRevertCache;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.util.AccountTemplateUtil;
-import com.google.gerrit.server.util.LabelVote;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@@ -89,7 +87,7 @@
private final PureRevertCache pureRevertCache;
private final Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider;
private final Provider<CodeOwnerResolver> codeOwnerResolverProvider;
- private final ApprovalsUtil approvalsUtil;
+ private final CodeOwnerApprovalCheckInput.Loader.Factory inputLoaderFactory;
private final CodeOwnerMetrics codeOwnerMetrics;
@Inject
@@ -100,7 +98,7 @@
PureRevertCache pureRevertCache,
Provider<CodeOwnerConfigHierarchy> codeOwnerConfigHierarchyProvider,
Provider<CodeOwnerResolver> codeOwnerResolverProvider,
- ApprovalsUtil approvalsUtil,
+ CodeOwnerApprovalCheckInput.Loader.Factory codeOwnerApprovalCheckInputLoaderFactory,
CodeOwnerMetrics codeOwnerMetrics) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
@@ -108,7 +106,7 @@
this.pureRevertCache = pureRevertCache;
this.codeOwnerConfigHierarchyProvider = codeOwnerConfigHierarchyProvider;
this.codeOwnerResolverProvider = codeOwnerResolverProvider;
- this.approvalsUtil = approvalsUtil;
+ this.inputLoaderFactory = codeOwnerApprovalCheckInputLoaderFactory;
this.codeOwnerMetrics = codeOwnerMetrics;
}
@@ -315,7 +313,6 @@
CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(changeNotes.getProjectName());
- Account.Id changeOwner = changeNotes.getChange().getOwner();
Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
ImmutableSet<Account.Id> exemptedAccounts = codeOwnersConfig.getExemptedAccounts();
logger.atFine().log("exemptedAccounts = %s", exemptedAccounts);
@@ -342,38 +339,6 @@
"change is a pure revert and is exempted from requiring code owner approvals");
}
- boolean implicitApprovalConfig = codeOwnersConfig.areImplicitApprovalsEnabled();
- boolean enableImplicitApproval =
- implicitApprovalConfig && changeOwner.equals(patchSetUploader);
- logger.atFine().log(
- "changeOwner = %d, patchSetUploader = %d, implict approval config = %s\n"
- + "=> implicit approval is %s",
- changeOwner.get(),
- patchSetUploader.get(),
- implicitApprovalConfig,
- enableImplicitApproval ? "enabled" : "disabled");
-
- ImmutableList<PatchSetApproval> currentPatchSetApprovals =
- getCurrentPatchSetApprovals(changeNotes);
-
- RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
- logger.atFine().log("requiredApproval = %s", requiredApproval);
-
- ImmutableSet<RequiredApproval> overrideApprovals = codeOwnersConfig.getOverrideApprovals();
- ImmutableSet<PatchSetApproval> overrides =
- getOverride(currentPatchSetApprovals, overrideApprovals, patchSetUploader);
- logger.atFine().log(
- "hasOverride = %s (overrideApprovals = %s, overrides = %s)",
- !overrides.isEmpty(),
- overrideApprovals.stream()
- .map(
- overrideApproval ->
- String.format(
- "%s (ignoreSelfApproval = %s)",
- overrideApproval, overrideApproval.labelType().isIgnoreSelfApproval()))
- .collect(toImmutableList()),
- overrides);
-
BranchNameKey branch = changeNotes.getChange().getDest();
Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
if (revision.isPresent()) {
@@ -383,17 +348,8 @@
logger.atFine().log("dest branch %s does not exist", branch.branch());
}
- CodeOwnerResolverResult globalCodeOwners =
- codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
- logger.atFine().log("global code owners = %s", globalCodeOwners);
-
- ImmutableSet<Account.Id> reviewerAccountIds =
- getReviewerAccountIds(requiredApproval, changeNotes, patchSetUploader);
- ImmutableSet<Account.Id> approverAccountIds =
- getApproverAccountIds(currentPatchSetApprovals, requiredApproval, patchSetUploader);
- logger.atFine().log("reviewers = %s, approvers = %s", reviewerAccountIds, approverAccountIds);
-
- FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
+ CodeOwnerApprovalCheckInput input =
+ inputLoaderFactory.create(codeOwnersConfig, codeOwnerResolver, changeNotes).load();
return changedFiles
.getFromDiffCache(
@@ -406,14 +362,8 @@
codeOwnerResolver,
branch,
revision.orElse(null),
- globalCodeOwners,
- enableImplicitApproval ? changeOwner : null,
- reviewerAccountIds,
- approverAccountIds,
- fallbackCodeOwners,
- overrides,
changedFile,
- /* checkAllOwners= */ false));
+ input));
}
}
@@ -470,8 +420,10 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy = codeOwnerConfigHierarchyProvider.get();
CodeOwnerResolver codeOwnerResolver =
codeOwnerResolverProvider.get().enforceVisibility(false);
- return changedFiles
- .getFromDiffCache(changeNotes.getProjectName(), patchSet.commitId())
+ CodeOwnerApprovalCheckInput input =
+ CodeOwnerApprovalCheckInput.createForComputingOwnedPaths(
+ codeOwnersConfig, codeOwnerResolver, changeNotes, accountIds);
+ return changedFiles.getFromDiffCache(changeNotes.getProjectName(), patchSet.commitId())
.stream()
.map(
changedFile ->
@@ -480,20 +432,8 @@
codeOwnerResolver,
branch,
revision.orElse(null),
- /* globalCodeOwners= */ codeOwnerResolver.resolveGlobalCodeOwners(
- changeNotes.getProjectName()),
- // Do not check for implicit approvals since implicit approvals of other users
- // should be ignored. For the given account we do not need to check for
- // implicit approvals since all owned files are already covered by the
- // explicit approval.
- /* implicitApprover= */ null,
- /* reviewerAccountIds= */ ImmutableSet.of(),
- // Assume an explicit approval of the owners we want to check.
- /* approverAccountIds= */ accountIds,
- fallbackCodeOwners,
- /* overrides= */ ImmutableSet.of(),
changedFile,
- /* checkAllOwners= */ true));
+ input));
}
}
@@ -538,14 +478,8 @@
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
@Nullable ObjectId revision,
- CodeOwnerResolverResult globalCodeOwners,
- @Nullable Account.Id implicitApprover,
- ImmutableSet<Account.Id> reviewerAccountIds,
- ImmutableSet<Account.Id> approverAccountIds,
- FallbackCodeOwners fallbackCodeOwners,
- ImmutableSet<PatchSetApproval> overrides,
ChangedFile changedFile,
- boolean checkAllOwners) {
+ CodeOwnerApprovalCheckInput input) {
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatus.start()) {
logger.atFine().log("computing file status for %s", changedFile);
@@ -560,14 +494,8 @@
codeOwnerResolver,
branch,
revision,
- globalCodeOwners,
- implicitApprover,
- reviewerAccountIds,
- approverAccountIds,
- fallbackCodeOwners,
- overrides,
newPath,
- checkAllOwners));
+ input));
// Compute the code owner status for the old path, if the file was deleted or renamed.
Optional<PathCodeOwnerStatus> oldPathStatus = Optional.empty();
@@ -584,14 +512,8 @@
codeOwnerResolver,
branch,
revision,
- globalCodeOwners,
- implicitApprover,
- reviewerAccountIds,
- approverAccountIds,
- fallbackCodeOwners,
- overrides,
changedFile.oldPath().get(),
- checkAllOwners));
+ input));
}
FileCodeOwnerStatus fileCodeOwnerStatus =
@@ -606,21 +528,15 @@
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
@Nullable ObjectId revision,
- CodeOwnerResolverResult globalCodeOwners,
- @Nullable Account.Id implicitApprover,
- ImmutableSet<Account.Id> reviewerAccountIds,
- ImmutableSet<Account.Id> approverAccountIds,
- FallbackCodeOwners fallbackCodeOwners,
- ImmutableSet<PatchSetApproval> overrides,
Path absolutePath,
- boolean checkAllOwners) {
+ CodeOwnerApprovalCheckInput input) {
logger.atFine().log("computing path status for %s", absolutePath);
- if (!overrides.isEmpty()) {
+ if (!input.overrides().isEmpty()) {
logger.atFine().log(
"the status for path %s is %s since an override is present (overrides = %s)",
- absolutePath, CodeOwnerStatus.APPROVED.name(), overrides);
- Optional<PatchSetApproval> override = overrides.stream().findAny();
+ absolutePath, CodeOwnerStatus.APPROVED.name(), input.overrides());
+ Optional<PatchSetApproval> override = input.overrides().stream().findAny();
checkState(override.isPresent(), "no override found");
return PathCodeOwnerStatus.create(
absolutePath,
@@ -638,35 +554,36 @@
boolean isGloballyApproved =
isApproved(
- globalCodeOwners,
+ input.globalCodeOwners(),
CodeOwnerKind.GLOBAL_CODE_OWNER,
- approverAccountIds,
- implicitApprover,
+ input.approvers(),
+ input.implicitApprover().orElse(null),
reason);
if (isGloballyApproved) {
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
}
- if (globalCodeOwners.ownedByAllUsers()) {
- activeOwners.addAll(approverAccountIds);
- activeOwners.addAll(reviewerAccountIds);
+ if (input.globalCodeOwners().ownedByAllUsers()) {
+ activeOwners.addAll(input.approvers());
+ activeOwners.addAll(input.reviewers());
} else {
activeOwners.addAll(
- Sets.intersection(globalCodeOwners.codeOwnersAccountIds(), approverAccountIds));
+ Sets.intersection(input.globalCodeOwners().codeOwnersAccountIds(), input.approvers()));
activeOwners.addAll(
- Sets.intersection(globalCodeOwners.codeOwnersAccountIds(), reviewerAccountIds));
+ Sets.intersection(input.globalCodeOwners().codeOwnersAccountIds(), input.reviewers()));
}
// Only check recursively for all OWNERs in two scenarios:
// 1. The path was not globally approved
// 2. The path was globally approved but is not owned by all users and we
// want to calculate all ownerIds.
- if (!isGloballyApproved || (checkAllOwners && !globalCodeOwners.ownedByAllUsers())) {
+ if (!isGloballyApproved
+ || (input.checkAllOwners() && !input.globalCodeOwners().ownedByAllUsers())) {
logger.atFine().log("%s was not approved by a global code owner", absolutePath);
if (isPending(
- globalCodeOwners, CodeOwnerKind.GLOBAL_CODE_OWNER, reviewerAccountIds, reason)) {
+ input.globalCodeOwners(), CodeOwnerKind.GLOBAL_CODE_OWNER, input.reviewers(), reason)) {
codeOwnerStatus.set(CodeOwnerStatus.PENDING);
}
@@ -688,13 +605,13 @@
boolean ownedByAllUsers = codeOwners.ownedByAllUsers();
if (ownedByAllUsers) {
- activeOwners.addAll(approverAccountIds);
- activeOwners.addAll(reviewerAccountIds);
+ activeOwners.addAll(input.approvers());
+ activeOwners.addAll(input.reviewers());
} else {
activeOwners.addAll(
- Sets.intersection(codeOwners.codeOwnersAccountIds(), approverAccountIds));
+ Sets.intersection(codeOwners.codeOwnersAccountIds(), input.approvers()));
activeOwners.addAll(
- Sets.intersection(codeOwners.codeOwnersAccountIds(), reviewerAccountIds));
+ Sets.intersection(codeOwners.codeOwnersAccountIds(), input.reviewers()));
}
logger.atFine().log(
"code owners = %s (code owner kind = %s, code owner config folder path = %s,"
@@ -709,12 +626,16 @@
}
if (isApproved(
- codeOwners, codeOwnerKind, approverAccountIds, implicitApprover, reason)) {
+ codeOwners,
+ codeOwnerKind,
+ input.approvers(),
+ input.implicitApprover().orElse(null),
+ reason)) {
codeOwnerStatus.set(CodeOwnerStatus.APPROVED);
// No need to recurse if we are not checking all owners or all owners are
// are already added.
- return checkAllOwners && !ownedByAllUsers;
- } else if (isPending(codeOwners, codeOwnerKind, reviewerAccountIds, reason)) {
+ return input.checkAllOwners() && !ownedByAllUsers;
+ } else if (isPending(codeOwners, codeOwnerKind, input.reviewers(), reason)) {
codeOwnerStatus.set(CodeOwnerStatus.PENDING);
// We need to continue to check if any of the higher-level code owners approved
@@ -742,25 +663,25 @@
CodeOwnerStatus codeOwnerStatusForFallbackCodeOwners =
getCodeOwnerStatusForFallbackCodeOwners(
codeOwnerStatus.get(),
- implicitApprover,
- reviewerAccountIds,
- approverAccountIds,
- fallbackCodeOwners,
+ input.implicitApprover().orElse(null),
+ input.reviewers(),
+ input.approvers(),
+ input.fallbackCodeOwners(),
absolutePath,
reason);
if (codeOwnerStatusForFallbackCodeOwners.equals(CodeOwnerStatus.APPROVED)) {
- activeOwners.addAll(approverAccountIds);
+ activeOwners.addAll(input.approvers());
} else if (codeOwnerStatusForFallbackCodeOwners.equals(CodeOwnerStatus.PENDING)) {
- switch (fallbackCodeOwners) {
+ switch (input.fallbackCodeOwners()) {
case NONE:
// do nothing, if codeOwnerStatus is PENDING, the reviewers that are code owners have
// already been added to activeOwners
break;
case ALL_USERS:
// all users are code owners
- activeOwners.addAll(approverAccountIds);
- activeOwners.addAll(reviewerAccountIds);
+ activeOwners.addAll(input.approvers());
+ activeOwners.addAll(input.reviewers());
break;
}
}
@@ -797,7 +718,7 @@
absolutePath,
codeOwnerStatus.get(),
reason.get(),
- checkAllOwners ? Optional.of(activeOwners.build()) : Optional.empty());
+ input.checkAllOwners() ? Optional.of(activeOwners.build()) : Optional.empty());
logger.atFine().log("pathCodeOwnerStatus = %s", pathCodeOwnerStatus);
return pathCodeOwnerStatus;
}
@@ -985,111 +906,6 @@
}
/**
- * Gets the IDs of the accounts that are reviewer on the given change.
- *
- * @param changeNotes the change notes
- */
- private ImmutableSet<Account.Id> getReviewerAccountIds(
- RequiredApproval requiredApproval, ChangeNotes changeNotes, Account.Id patchSetUploader) {
- ImmutableSet<Account.Id> reviewerAccountIds =
- changeNotes.getReviewers().byState(ReviewerStateInternal.REVIEWER);
- if (requiredApproval.labelType().isIgnoreSelfApproval()
- && reviewerAccountIds.contains(patchSetUploader)) {
- logger.atFine().log(
- "Removing patch set uploader %s from reviewers since the label of the required"
- + " approval (%s) is configured to ignore self approvals",
- patchSetUploader, requiredApproval.labelType());
- return filterOutAccount(reviewerAccountIds, patchSetUploader);
- }
- return reviewerAccountIds;
- }
-
- /**
- * Gets the IDs of the accounts that posted a patch set approval on the given revisions that
- * counts as code owner approval.
- *
- * @param requiredApproval approval that is required from code owners to approve the files in a
- * change
- */
- private ImmutableSet<Account.Id> getApproverAccountIds(
- ImmutableList<PatchSetApproval> currentPatchSetApprovals,
- RequiredApproval requiredApproval,
- Account.Id patchSetUploader) {
- ImmutableSet<Account.Id> approverAccountIds =
- currentPatchSetApprovals.stream()
- .filter(requiredApproval::isApprovedBy)
- .map(PatchSetApproval::accountId)
- .collect(toImmutableSet());
-
- if (requiredApproval.labelType().isIgnoreSelfApproval()
- && approverAccountIds.contains(patchSetUploader)) {
- logger.atFine().log(
- "Removing patch set uploader %s from approvers since the label of the required"
- + " approval (%s) is configured to ignore self approvals",
- patchSetUploader, requiredApproval.labelType());
- return filterOutAccount(approverAccountIds, patchSetUploader);
- }
-
- return approverAccountIds;
- }
-
- private ImmutableList<PatchSetApproval> getCurrentPatchSetApprovals(ChangeNotes changeNotes) {
- try (Timer0.Context ctx = codeOwnerMetrics.computePatchSetApprovals.start()) {
- return ImmutableList.copyOf(
- approvalsUtil.byPatchSet(changeNotes, changeNotes.getCurrentPatchSet().id()));
- }
- }
-
- private ImmutableSet<Account.Id> filterOutAccount(
- ImmutableSet<Account.Id> accountIds, Account.Id accountIdToFilterOut) {
- return accountIds.stream()
- .filter(accountId -> !accountId.equals(accountIdToFilterOut))
- .collect(toImmutableSet());
- }
-
- /**
- * Gets the overrides that were applied on the change.
- *
- * @param overrideApprovals approvals that count as override for the code owners submit check.
- * @param patchSetUploader account ID of the patch set uploader
- * @return the overrides that were applied on the change
- */
- private ImmutableSet<PatchSetApproval> getOverride(
- ImmutableList<PatchSetApproval> currentPatchSetApprovals,
- ImmutableSet<RequiredApproval> overrideApprovals,
- Account.Id patchSetUploader) {
- ImmutableSet<RequiredApproval> overrideApprovalsThatIgnoreSelfApprovals =
- overrideApprovals.stream()
- .filter(overrideApproval -> overrideApproval.labelType().isIgnoreSelfApproval())
- .collect(toImmutableSet());
- return currentPatchSetApprovals.stream()
- .filter(
- approval -> {
- // If the approval is from the patch set uploader and if it matches any of the labels
- // for which self approvals are ignored, filter it out.
- if (approval.accountId().equals(patchSetUploader)
- && overrideApprovalsThatIgnoreSelfApprovals.stream()
- .anyMatch(
- requiredApproval ->
- requiredApproval
- .labelType()
- .getLabelId()
- .equals(approval.key().labelId()))) {
- logger.atFine().log(
- "Filtered out self-override %s of patch set uploader",
- LabelVote.create(approval.label(), approval.value()));
- return false;
- }
- return true;
- })
- .filter(
- patchSetApproval ->
- overrideApprovals.stream()
- .anyMatch(overrideApproval -> overrideApproval.isApprovedBy(patchSetApproval)))
- .collect(toImmutableSet());
- }
-
- /**
* Gets the current revision of the destination branch of the given change.
*
* <p>This is the revision from which the code owner configs should be read when computing code
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
new file mode 100644
index 0000000..cd2ef0e
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckInput.java
@@ -0,0 +1,361 @@
+// Copyright (C) 2022 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;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.metrics.Timer0;
+import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
+import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
+import com.google.gerrit.server.approval.ApprovalsUtil;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.util.LabelVote;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.util.Optional;
+
+/**
+ * Input for {@link CodeOwnerApprovalCheck}.
+ *
+ * <p>Provides all data that is needed to check the code owner approvals on a change.
+ */
+@AutoValue
+public abstract class CodeOwnerApprovalCheckInput {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ /**
+ * Gets the IDs of the accounts of all reviewers that can possibly code owner approve the change
+ * (if they are code owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out since in this case the
+ * patch set uploader cannot approve the change even if they are a code owner.
+ */
+ public abstract ImmutableSet<Account.Id> reviewers();
+
+ /**
+ * Gets the IDs of the accounts that have an approval on the current patch set that possibly
+ * counts as code owner approval (if they are code owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out since in this case the
+ * approval of the patch set uploader is ignored even if they are a code owner.
+ */
+ public abstract ImmutableSet<Account.Id> approvers();
+
+ /**
+ * Account from which an implicit code owner approval should be assumed.
+ *
+ * @see CodeOwnersPluginProjectConfigSnapshot#areImplicitApprovalsEnabled()
+ * @return the account of the change owner if implicit approvals are enabled, otherwise {@link
+ * Optional#empty()}
+ */
+ public abstract Optional<Account.Id> implicitApprover();
+
+ /**
+ * Gets the approvals from the current patch set that count as code owner overrides.
+ *
+ * <p>If self approvals are ignored an override of the patch set uploader is filtered out since it
+ * doesn't count as code owner override.
+ */
+ public abstract ImmutableSet<PatchSetApproval> overrides();
+
+ /** Gets the configured global code owners. */
+ public abstract CodeOwnerResolverResult globalCodeOwners();
+
+ /** Gets the policy that defines who owns paths for which no code owners are defined. */
+ public abstract FallbackCodeOwners fallbackCodeOwners();
+
+ /**
+ * Whether all code owners should be checked. *
+ *
+ * <p>If {@code true} {@link PathCodeOwnerStatus#owners()} are expected to be set in {@link
+ * PathCodeOwnerStatus} instances that are created by {@link CodeOwnerApprovalCheck}.
+ */
+ public abstract boolean checkAllOwners();
+
+ /**
+ * Creates a {@link CodeOwnerApprovalCheckInput} instance for computing the paths in a change that
+ * are owned by the given accounts.
+ *
+ * @param codeOwnersConfig the code-owners plugin configuration
+ * @param codeOwnerResolver the {@link CodeOwnerResolver} that should be used to resolve the
+ * configured global code owners
+ * @param changeNotes the notes of the change for which owned paths should be computed
+ * @param accounts the accounts for which the owned paths should be computed
+ * @return the created {@link CodeOwnerApprovalCheckInput} instance
+ */
+ public static CodeOwnerApprovalCheckInput createForComputingOwnedPaths(
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ CodeOwnerResolver codeOwnerResolver,
+ ChangeNotes changeNotes,
+ ImmutableSet<Account.Id> accounts) {
+ CodeOwnerResolverResult globalCodeOwners =
+ codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
+ logger.atFine().log("global code owners = %s", globalCodeOwners);
+
+ FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
+ logger.atFine().log("fallbackCodeOwner = %s", fallbackCodeOwners);
+
+ return create(
+ /* reviewers= */ ImmutableSet.of(),
+ /* approvers= */ accounts,
+ // Do not check for implicit approvals since implicit approvals of other users
+ // should be ignored. For the given account we do not need to check for
+ // implicit approvals since all owned files are already covered by the
+ // explicit approval.
+ /* implicitApprover= */ Optional.empty(),
+ /* overrides= */ ImmutableSet.of(),
+ globalCodeOwners,
+ fallbackCodeOwners,
+ /* checkAllOwners= */ true);
+ }
+
+ /**
+ * Creates a {@link CodeOwnerApprovalCheckInput} instance.
+ *
+ * @param reviewers the reviewers that can possibly code owner approve the change (if they are
+ * code owners)
+ * @param approvers the accounts that have an approval on the current patch set that possibly
+ * counts as code owner approval (if they are code owners)
+ * @param implicitApprover account from which an implicit code owner approval should be assumed
+ * @param overrides the approvals from the current patch set that count as code owner overrides
+ * @param globalCodeOwners the configured global code owners
+ * @param fallbackCodeOwners the policy that defines who owns paths for which no code owners are
+ * defined
+ * @param checkAllOwners Whether all code owners are checked. If {@code true} {@link
+ * PathCodeOwnerStatus#owners()} will be set in the the {@link PathCodeOwnerStatus} instances
+ * that are created by {@link CodeOwnerApprovalCheck}. Checking all owners means that no
+ * shortcuts can be applied, hence checking the code owner approvals with {@code
+ * checkAllOwners=true} is more expensive.
+ * @return the created {@link CodeOwnerApprovalCheckInput} instance
+ */
+ private static CodeOwnerApprovalCheckInput create(
+ ImmutableSet<Account.Id> reviewers,
+ ImmutableSet<Account.Id> approvers,
+ Optional<Account.Id> implicitApprover,
+ ImmutableSet<PatchSetApproval> overrides,
+ CodeOwnerResolverResult globalCodeOwners,
+ FallbackCodeOwners fallbackCodeOwners,
+ boolean checkAllOwners) {
+ return new AutoValue_CodeOwnerApprovalCheckInput(
+ reviewers,
+ approvers,
+ implicitApprover,
+ overrides,
+ globalCodeOwners,
+ fallbackCodeOwners,
+ checkAllOwners);
+ }
+
+ /**
+ * Class to load all inputs that are required for checking the code owner approvals on a change.
+ */
+ public static class Loader {
+ /** Factory to create the {@link Loader} with injected dependencies. */
+ interface Factory {
+ Loader create(
+ CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ CodeOwnerResolver codeOwnerResolver,
+ ChangeNotes changeNotes);
+ }
+
+ private final ApprovalsUtil approvalsUtil;
+ private final CodeOwnerMetrics codeOwnerMetrics;
+ private final CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig;
+ private final CodeOwnerResolver codeOwnerResolver;
+ private final ChangeNotes changeNotes;
+
+ @Inject
+ Loader(
+ ApprovalsUtil approvalsUtil,
+ CodeOwnerMetrics codeOwnerMetrics,
+ @Assisted CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig,
+ @Assisted CodeOwnerResolver codeOwnerResolver,
+ @Assisted ChangeNotes changeNotes) {
+ this.approvalsUtil = approvalsUtil;
+ this.codeOwnerMetrics = codeOwnerMetrics;
+ this.codeOwnersConfig = codeOwnersConfig;
+ this.codeOwnerResolver = codeOwnerResolver;
+ this.changeNotes = changeNotes;
+ }
+
+ CodeOwnerApprovalCheckInput load() {
+ logger.atFine().log(
+ "requiredApproval = %s, overrideApprovals = %s",
+ codeOwnersConfig.getRequiredApproval().formatForLogging(),
+ RequiredApproval.formatForLogging(codeOwnersConfig.getOverrideApprovals()));
+ return CodeOwnerApprovalCheckInput.create(
+ getReviewers(),
+ getApprovers(),
+ getImplicitApprover(),
+ getOverrides(),
+ getGlobalCodeOwners(),
+ getFallbackCodeOwners(),
+ /* checkAllOwners= */ false);
+ }
+
+ /**
+ * Gets the IDs of the accounts of all reviewers that can possibly code owner approve the change
+ * (if they are code owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out since in this case
+ * the patch set uploader cannot approve the change even if they are a code owner.
+ */
+ private ImmutableSet<Account.Id> getReviewers() {
+ ImmutableSet<Account.Id> reviewerAccountIds =
+ changeNotes.getReviewers().byState(ReviewerStateInternal.REVIEWER);
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+ Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
+ if (requiredApproval.labelType().isIgnoreSelfApproval()
+ && reviewerAccountIds.contains(patchSetUploader)) {
+ logger.atFine().log(
+ "Removing patch set uploader %s from reviewers since the label of the required"
+ + " approval (%s) is configured to ignore self approvals",
+ patchSetUploader, requiredApproval.labelType());
+ return filterOutAccount(reviewerAccountIds, patchSetUploader);
+ }
+ logger.atFine().log("reviewers = %s", reviewerAccountIds);
+ return reviewerAccountIds;
+ }
+
+ /**
+ * Gets the IDs of the accounts that have an approval on the current patch set that possibly
+ * counts as code owner approval (if they are code owners).
+ *
+ * <p>If self approvals are ignored the patch set uploader is filtered out since in this case
+ * the approval of the patch set uploader is ignored even if they are a code owner.
+ */
+ private ImmutableSet<Account.Id> getApprovers() {
+ ImmutableList<PatchSetApproval> currentPatchSetApprovals =
+ getCurrentPatchSetApprovals(changeNotes);
+ RequiredApproval requiredApproval = codeOwnersConfig.getRequiredApproval();
+ ImmutableSet<Account.Id> approverAccountIds =
+ currentPatchSetApprovals.stream()
+ .filter(requiredApproval::isApprovedBy)
+ .map(PatchSetApproval::accountId)
+ .collect(toImmutableSet());
+ Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
+ if (requiredApproval.labelType().isIgnoreSelfApproval()
+ && approverAccountIds.contains(patchSetUploader)) {
+ logger.atFine().log(
+ "Removing patch set uploader %s from approvers since the label of the required"
+ + " approval (%s) is configured to ignore self approvals",
+ patchSetUploader, requiredApproval.labelType());
+ return filterOutAccount(approverAccountIds, patchSetUploader);
+ }
+ logger.atFine().log("approvers = %s", approverAccountIds);
+ return approverAccountIds;
+ }
+
+ private Optional<Account.Id> getImplicitApprover() {
+ Account.Id changeOwner = changeNotes.getChange().getOwner();
+ Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
+ boolean implicitApprovalConfig = codeOwnersConfig.areImplicitApprovalsEnabled();
+ boolean enableImplicitApproval =
+ implicitApprovalConfig && changeOwner.equals(patchSetUploader);
+ logger.atFine().log(
+ "changeOwner = %d, patchSetUploader = %d, implict approval config = %s\n"
+ + "=> implicit approval is %s",
+ changeOwner.get(),
+ patchSetUploader.get(),
+ implicitApprovalConfig,
+ enableImplicitApproval ? "enabled" : "disabled");
+ return enableImplicitApproval ? Optional.of(changeOwner) : Optional.empty();
+ }
+
+ /**
+ * Gets the approvals from the current patch set that count as code owner overrides.
+ *
+ * <p>If self approvals are ignored an override of the patch set uploader is filtered out since
+ * it doesn't count as code owner override.
+ */
+ private ImmutableSet<PatchSetApproval> getOverrides() {
+ ImmutableList<PatchSetApproval> currentPatchSetApprovals =
+ getCurrentPatchSetApprovals(changeNotes);
+ ImmutableSortedSet<RequiredApproval> overrideApprovals =
+ codeOwnersConfig.getOverrideApprovals();
+ ImmutableSet<RequiredApproval> overrideApprovalsThatIgnoreSelfApprovals =
+ overrideApprovals.stream()
+ .filter(overrideApproval -> overrideApproval.labelType().isIgnoreSelfApproval())
+ .collect(toImmutableSet());
+ Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader();
+ ImmutableSet<PatchSetApproval> overrides =
+ currentPatchSetApprovals.stream()
+ .filter(
+ approval -> {
+ // If the approval is from the patch set uploader and if it matches any of the
+ // labels
+ // for which self approvals are ignored, filter it out.
+ if (approval.accountId().equals(patchSetUploader)
+ && overrideApprovalsThatIgnoreSelfApprovals.stream()
+ .anyMatch(
+ requiredApproval ->
+ requiredApproval
+ .labelType()
+ .getLabelId()
+ .equals(approval.key().labelId()))) {
+ logger.atFine().log(
+ "Filtered out self-override %s of patch set uploader",
+ LabelVote.create(approval.label(), approval.value()));
+ return false;
+ }
+ return true;
+ })
+ .filter(
+ patchSetApproval ->
+ overrideApprovals.stream()
+ .anyMatch(
+ overrideApproval -> overrideApproval.isApprovedBy(patchSetApproval)))
+ .collect(toImmutableSet());
+ logger.atFine().log("hasOverride = %s (overrides = %s)", !overrides.isEmpty(), overrides);
+ return overrides;
+ }
+
+ private CodeOwnerResolverResult getGlobalCodeOwners() {
+ CodeOwnerResolverResult globalCodeOwners =
+ codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
+ logger.atFine().log("global code owners = %s", globalCodeOwners);
+ return globalCodeOwners;
+ }
+
+ private FallbackCodeOwners getFallbackCodeOwners() {
+ FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
+ logger.atFine().log("fallbackCodeOwners = %s", fallbackCodeOwners);
+ return fallbackCodeOwners;
+ }
+
+ private ImmutableList<PatchSetApproval> getCurrentPatchSetApprovals(ChangeNotes changeNotes) {
+ try (Timer0.Context ctx = codeOwnerMetrics.computePatchSetApprovals.start()) {
+ return ImmutableList.copyOf(
+ approvalsUtil.byPatchSet(changeNotes, changeNotes.getCurrentPatchSet().id()));
+ }
+ }
+
+ private static ImmutableSet<Account.Id> filterOutAccount(
+ ImmutableSet<Account.Id> accountIds, Account.Id accountIdToFilterOut) {
+ return accountIds.stream()
+ .filter(accountId -> !accountId.equals(accountIdToFilterOut))
+ .collect(toImmutableSet());
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 5b2ab1c..89fc56e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -580,7 +580,7 @@
/**
* Checks whether implicit code owner approvals are enabled.
*
- * <p>If enabled, an implict code owner approval from the change owner is assumed if the last
+ * <p>If enabled, an implicit code owner approval from the change owner is assumed if the last
* patch set was uploaded by the change owner.
*/
public boolean areImplicitApprovalsEnabled() {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java
index 4cbdba0..9dd283d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
@@ -25,6 +26,7 @@
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.server.project.ProjectState;
+import java.util.Collection;
import java.util.Optional;
/**
@@ -69,6 +71,18 @@
return labelType().getName() + "+" + value();
}
+ public final String formatForLogging() {
+ return String.format(
+ "%s (ignoreSelfApproval = %s)", toString(), labelType().isIgnoreSelfApproval());
+ }
+
+ public static String formatForLogging(Collection<RequiredApproval> requiredApprovals) {
+ return requiredApprovals.stream()
+ .map(RequiredApproval::formatForLogging)
+ .collect(toImmutableList())
+ .toString();
+ }
+
/**
* Parses a string-representation of a {@link RequiredApproval}.
*
