Merge "CodeOwnerApprovalCheck: Allow to check for an account which paths it owns"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 26d67f0..7b83265 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
@@ -230,12 +231,99 @@
}
}
+ /**
+ * Gets the code owner status for all files/paths that were changed in the current revision of the
+ * given change assuming that there is only an approval from the given account.
+ *
+ * <p>This method doesn't take approvals from other users and global code owners into account.
+ *
+ * <p>The purpose of this method is to find the files/paths in a change that are owned by the
+ * given account.
+ *
+ * @param changeNotes the notes of the change for which the current code owner statuses should be
+ * returned
+ * @param accountId the ID of the account for which an approval should be assumed
+ */
+ public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
+ ChangeNotes changeNotes, Account.Id accountId)
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
+ requireNonNull(changeNotes, "changeNotes");
+ requireNonNull(accountId, "accountId");
+ try (TraceTimer traceTimer =
+ TraceContext.newTimer(
+ "Compute file statuses for account",
+ Metadata.builder()
+ .projectName(changeNotes.getProjectName().get())
+ .changeId(changeNotes.getChangeId().get())
+ .build())) {
+ RequiredApproval requiredApproval =
+ codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName());
+ logger.atFine().log("requiredApproval = %s", requiredApproval);
+
+ BranchNameKey branch = changeNotes.getChange().getDest();
+ ObjectId revision = getDestBranchRevision(changeNotes.getChange());
+ logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+
+ // If the branch doesn't contain any code owner config file yet, we apply special logic
+ // (project owners count as code owners) to allow bootstrapping the code owner configuration
+ // in the branch.
+ boolean isBootstrapping =
+ !codeOwnerConfigScannerFactory.create().containsAnyCodeOwnerConfigFile(branch);
+ boolean isProjectOwner = isProjectOwner(changeNotes.getProjectName(), accountId);
+ logger.atFine().log(
+ "isBootstrapping = %s (isProjectOwner = %s)", isBootstrapping, isProjectOwner);
+ if (isBootstrapping && isProjectOwner) {
+ // Return all paths as approved.
+ return changedFiles
+ .compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .stream()
+ .map(
+ changedFile ->
+ FileCodeOwnerStatus.create(
+ changedFile,
+ changedFile
+ .newPath()
+ .map(
+ newPath ->
+ PathCodeOwnerStatus.create(newPath, CodeOwnerStatus.APPROVED)),
+ changedFile
+ .oldPath()
+ .map(
+ oldPath ->
+ PathCodeOwnerStatus.create(
+ oldPath, CodeOwnerStatus.APPROVED))));
+ }
+
+ return changedFiles
+ .compute(changeNotes.getProjectName(), changeNotes.getCurrentPatchSet().commitId())
+ .stream()
+ .map(
+ changedFile ->
+ getFileStatus(
+ branch,
+ revision,
+ /* globalCodeOwners= */ CodeOwnerResolverResult.createEmpty(),
+ // 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.
+ /* enableImplicitApprovalFromUploader= */ false,
+ /* patchSetUploader= */ null,
+ /* reviewerAccountIds= */ ImmutableSet.of(),
+ // Assume an explicit approval of the given account.
+ /* approverAccountIds= */ ImmutableSet.of(accountId),
+ /* hasOverride= */ false,
+ /* isBootstrapping= */ false,
+ changedFile));
+ }
+ }
+
private FileCodeOwnerStatus getFileStatus(
BranchNameKey branch,
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
boolean hasOverride,
@@ -294,7 +382,7 @@
ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
boolean hasOverride,
@@ -340,7 +428,7 @@
BranchNameKey branch,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
Path absolutePath) {
@@ -386,7 +474,7 @@
CodeOwnerResolverResult globalCodeOwners,
ImmutableSet<Account.Id> approverAccountIds,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader) {
+ @Nullable Account.Id patchSetUploader) {
return (enableImplicitApprovalFromUploader
&& isImplicitlyApprovedBootstrappingMode(
projectName, absolutePath, globalCodeOwners, patchSetUploader))
@@ -399,6 +487,8 @@
Path absolutePath,
CodeOwnerResolverResult globalCodeOwners,
Account.Id patchSetUploader) {
+ requireNonNull(
+ patchSetUploader, "patchSetUploader must be set if implicit approvals are enabled");
if (isProjectOwner(projectName, patchSetUploader)) {
// The uploader of the patch set is a project owner and thus a code owner. This means there
// is an implicit code owner approval from the patch set uploader so that the path is
@@ -473,7 +563,7 @@
BranchNameKey branch,
CodeOwnerResolverResult globalCodeOwners,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader,
+ @Nullable Account.Id patchSetUploader,
ObjectId revision,
ImmutableSet<Account.Id> reviewerAccountIds,
ImmutableSet<Account.Id> approverAccountIds,
@@ -632,14 +722,17 @@
CodeOwnerResolverResult codeOwners,
ImmutableSet<Account.Id> approverAccountIds,
boolean enableImplicitApprovalFromUploader,
- Account.Id patchSetUploader) {
- if (enableImplicitApprovalFromUploader
- && (codeOwners.codeOwnersAccountIds().contains(patchSetUploader)
- || codeOwners.ownedByAllUsers())) {
- // If the uploader of the patch set owns the path, there is an implicit code owner
- // approval from the patch set uploader so that the path is automatically approved.
- logger.atFine().log("%s was implicitly approved by the patch set uploader", absolutePath);
- return true;
+ @Nullable Account.Id patchSetUploader) {
+ if (enableImplicitApprovalFromUploader) {
+ requireNonNull(
+ patchSetUploader, "patchSetUploader must be set if implicit approvals are enabled");
+ if (codeOwners.codeOwnersAccountIds().contains(patchSetUploader)
+ || codeOwners.ownedByAllUsers()) {
+ // If the uploader of the patch set owns the path, there is an implicit code owner
+ // approval from the patch set uploader so that the path is automatically approved.
+ logger.atFine().log("%s was implicitly approved by the patch set uploader", absolutePath);
+ return true;
+ }
}
if (!Collections.disjoint(approverAccountIds, codeOwners.codeOwnersAccountIds())
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
index a03f0e1..b5b71c0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java
@@ -79,4 +79,13 @@
return new AutoValue_CodeOwnerResolverResult(
codeOwners, ownedByAllUsers, hasUnresolvedCodeOwners, hasUnresolvedImports);
}
+
+ /** Creates a empty {@link CodeOwnerResolverResult} instance. */
+ public static CodeOwnerResolverResult createEmpty() {
+ return new AutoValue_CodeOwnerResolverResult(
+ /* codeOwners= */ ImmutableSet.of(),
+ /* ownedByAllUsers= */ false,
+ /* hasUnresolvedCodeOwners= */ false,
+ /* hasUnresolvedImports= */ false);
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
new file mode 100644
index 0000000..e392e47
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckForAccountTest.java
@@ -0,0 +1,265 @@
+// 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;
+
+import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatStream;
+
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.plugins.codeowners.JgitPath;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
+import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus;
+import com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.inject.Inject;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.stream.Stream;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnerApprovalCheck#getFileStatusesForAccount(ChangeNotes, Account.Id)}. */
+public class CodeOwnerApprovalCheckForAccountTest extends AbstractCodeOwnersTest {
+ @Inject private ChangeNotes.Factory changeNotesFactory;
+ @Inject private RequestScopeOperations requestScopeOperations;
+
+ private CodeOwnerApprovalCheck codeOwnerApprovalCheck;
+ private CodeOwnerConfigOperations codeOwnerConfigOperations;
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ codeOwnerApprovalCheck = plugin.getSysInjector().getInstance(CodeOwnerApprovalCheck.class);
+ codeOwnerConfigOperations =
+ plugin.getSysInjector().getInstance(CodeOwnerConfigOperations.class);
+ }
+
+ @Test
+ public void notApprovedByUser() throws Exception {
+ // create arbitrary code owner config to avoid entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+ createArbitraryCodeOwnerConfigFile();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would not be approved by the user.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void approvalFromOtherCodeOwnerHasNoEffect() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(codeOwner.email())
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Add a Code-Review+1 (= code owner approval) from the code owner.
+ requestScopeOperations.setApiUser(codeOwner.id());
+ recommend(changeId);
+
+ // Verify that the file would not be approved by the user.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void approvedByUser() throws Exception {
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the user.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @Test
+ public void notApprovedByUser_bootstrapping() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would not be approved by the user since the user is not a project owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @Test
+ public void approvedByProjectOwner_bootstrapping() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the 'admin' user since the 'admin' user is a
+ // project owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), admin.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @Test
+ public void approvedByFallbackCodeOwner() throws Exception {
+ // create arbitrary code owner config to avoid entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+ createArbitraryCodeOwnerConfigFile();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the user since the user is a fallback code owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @Test
+ public void notApprovedByFallbackCodeOwnerIfCodeOwerIsDefined() throws Exception {
+ TestAccount codeOwner =
+ accountCreator.create(
+ "codeOwner", "codeOwner@example.com", "CodeOwner", /* displayName= */ null);
+
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(codeOwner.email())
+ .create();
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would not be approved by the user since fallback code owners do not
+ // apply.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.INSUFFICIENT_REVIEWERS);
+ }
+
+ @GerritConfig(name = "plugin.code-owners.fallbackCodeOwners", value = "ALL_USERS")
+ @Test
+ public void approvedByFallbackCodeOwner_bootstrappingMode() throws Exception {
+ // since no code owner config exists we are entering the bootstrapping code path in
+ // CodeOwnerApprovalCheck
+
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange("Change Adding A File", JgitPath.of(path).get(), "file content").getChangeId();
+
+ // Verify that the file would be approved by the user since the user is a fallback code owner.
+ Stream<FileCodeOwnerStatus> fileCodeOwnerStatuses =
+ codeOwnerApprovalCheck.getFileStatusesForAccount(getChangeNotes(changeId), user.id());
+ FileCodeOwnerStatusSubject fileCodeOwnerStatusSubject =
+ assertThatStream(fileCodeOwnerStatuses).onlyElement();
+ fileCodeOwnerStatusSubject.hasNewPathStatus().value().hasPathThat().isEqualTo(path);
+ fileCodeOwnerStatusSubject
+ .hasNewPathStatus()
+ .value()
+ .hasStatusThat()
+ .isEqualTo(CodeOwnerStatus.APPROVED);
+ }
+
+ private ChangeNotes getChangeNotes(String changeId) throws Exception {
+ return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
+ }
+}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 6386a15..f9e79ed 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -52,8 +52,13 @@
import org.junit.Test;
/**
- * Tests for {@link CodeOwnerApprovalCheck}. Further tests with fallback code owners are implemented
- * in {@link CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest}}.
+ * Tests for {@link CodeOwnerApprovalCheck}.
+ *
+ * <p>Further tests with fallback code owners are implemented in {@link
+ * CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest} and the functionality of {@link
+ * CodeOwnerApprovalCheck#getFileStatusesForAccount(ChangeNotes,
+ * com.google.gerrit.entities.Account.Id)} is covered by {@link
+ * CodeOwnerApprovalCheckForAccountTest}.
*/
public class CodeOwnerApprovalCheckTest extends AbstractCodeOwnersTest {
@Inject private ChangeNotes.Factory changeNotesFactory;
