CodeOwnerApprovalCheck: Return 409 Conflict if destination branch was deleted
It can happen that the destination branch of a change gets deleted. In
this case we cannot compute the code owner status for the change, since
the code owner configs are loaded from the destination branch, which is
not possible when the branch was deleted. Return 409 Conflict in this
case rather than failing with an internal server error.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ibd694fc22113dad89c50cf8404f25b703b79855d
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index a49620b..5f55f62 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -14,7 +14,6 @@
package com.google.gerrit.plugins.codeowners.backend;
-import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static java.util.Objects.requireNonNull;
@@ -27,6 +26,7 @@
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerStatus;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.config.RequiredApproval;
@@ -106,7 +106,7 @@
* @return whether the given change has sufficient code owner approvals to be submittable
*/
public boolean isSubmittable(ChangeNotes changeNotes)
- throws IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
logger.atFine().log(
"checking if change %d in project %s is submittable",
@@ -156,7 +156,7 @@
* returned
*/
public Stream<FileCodeOwnerStatus> getFileStatuses(ChangeNotes changeNotes)
- throws IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
try (TraceTimer traceTimer =
TraceContext.newTimer(
@@ -636,16 +636,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
*/
- private ObjectId getDestBranchRevision(Change change) throws IOException {
+ private ObjectId getDestBranchRevision(Change change)
+ throws IOException, ResourceConflictException {
try (Repository repository = repoManager.openRepository(change.getProject());
RevWalk rw = new RevWalk(repository)) {
Ref ref = repository.exactRef(change.getDest().branch());
- checkNotNull(
- ref,
- "branch %s in repository %s not found",
- change.getDest().branch(),
- change.getProject().get());
+ if (ref == null) {
+ throw new ResourceConflictException("destination branch not found");
+ }
return rw.parseCommit(ref.getObjectId());
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index c1a21a1..f179a9c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -22,6 +22,7 @@
import com.google.gerrit.entities.SubmitRequirement;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -98,7 +99,7 @@
}
private SubmitRecord getSubmitRecord(ChangeNotes changeNotes)
- throws IOException, PatchListNotAvailableException {
+ throws ResourceConflictException, IOException, PatchListNotAvailableException {
requireNonNull(changeNotes, "changeNotes");
return codeOwnerApprovalCheck.isSubmittable(changeNotes) ? ok() : notReady();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
index dc2549f..1249301 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java
@@ -22,13 +22,17 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.TestAccount;
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.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.plugins.codeowners.JgitPath;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
@@ -1848,6 +1852,24 @@
}
}
+ @Test
+ public void getStatus_branchDeleted() throws Exception {
+ String branchName = "tempBranch";
+ createBranch(BranchNameKey.create(project, branchName));
+
+ String changeId = createChange("refs/for/" + branchName).getChangeId();
+
+ DeleteBranchesInput input = new DeleteBranchesInput();
+ input.branches = ImmutableList.of(branchName);
+ gApi.projects().name(project.get()).deleteBranches(input);
+
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> codeOwnerApprovalCheck.getFileStatuses(getChangeNotes(changeId)));
+ assertThat(exception).hasMessageThat().isEqualTo("destination branch not found");
+ }
+
private ChangeNotes getChangeNotes(String changeId) throws Exception {
return changeNotesFactory.create(project, Change.id(gApi.changes().id(changeId).get()._number));
}
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index abc4bfe..fdcd4da 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -340,6 +340,11 @@
}
```
+If the destination branch of a change no longer exists (e.g. because it was
+deleted), `409 Conflict` is returned. Since the code owners are retrieved from
+the destination branch, computing the code owner status is not possible, if the
+destination branch is missing.
+
## <a id="revision-endpoints"> Revision Endpoints
### <a id="list-code-owners-for-path-in-change"> List Code Owners for path in change
