Fix submit rule for changes for which the target branch doesn't exist
There are 2 possibilities to have changes where the target branch
doesn't exist:
1. An initial commit is pushed for review to the not yet created master
branch (that's a hard-coded special case where Gerrit allows
uploading a change for an unborn branch).
2. A change is created for an existing branch and the branch gets
deleted afterwards.
At the moment the code owner submit rule returns not ready in this case.
Since the target branch of the change doesn't exist it cannot contain
OWNERS files that define code owners. However it is possible that there
are default code owners (defined in refs/meta/config), global code
owners (defined in code-owners.config) or fallback code owners
(configured via code-owners.config) that could approve the change. In
addition there can be a code owner override that code-owner-approves the
change.
Now, if the target branch doesn't exist, CodeOwnerApprovalCheck skips
checking code owners from OWNERS files in the target branch, but still
checks default, global and fallback code owners, as well as overrides.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Idfcec9c0dc1601ae4cbd2185203e658a6a65b713
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index b2e2127..97ece59 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -32,7 +32,6 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -120,11 +119,9 @@
* @param start number of owned paths to skip
* @param limit the max number of owned paths that should be returned (0 = unlimited)
* @return the paths of the files in the given patch set that are owned by the specified account
- * @throws ResourceConflictException if the destination branch of the change no longer exists
*/
public ImmutableList<OwnedChangedFile> getOwnedPaths(
- ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit)
- throws ResourceConflictException {
+ ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId, int start, int limit) {
try (Timer0.Context ctx = codeOwnerMetrics.computeOwnedPaths.start()) {
logger.atFine().log(
"compute owned paths for account %d (project = %s, change = %d, patch set = %d,"
@@ -190,7 +187,7 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -235,8 +232,7 @@
* @see #getFileStatuses(CodeOwnerConfigHierarchy, CodeOwnerResolver, ChangeNotes)
*/
public ImmutableSet<FileCodeOwnerStatus> getFileStatusesAsSet(
- ChangeNotes changeNotes, int start, int limit)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ ChangeNotes changeNotes, int start, int limit) throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) {
logger.atFine().log(
@@ -289,7 +285,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (Timer0.Context ctx = codeOwnerMetrics.prepareFileStatusComputation.start()) {
logger.atFine().log(
@@ -358,8 +354,13 @@
overrides);
BranchNameKey branch = changeNotes.getChange().getDest();
- ObjectId revision = getDestBranchRevision(changeNotes.getChange());
- logger.atFine().log("dest branch %s has revision %s", branch.branch(), revision.name());
+ Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
+ if (revision.isPresent()) {
+ logger.atFine().log(
+ "dest branch %s has revision %s", branch.branch(), revision.get().name());
+ } else {
+ logger.atFine().log("dest branch %s does not exist", branch.branch());
+ }
CodeOwnerResolverResult globalCodeOwners =
codeOwnerResolver.resolveGlobalCodeOwners(changeNotes.getProjectName());
@@ -383,7 +384,7 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
branch,
- revision,
+ revision.orElse(null),
globalCodeOwners,
enableImplicitApproval ? changeOwner : null,
reviewerAccountIds,
@@ -410,7 +411,7 @@
@VisibleForTesting
public Stream<FileCodeOwnerStatus> getFileStatusesForAccount(
ChangeNotes changeNotes, PatchSet patchSet, Account.Id accountId)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
requireNonNull(patchSet, "patchSet");
requireNonNull(accountId, "accountId");
@@ -430,8 +431,13 @@
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());
+ Optional<ObjectId> revision = getDestBranchRevision(changeNotes.getChange());
+ if (revision.isPresent()) {
+ logger.atFine().log(
+ "dest branch %s has revision %s", branch.branch(), revision.get().name());
+ } else {
+ logger.atFine().log("dest branch %s does not exist", branch.branch());
+ }
FallbackCodeOwners fallbackCodeOwners = codeOwnersConfig.getFallbackCodeOwners();
logger.atFine().log("fallbackCodeOwner = %s", fallbackCodeOwners);
@@ -447,7 +453,7 @@
codeOwnerConfigHierarchy,
codeOwnerResolver,
branch,
- revision,
+ revision.orElse(null),
/* 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
@@ -503,7 +509,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
- ObjectId revision,
+ @Nullable ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -568,7 +574,7 @@
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
CodeOwnerResolver codeOwnerResolver,
BranchNameKey branch,
- ObjectId revision,
+ @Nullable ObjectId revision,
CodeOwnerResolverResult globalCodeOwners,
@Nullable Account.Id implicitApprover,
ImmutableSet<Account.Id> reviewerAccountIds,
@@ -1004,18 +1010,18 @@
* <p>This is the revision from which the code owner configs should be read when computing code
* owners for the files that are touched in the change.
*
- * @throws ResourceConflictException thrown if the destination branch is not found, e.g. when the
- * branch got deleted after the change was created
+ * @return the current revision of the destination branch of the given change, {@link
+ * Optional#empty()} if the destination branch is not found (e.g. when the initial change is
+ * uploaded to an unborn branch or when the branch got deleted after the change was created)
*/
- private ObjectId getDestBranchRevision(Change change)
- throws IOException, ResourceConflictException {
+ private Optional<ObjectId> getDestBranchRevision(Change change) throws IOException {
try (Repository repository = repoManager.openRepository(change.getProject());
RevWalk rw = new RevWalk(repository)) {
Ref ref = repository.exactRef(change.getDest().branch());
if (ref == null) {
- throw new ResourceConflictException("destination branch not found");
+ return Optional.empty();
}
- return rw.parseCommit(ref.getObjectId());
+ return Optional.of(rw.parseCommit(ref.getObjectId()));
}
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index 7ba3352..8fb6d09 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -18,6 +18,7 @@
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
@@ -42,6 +43,10 @@
* config in the root folder of the branch. The same as any other parent it can be ignored (e.g. by
* using {@code set noparent} in the root code owner config if the {@code find-owners} backend is
* used).
+ *
+ * <p>Visiting the code owner configs also works for non-existing branches (provided branch revision
+ * is {@code null}). In this case only the default code owner config in {@code refs/meta/config} is
+ * visited (if it exists).
*/
public class CodeOwnerConfigHierarchy {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -65,7 +70,8 @@
* the path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
@@ -73,7 +79,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor) {
visit(
@@ -89,7 +95,8 @@
* the path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param codeOwnerConfigVisitor visitor that should be invoked for the applying code owner
@@ -99,7 +106,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
CodeOwnerConfigVisitor codeOwnerConfigVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -119,7 +126,8 @@
* path hierarchy from the given path up to the root folder.
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absolutePath the path for which the code owner configs should be visited; the path must
* be absolute; can be the path of a file or folder; the path may or may not exist
* @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
@@ -128,7 +136,7 @@
*/
public void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -153,7 +161,8 @@
* (e.g. for large changes).
*
* @param branchNameKey project and branch from which the code owner configs should be visited
- * @param revision the branch revision from which the code owner configs should be loaded
+ * @param revision the branch revision from which the code owner configs should be loaded, {@code
+ * null} if the branch doesn't exist
* @param absoluteFilePath the path for which the code owner configs should be visited; the path
* must be absolute; must be the path of a file; the path may or may not exist
* @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
@@ -162,7 +171,7 @@
*/
public void visitForFile(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absoluteFilePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
@@ -177,13 +186,12 @@
private void visit(
BranchNameKey branchNameKey,
- ObjectId revision,
+ @Nullable ObjectId revision,
Path absolutePath,
Path startFolder,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
requireNonNull(branchNameKey, "branch");
- requireNonNull(revision, "revision");
requireNonNull(absolutePath, "absolutePath");
requireNonNull(pathCodeOwnersVisitor, "pathCodeOwnersVisitor");
requireNonNull(parentCodeOwnersIgnoredCallback, "parentCodeOwnersIgnoredCallback");
@@ -191,47 +199,52 @@
logger.atFine().log(
"visiting code owner configs for '%s' in branch '%s' in project '%s' (revision = '%s')",
- absolutePath, branchNameKey.shortName(), branchNameKey.project(), revision.name());
+ absolutePath,
+ branchNameKey.shortName(),
+ branchNameKey.project(),
+ revision != null ? revision.name() : "n/a");
- // Next path in which we look for a code owner configuration. We start at the given folder and
- // then go up the parent hierarchy.
- Path ownerConfigFolder = startFolder;
+ if (revision != null) {
+ // Next path in which we look for a code owner configuration. We start at the given folder and
+ // then go up the parent hierarchy.
+ Path ownerConfigFolder = startFolder;
- // Iterate over the parent code owner configurations.
- while (ownerConfigFolder != null) {
- // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
- // exists.
- logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
- CodeOwnerConfig.Key codeOwnerConfigKey =
- CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
- Optional<PathCodeOwners> pathCodeOwners =
- pathCodeOwnersFactory.create(
- transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
- if (pathCodeOwners.isPresent()) {
- logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
- boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
- boolean ignoreParentCodeOwners =
- pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
- if (ignoreParentCodeOwners) {
- parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ // Iterate over the parent code owner configurations.
+ while (ownerConfigFolder != null) {
+ // Read code owner config and invoke the codeOwnerConfigVisitor if the code owner config
+ // exists.
+ logger.atFine().log("inspecting code owner config for %s", ownerConfigFolder);
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ CodeOwnerConfig.Key.create(branchNameKey, ownerConfigFolder);
+ Optional<PathCodeOwners> pathCodeOwners =
+ pathCodeOwnersFactory.create(
+ transientCodeOwnerConfigCache, codeOwnerConfigKey, revision, absolutePath);
+ if (pathCodeOwners.isPresent()) {
+ logger.atFine().log("visit code owner config for %s", ownerConfigFolder);
+ boolean visitFurtherCodeOwnerConfigs = pathCodeOwnersVisitor.visit(pathCodeOwners.get());
+ boolean ignoreParentCodeOwners =
+ pathCodeOwners.get().resolveCodeOwnerConfig().get().ignoreParentCodeOwners();
+ if (ignoreParentCodeOwners) {
+ parentCodeOwnersIgnoredCallback.accept(codeOwnerConfigKey);
+ }
+ logger.atFine().log(
+ "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
+ visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
+ if (!visitFurtherCodeOwnerConfigs || ignoreParentCodeOwners) {
+ // If no further code owner configs should be visited or if all parent code owner
+ // configs are ignored, we are done.
+ // No need to check further parent code owner configs (including the default code owner
+ // config in refs/meta/config which is the parent of the root code owner config), hence
+ // we can return here.
+ return;
+ }
+ } else {
+ logger.atFine().log("no code owner config found in %s", ownerConfigFolder);
}
- logger.atFine().log(
- "visitFurtherCodeOwnerConfigs = %s, ignoreParentCodeOwners = %s",
- visitFurtherCodeOwnerConfigs, ignoreParentCodeOwners);
- if (!visitFurtherCodeOwnerConfigs || ignoreParentCodeOwners) {
- // If no further code owner configs should be visited or if all parent code owner configs
- // are ignored, we are done.
- // No need to check further parent code owner configs (including the default code owner
- // config in refs/meta/config which is the parent of the root code owner config), hence we
- // can return here.
- return;
- }
- } else {
- logger.atFine().log("no code owner config found in %s", ownerConfigFolder);
+
+ // Continue the loop with the next parent folder.
+ ownerConfigFolder = ownerConfigFolder.getParent();
}
-
- // Continue the loop with the next parent folder.
- ownerConfigFolder = ownerConfigFolder.getParent();
}
if (!RefNames.REFS_CONFIG.equals(branchNameKey.branch())) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index ec53945..58a500e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -21,8 +21,6 @@
import com.google.gerrit.entities.LegacySubmitRequirement;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.annotations.Exports;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
@@ -95,11 +93,6 @@
return Optional.of(getSubmitRecord(changeData.notes()));
}
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't evaluate code owner statuses for patch set %d of change %d.",
- changeData.currentPatchSet().id().get(), changeData.change().getId().get());
- return Optional.of(notReady());
} catch (Exception e) {
// Whether the exception should be treated as RULE_ERROR.
// RULE_ERROR must only be returned if the exception is caused by user misconfiguration (e.g.
@@ -162,7 +155,7 @@
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws ResourceConflictException, IOException, DiffNotAvailableException {
+ throws IOException, DiffNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 0e0ec81..4e81f46 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -23,7 +23,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.events.ReviewerAddedListener;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -206,23 +205,15 @@
Project.NameKey projectName, Change.Id changeId, Account.Id reviewerAccountId) {
ChangeNotes changeNotes = changeNotesFactory.create(projectName, changeId);
- ImmutableList<Path> ownedPaths;
- try {
- // limit + 1, so that we can show an indicator if there are more than <limit> files.
- ownedPaths =
- OwnedChangedFile.getOwnedPaths(
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- reviewerAccountId,
- /* start= */ 0,
- limit + 1));
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't compute owned paths of change %s for account %s",
- changeNotes.getChangeId(), reviewerAccountId.get());
- return Optional.empty();
- }
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
+ ImmutableList<Path> ownedPaths =
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ reviewerAccountId,
+ /* start= */ 0,
+ limit + 1));
if (ownedPaths.isEmpty()) {
// this reviewer doesn't own any of the modified paths
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index 5b6f6dd..b538ebd 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -17,9 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginProjectConfigSnapshot;
@@ -53,8 +51,6 @@
*/
@Singleton
class OnCodeOwnerApproval implements OnPostReview {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -120,23 +116,15 @@
int limit) {
LabelVote newVote = getNewVote(requiredApproval, approvals);
- ImmutableList<Path> ownedPaths;
- try {
- // limit + 1, so that we can show an indicator if there are more than <limit> files.
- ownedPaths =
- OwnedChangedFile.getOwnedPaths(
- codeOwnerApprovalCheck.getOwnedPaths(
- changeNotes,
- changeNotes.getCurrentPatchSet(),
- user.getAccountId(),
- /* start= */ 0,
- limit + 1));
- } catch (RestApiException e) {
- logger.atFine().withCause(e).log(
- "Couldn't compute owned paths of change %s for account %s",
- changeNotes.getChangeId(), user.getAccountId().get());
- return Optional.empty();
- }
+ // limit + 1, so that we can show an indicator if there are more than <limit> files.
+ ImmutableList<Path> ownedPaths =
+ OwnedChangedFile.getOwnedPaths(
+ codeOwnerApprovalCheck.getOwnedPaths(
+ changeNotes,
+ changeNotes.getCurrentPatchSet(),
+ user.getAccountId(),
+ /* start= */ 0,
+ limit + 1));
if (ownedPaths.isEmpty()) {
// the user doesn't own any of the modified paths
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index 22cc758..f3e7d1b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -16,8 +16,10 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerStatusInfoSubject.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementInfoSubject.assertThatCollection;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
@@ -25,8 +27,10 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestMetricMaker;
+import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.RefNames;
@@ -53,6 +57,7 @@
/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.CodeOwnerSubmitRule}. */
public class CodeOwnerSubmitRuleIT extends AbstractCodeOwnersIT {
+ @Inject private RequestScopeOperations requestScopeOperations;
@Inject private ProjectOperations projectOperations;
@Inject private TestMetricMaker testMetricMaker;
@@ -100,7 +105,17 @@
}
@Test
- public void changeWithInsufficentReviewersIsNotSubmittable() throws Exception {
+ public void changeWithInsufficientReviewersIsNotSubmittable() throws Exception {
+ testChangeWithInsufficientReviewersIsNotSubmittable();
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithInsufficientReviewersIsNotSubmittable() throws Exception {
+ testChangeWithInsufficientReviewersIsNotSubmittable();
+ }
+
+ private void testChangeWithInsufficientReviewersIsNotSubmittable() throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Approve by a non-code-owner.
@@ -151,10 +166,23 @@
.addCodeOwnerEmail(user.email())
.create();
+ testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(user);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithPendingCodeOwnerApprovalsIsNotSubmittable() throws Exception {
+ setAsDefaultCodeOwners(user);
+
+ testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(user);
+ }
+
+ private void testChangeWithPendingCodeOwnerApprovalsIsNotSubmittable(TestAccount codeOwner)
+ throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Add a reviewer that is a code owner.
- gApi.changes().id(changeId).addReviewer(user.email());
+ gApi.changes().id(changeId).addReviewer(codeOwner.email());
// Approve by a non-code-owner.
approve(changeId);
@@ -201,13 +229,33 @@
.project(project)
.branch("master")
.folderPath("/foo/")
- .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
.create();
+ testChangeWithCodeOwnerApprovalsIsSubmittable(user);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithCodeOwnerApprovalsIsSubmittable() throws Exception {
+ setAsDefaultCodeOwners(user);
+
+ testChangeWithCodeOwnerApprovalsIsSubmittable(user);
+ }
+
+ private void testChangeWithCodeOwnerApprovalsIsSubmittable(TestAccount codeOwner)
+ throws Exception {
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
// Approve by a code-owner.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(codeOwner.id());
approve(changeId);
+ requestScopeOperations.setApiUser(admin.id());
// Verify that the code owner status for the changed file is APPROVED.
CodeOwnerStatusInfo codeOwnerStatus =
@@ -239,6 +287,17 @@
@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
public void changeWithOverrideApprovalIsSubmittable() throws Exception {
+ testChangeWithOverrideApprovalIsSubmittable();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ @TestProjectInput(createEmptyCommit = false)
+ public void initialChangeWithOverrideApprovalIsSubmittable() throws Exception {
+ testChangeWithOverrideApprovalIsSubmittable();
+ }
+
+ private void testChangeWithOverrideApprovalIsSubmittable() throws Exception {
createOwnersOverrideLabel();
String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index 34642d5..88ff7d0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -42,7 +42,6 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelDefinitionInput;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus;
@@ -1482,19 +1481,204 @@
}
@Test
- public void getStatus_branchDeleted() throws Exception {
+ public void getStatus_branchDeleted_defaultCodeOwner() throws Exception {
String branchName = "tempBranch";
createBranch(BranchNameKey.create(project, branchName));
- String changeId = createChange("refs/for/" + branchName).getChangeId();
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = ImmutableList.of(branchName);
gApi.projects().name(project.get()).deleteBranches(input);
- ResourceConflictException exception =
- assertThrows(ResourceConflictException.class, () -> getFileCodeOwnerStatuses(changeId));
- assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
+ testGetStatusBranchDoesNotExistWithDefaultCodeOwner(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getStatus_initialChange_defaultCodeOwner() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithDefaultCodeOwner(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithDefaultCodeOwner(String changeId, Path path)
+ throws Exception {
+ setAsDefaultCodeOwners(admin);
+
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add default code owner as a reviewer.
+ gApi.changes().id(changeId).addReviewer(admin.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+
+ // Approve as default code owner.
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a default code owner",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void getStatus_branchDeleted_globalCodeOwner() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExistWithGlobalCodeOwner(changeId, path, bot);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "bot@example.com")
+ public void getStatus_initialChange_globalCodeOwner() throws Exception {
+ // Create a bot user that is a global code owner.
+ TestAccount bot =
+ accountCreator.create("bot", "bot@example.com", "Bot", /* displayName= */ null);
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithGlobalCodeOwner(changeId, path, bot);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithGlobalCodeOwner(
+ String changeId, Path path, TestAccount globalCodeOwner) throws Exception {
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add global code owner as a reviewer.
+ gApi.changes().id(changeId).addReviewer(globalCodeOwner.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(globalCodeOwner.id()))));
+
+ // Approve as default code owner.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/heads/*").group(REGISTERED_USERS).range(-2, +2))
+ .update();
+ requestScopeOperations.setApiUser(globalCodeOwner.id());
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a global code owner",
+ AccountTemplateUtil.getAccountTemplate(globalCodeOwner.id()))));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void getStatus_branchDeleted_override() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExistWithOverride(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void getStatus_initialChange_override() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(admin, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExistWithOverride(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExistWithOverride(String changeId, Path path)
+ throws Exception {
+ createOwnersOverrideLabel();
+
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Apply an override
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "override approval Owners-Override+1 by %s is present",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
index c91d897..5c31eed 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckWithAllUsersAsFallbackCodeOwnersTest.java
@@ -16,11 +16,15 @@
import static com.google.gerrit.plugins.codeowners.testing.FileCodeOwnerStatusSubject.assertThatCollection;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig;
@@ -459,6 +463,70 @@
FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
}
+ @Test
+ public void getStatus_branchDeleted() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ testGetStatusBranchDoesNotExist(changeId, path);
+ }
+
+ @Test
+ @TestProjectInput(createEmptyCommit = false)
+ public void getStatus_initialChange() throws Exception {
+ // Create a change as a user that is not a code owner.
+ Path path = Paths.get("/foo/bar.baz");
+ String changeId =
+ createChange(user, "Change Adding A File", JgitPath.of(path).get(), "file content")
+ .getChangeId();
+
+ testGetStatusBranchDoesNotExist(changeId, path);
+ }
+
+ private void testGetStatusBranchDoesNotExist(String changeId, Path path) throws Exception {
+ ImmutableSet<FileCodeOwnerStatus> fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(path, CodeOwnerStatus.INSUFFICIENT_REVIEWERS));
+
+ // Add code owner as a reviewer (all users are fallback code owners).
+ gApi.changes().id(changeId).addReviewer(admin.email());
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.PENDING,
+ String.format(
+ "reviewer %s is a fallback code owner (all users are fallback code owners)",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+
+ // Approve as a code owner (all users are fallback code owners).
+ approve(changeId);
+
+ fileCodeOwnerStatuses = getFileCodeOwnerStatuses(changeId);
+ assertThatCollection(fileCodeOwnerStatuses)
+ .containsExactly(
+ FileCodeOwnerStatus.addition(
+ path,
+ CodeOwnerStatus.APPROVED,
+ String.format(
+ "approved by %s who is a fallback code owner"
+ + " (all users are fallback code owners)",
+ AccountTemplateUtil.getAccountTemplate(admin.id()))));
+ }
+
private ImmutableSet<FileCodeOwnerStatus> getFileCodeOwnerStatuses(String changeId)
throws Exception {
return codeOwnerApprovalCheck.getFileStatusesAsSet(
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
index df5ad05..0422551 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchyTest.java
@@ -89,17 +89,13 @@
}
@Test
- public void cannotVisitCodeOwnerConfigsForNullRevision() throws Exception {
- NullPointerException npe =
- assertThrows(
- NullPointerException.class,
- () ->
- codeOwnerConfigHierarchy.visit(
- BranchNameKey.create(project, "master"),
- /* revision= */ null,
- Paths.get("/foo/bar/baz.md"),
- visitor));
- assertThat(npe).hasMessageThat().isEqualTo("revision");
+ public void visitorNotInvokedForNullRevision() throws Exception {
+ codeOwnerConfigHierarchy.visit(
+ BranchNameKey.create(project, "master"),
+ /* revision= */ null,
+ Paths.get("/foo/bar/baz.md"),
+ visitor);
+ verifyNoInteractions(visitor);
}
@Test
@@ -583,6 +579,27 @@
}
@Test
+ public void visitorInvokedForCodeOwnerConfigInRefsMetaConfig_nullRevision() throws Exception {
+ CodeOwnerConfig.Key metaCodeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch(RefNames.REFS_CONFIG)
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ when(visitor.visit(any(CodeOwnerConfig.class))).thenReturn(true);
+ codeOwnerConfigHierarchy.visit(
+ BranchNameKey.create(project, "master"),
+ /* revision= */ null,
+ Paths.get("/foo/bar/baz.md"),
+ visitor);
+ verify(visitor).visit(codeOwnerConfigOperations.codeOwnerConfig(metaCodeOwnerConfigKey).get());
+ verifyNoMoreInteractions(visitor);
+ }
+
+ @Test
public void visitorInvokedForCodeOwnerConfigInRefsMetaConfigIfItDoesntApply() throws Exception {
CodeOwnerConfig.Key metaCodeOwnerConfigKey =
codeOwnerConfigOperations
