Adapt to API change in Gerrit core and handle case where branch rev not present
Change I60ced9e37 in Gerrit core changed
BranchResource#getRevision()
from returning String to returning Optional<String> to force callers to
handle the case where the revision is not present. As explained in the
commit message of change I60ced9e37 the revision is not present if the
branch identifier in a REST call was a symbolic ref that points to an
unborn branch.
Handle and test this case for all branch REST endpoints.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ibf85ff0a6511b47806f4bd2fbf496b91a1622ea9
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java
index 75cc0eb..dd603fc 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractPathResource.java
@@ -14,11 +14,10 @@
package com.google.gerrit.plugins.codeowners.restapi;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestResource;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.project.BranchResource;
@@ -58,16 +57,14 @@
private final ObjectId revision;
private final Path path;
- protected AbstractPathResource(BranchResource branchResource, Path path) {
+ protected AbstractPathResource(BranchResource branchResource, Path path)
+ throws ResourceNotFoundException {
+ if (branchResource.getRevision().isEmpty()) {
+ throw new ResourceNotFoundException(IdString.fromDecoded(branchResource.getName()));
+ }
+
this.branchNameKey = branchResource.getBranchKey();
-
- checkState(
- branchResource.getRevision() != null,
- "branch %s in project %s wasn't created yet",
- branchResource.getBranchKey().branch(),
- branchResource.getBranchKey().project().get());
- this.revision = ObjectId.fromString(branchResource.getRevision());
-
+ this.revision = ObjectId.fromString(branchResource.getRevision().get());
this.path = path;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
index d6470a2..ad8b388 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CheckCodeOwner.java
@@ -148,7 +148,7 @@
@Override
public Response<CodeOwnerCheckInfo> apply(BranchResource branchResource)
throws BadRequestException, AuthException, IOException, ConfigInvalidException,
- PermissionBackendException {
+ PermissionBackendException, ResourceNotFoundException {
permissionBackend.currentUser().check(checkCodeOwnerCapability.getPermission());
validateInput(branchResource);
@@ -164,7 +164,7 @@
Set<String> annotations = new HashSet<>();
codeOwnerConfigHierarchy.visit(
branchResource.getBranchKey(),
- ObjectId.fromString(branchResource.getRevision()),
+ ObjectId.fromString(branchResource.getRevision().get()),
absolutePath,
codeOwnerConfig -> {
messages.add(
@@ -348,7 +348,11 @@
private void validateInput(BranchResource branchResource)
throws BadRequestException, AuthException, IOException, ConfigInvalidException,
- PermissionBackendException {
+ PermissionBackendException, ResourceNotFoundException {
+ if (branchResource.getRevision().isEmpty()) {
+ throw new ResourceNotFoundException(IdString.fromDecoded(branchResource.getName()));
+ }
+
if (email == null) {
throw new BadRequestException("email required");
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java
index 9b13e81..3d324d5 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerConfigsInBranchCollection.java
@@ -49,7 +49,8 @@
}
@Override
- public PathResource parse(BranchResource branchResource, IdString id) throws BadRequestException {
+ public PathResource parse(BranchResource branchResource, IdString id)
+ throws BadRequestException, ResourceNotFoundException {
return PathResource.parse(branchResource, id);
}
@@ -72,11 +73,12 @@
static final TypeLiteral<RestView<PathResource>> PATH_KIND = new TypeLiteral<>() {};
static PathResource parse(BranchResource branchResource, IdString pathId)
- throws BadRequestException {
+ throws BadRequestException, ResourceNotFoundException {
return new PathResource(branchResource, parsePath(pathId));
}
- private PathResource(BranchResource branchResource, Path path) {
+ private PathResource(BranchResource branchResource, Path path)
+ throws ResourceNotFoundException {
super(branchResource, path);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java
index 0f4011c..feb1199 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/CodeOwnersInBranchCollection.java
@@ -48,7 +48,8 @@
}
@Override
- public PathResource parse(BranchResource branchResource, IdString id) throws BadRequestException {
+ public PathResource parse(BranchResource branchResource, IdString id)
+ throws BadRequestException, ResourceNotFoundException {
return PathResource.parse(branchResource, id);
}
@@ -70,7 +71,7 @@
static final TypeLiteral<RestView<PathResource>> PATH_KIND = new TypeLiteral<>() {};
static PathResource parse(BranchResource branchResource, IdString pathId)
- throws BadRequestException {
+ throws BadRequestException, ResourceNotFoundException {
return new PathResource(branchResource, parsePath(pathId));
}
@@ -78,7 +79,8 @@
return new PathResource(getBranch(), revision, getPath());
}
- private PathResource(BranchResource branchResource, Path path) {
+ private PathResource(BranchResource branchResource, Path path)
+ throws ResourceNotFoundException {
super(branchResource, path);
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
index 7952b7e..83eb13f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerIT.java
@@ -24,6 +24,7 @@
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
@@ -62,6 +63,7 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.util.Arrays;
+import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Test;
@@ -88,6 +90,25 @@
}
@Test
+ public void checkCodeOwnerForNonExistingBranch() throws Exception {
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/non-existing/code_owners.check", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ public void checkCodeOwnerForSymbolicRefPointingToAnUnbornBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ repo.updateRef(Constants.HEAD, true).link("refs/heads/non-existing");
+ }
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/HEAD/code_owners.check", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
public void requiresEmail() throws Exception {
BadRequestException exception =
assertThrows(BadRequestException.class, () -> checkCodeOwner("/", /* email= */ null));
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java
index 3048707..5ddd85f 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnerConfigForPathInBranchIT.java
@@ -18,6 +18,7 @@
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerConfigInfoSubject.assertThatOptional;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
@@ -25,6 +26,8 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
import com.google.gerrit.plugins.codeowners.util.JgitPath;
import java.util.Optional;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
/**
@@ -39,6 +42,28 @@
public class GetCodeOwnerConfigForPathInBranchIT extends AbstractCodeOwnersIT {
@Test
@GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "true")
+ public void getCodeOwnerConfigForNonExistingBranch() throws Exception {
+ RestResponse response =
+ adminRestSession.get(
+ String.format(
+ "/projects/%s/branches/non-existing/code_owners.config/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "true")
+ public void getCodeOwnerConfigFromSymbolicRefPointingToAnUnbornBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ repo.updateRef(Constants.HEAD, true).link("refs/heads/non-existing");
+ }
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/HEAD/code_owners.config/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableExperimentalRestEndpoints", value = "true")
public void getNonExistingCodeOwnerConfig() throws Exception {
assertThatOptional(codeOwnerConfigsApiFactory.branch(project, "master").get("/")).isEmpty();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
index dfd97d4..a78d420 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/GetCodeOwnersForPathInBranchIT.java
@@ -21,6 +21,7 @@
import static java.util.stream.Collectors.toList;
import static org.junit.Assert.fail;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
@@ -38,6 +39,8 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSetModification;
import com.google.inject.Inject;
import java.util.List;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
@@ -60,6 +63,25 @@
}
@Test
+ public void getCodeOwnersForNonExistingBranch() throws Exception {
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/non-existing/code_owners/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
+ public void getCodeOwnersFromSymbolicRefPointingToAnUnbornBranch() throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ repo.updateRef(Constants.HEAD, true).link("refs/heads/non-existing");
+ }
+ RestResponse response =
+ adminRestSession.get(
+ String.format("/projects/%s/branches/HEAD/code_owners/path", project.get()));
+ response.assertNotFound();
+ }
+
+ @Test
public void getCodeOwnersOrderNotDefinedIfCodeOwnersHaveTheSameScoring() throws Exception {
TestAccount user2 = accountCreator.user2();
TestAccount user3 = accountCreator.create("user3", "user3@example.com", "User3", null);
