CodeOwnerApprovalCheck: Factor out computation of input data
The CodeOwnerApprovalCheck class is quite large which makes the class
difficult to read. To make it smaller we factor out the computation of
all input data that this required for checking the code owner approvals
of a change. The input data is now represented by the new
CodeOwnerApprovalCheckInput class which is an immutable AutoValue class.
The loading of the input is done by the
CodeOwnerApprovalCheckInput.Loader class (e.g. this class computes the
relevant reviewers and approvers, loads the global code owners and
fallback code owners from the config etc.). The Loader class needs to
get some classes injected which is why we have a Factory to create it.
Having a class that represents all inputs, make it easier for
CodeOwnerApprovalCheck to pass down the input through the methods that
do the computation (just pass 1 parameter instead of n parameters).
In addition to checking code owner approvals on a change
CodeOwnerApprovalCheck also computes owned paths for a set of users.
This is achieved by using the same algorithm, but with specially
constructed inputs. By having the CodeOwnerApprovalCheckInput class we
can make the creation of the input for this purpose more explicit.
In future we need additional inputs for the code owner approval check
(e.g. approvals on previous patch sets for making code owner approvals
sticky on file level). Adding these additional inputs is easier now that
we have CodeOwnerApprovalCheckInput, otherwise we would need to add even
more logic to the already large CodeOwnerApprovalCheck class.
This change is a pure refactoring. This means no behavior is changed.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I298f8e3b6acd7afdec900e1c1fde6d1f26ed1a06
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}.
*
