Return 409 Conflict if a code owner config is invalid
At the moment we fail with 500 ISE if there is a code owner config, but
it's not an error that should be fixed by the Gerrit team. Rather it's a
configuration issue that needs to be fixed by the project owner.
Returning 409 Conflict is consistent with how Gerrit core behaves in
similar situations (e.g. when there is an invalid Prolog rule).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I27bdcf994cc9a0e086f02706d729594ac7d1a3a2
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
index aa0c836..1de1325 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
@@ -121,9 +121,17 @@
revWalk.close();
}
}
- } catch (IOException | ConfigInvalidException e) {
+ } catch (IOException e) {
throw new StorageException(
String.format("failed to load code owner config %s", codeOwnerConfigKey), e);
+ } catch (ConfigInvalidException e) {
+ throw new StorageException(
+ String.format(
+ "invalid code owner config file %s (project = %s, branch = %s)",
+ codeOwnerConfigKey.filePath(defaultFileName),
+ codeOwnerConfigKey.project(),
+ codeOwnerConfigKey.branchNameKey().branch()),
+ e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
index 519bc8f..5f697d2 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwners.java
@@ -77,7 +77,7 @@
* ConfigInvalidException}). If yes, the {@link ConfigInvalidException} is returned. If no, {@link
* Optional#empty()} is returned.
*/
- public static Optional<ConfigInvalidException> getInvalidConfigCause(Exception e) {
+ public static Optional<ConfigInvalidException> getInvalidConfigCause(Throwable e) {
return Throwables.getCausalChain(e).stream()
.filter(t -> t instanceof ConfigInvalidException)
.map(t -> (ConfigInvalidException) t)
diff --git a/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java b/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java
index 7bf92bd..08f75bc 100644
--- a/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java
+++ b/java/com/google/gerrit/plugins/codeowners/config/InvalidPluginConfigurationException.java
@@ -17,6 +17,7 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwners;
import java.util.Optional;
/**
@@ -27,12 +28,14 @@
public static class ExceptionHook implements com.google.gerrit.server.ExceptionHook {
@Override
public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
- return isInvalidPluginConfigurationException(throwable);
+ return isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable);
}
@Override
public ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
- if (isInvalidPluginConfigurationException(throwable)) {
+ if (isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)) {
return ImmutableList.of(throwable.getMessage());
}
return ImmutableList.of();
@@ -40,7 +43,8 @@
@Override
public Optional<Status> getStatus(Throwable throwable) {
- if (isInvalidPluginConfigurationException(throwable)) {
+ if (isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)) {
return Optional.of(Status.create(409, "Conflict"));
}
return Optional.empty();
@@ -50,6 +54,10 @@
return Throwables.getCausalChain(throwable).stream()
.anyMatch(t -> t instanceof InvalidPluginConfigurationException);
}
+
+ private static boolean isInvalidCodeOwnerConfigException(Throwable throwable) {
+ return CodeOwners.getInvalidConfigCause(throwable).isPresent();
+ }
}
private static final long serialVersionUID = 1L;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
index ec985c1..364c743 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
@@ -19,8 +19,13 @@
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.plugins.codeowners.JgitPath;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
+import com.google.gerrit.plugins.codeowners.config.BackendConfig;
+import com.google.gerrit.plugins.codeowners.config.StatusConfig;
+import org.junit.Before;
import org.junit.Test;
/**
@@ -36,6 +41,13 @@
* extend {@link AbstractCodeOwnersIT}.
*/
public class GetCodeOwnerStatusRestIT extends AbstractCodeOwnersTest {
+ private BackendConfig backendConfig;
+
+ @Before
+ public void setUpCodeOwnersPlugin() throws Exception {
+ backendConfig = plugin.getSysInjector().getInstance(BackendConfig.class);
+ }
+
@Test
@GerritConfig(name = "plugin.code-owners.backend", value = "non-existing-backend")
public void cannotGetStatusIfPluginConfigurationIsInvalid() throws Exception {
@@ -51,4 +63,34 @@
+ " 'non-existing-backend' that is configured in gerrit.config (parameter"
+ " plugin.code-owners.backend) not found.");
}
+
+ @Test
+ public void cannotGetStatusIfCodeOwnerConfigIsInvalid() throws Exception {
+ String filePath = getCodeOwnerConfigFilePath(createCodeOwnerConfigKey("/"));
+ disableCodeOwnersForProject(project);
+ String changeId =
+ createChange("Add code owners", JgitPath.of(filePath).get(), "INVALID").getChangeId();
+ approve(changeId);
+ gApi.changes().id(changeId).current().submit();
+ setCodeOwnersConfig(project, null, StatusConfig.KEY_DISABLED, "false");
+
+ String changeId2 = createChange().getChangeId();
+ RestResponse r =
+ adminRestSession.get(
+ String.format("/changes/%s/code_owners.status", IdString.fromDecoded(changeId2)));
+ r.assertConflict();
+ assertThat(r.getEntityContent())
+ .contains(
+ String.format(
+ "invalid code owner config file %s (project = %s, branch = refs/heads/master)",
+ filePath, project.get()));
+ }
+
+ private CodeOwnerConfig.Key createCodeOwnerConfigKey(String folderPath) {
+ return CodeOwnerConfig.Key.create(project, "master", folderPath);
+ }
+
+ private String getCodeOwnerConfigFilePath(CodeOwnerConfig.Key codeOwnerConfigKey) {
+ return backendConfig.getDefaultBackend().getFilePath(codeOwnerConfigKey).toString();
+ }
}
