Treat uncaught InvalidPathExceptions as '409 Conflict'
If there is an invalid path Paths.get(path) throws an
InvalidPathException. This is a RuntimeException that currently results
in '500 Internal Server Error', not telling the user what is wrong. By
handling this exception the user gets a better error message, e.g.
telling which path is considered as invalid.
E.g. InvalidPathExceptions may be thrown for paths that contain unicode
characters. Whether unicode characters in file names trigger this
exception depends on the local environment of the system that is
running Gerrit (local encoding and locale settings), but we haven't
figured out yet which system properties matter here (e.g. unicode
characters in file names work fine for me when I run tests from Eclipse,
but not when I run the same test via Bazel from the commandline). Since
the behaviour depends on the local system we do not have any
integration test for the code handling InvalidPathExceptions.
Once we figure out which system properties are relevant here, we will
set them in the test environment and then we can have tests with unicode
characters in filenames. Until then the best we can do is being
transparant to users that unicode paths do not work.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I34f0cd30926e02d577480e8efc619ca47b69c821
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
index 53d7cd2..8a2e85e 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -19,6 +19,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
import com.google.gerrit.server.ExceptionHook;
+import java.nio.file.InvalidPathException;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -39,7 +40,8 @@
@Override
public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
return isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable);
+ || isInvalidCodeOwnerConfigException(throwable)
+ || isInvalidPathException(throwable);
}
@Override
@@ -56,13 +58,19 @@
return ImmutableList.of(configInvalidException.get().getMessage());
}
+ Optional<InvalidPathException> invalidPathException = getInvalidPathException(throwable);
+ if (invalidPathException.isPresent()) {
+ return ImmutableList.of(invalidPathException.get().getMessage());
+ }
+
return ImmutableList.of();
}
@Override
public Optional<Status> getStatus(Throwable throwable) {
if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)) {
+ || isInvalidCodeOwnerConfigException(throwable)
+ || isInvalidPathException(throwable)) {
return Optional.of(Status.create(409, "Conflict"));
}
return Optional.empty();
@@ -74,9 +82,22 @@
private static Optional<InvalidPluginConfigurationException> getInvalidPluginConfigurationCause(
Throwable throwable) {
+ return getInvalidPluginConfigurationCause(InvalidPluginConfigurationException.class, throwable);
+ }
+
+ private static boolean isInvalidPathException(Throwable throwable) {
+ return getInvalidPathException(throwable).isPresent();
+ }
+
+ private static Optional<InvalidPathException> getInvalidPathException(Throwable throwable) {
+ return getInvalidPluginConfigurationCause(InvalidPathException.class, throwable);
+ }
+
+ private static <T extends Throwable> Optional<T> getInvalidPluginConfigurationCause(
+ Class<T> exceptionClass, Throwable throwable) {
return Throwables.getCausalChain(throwable).stream()
- .filter(t -> t instanceof InvalidPluginConfigurationException)
- .map(t -> (InvalidPluginConfigurationException) t)
+ .filter(exceptionClass::isInstance)
+ .map(exceptionClass::cast)
.findFirst();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
index e05f8cd..c727080 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
@@ -21,6 +21,7 @@
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
import com.google.gerrit.server.ExceptionHook.Status;
+import java.nio.file.InvalidPathException;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.junit.Before;
@@ -44,6 +45,9 @@
assertThat(skipRetryWithTrace(newConfigInvalidException())).isTrue();
assertThat(skipRetryWithTrace(newExceptionWithCause(newConfigInvalidException()))).isTrue();
+ assertThat(skipRetryWithTrace(newInvalidPathException())).isTrue();
+ assertThat(skipRetryWithTrace(newExceptionWithCause(newInvalidPathException()))).isTrue();
+
assertThat(skipRetryWithTrace(new Exception())).isFalse();
assertThat(skipRetryWithTrace(newExceptionWithCause(new Exception()))).isFalse();
}
@@ -63,6 +67,12 @@
assertThat(getUserMessages(newExceptionWithCause(configInvalidException)))
.containsExactly(configInvalidException.getMessage());
+ InvalidPathException invalidPathException = newInvalidPathException();
+ assertThat(getUserMessages(invalidPathException))
+ .containsExactly(invalidPathException.getMessage());
+ assertThat(getUserMessages(newExceptionWithCause(invalidPathException)))
+ .containsExactly(invalidPathException.getMessage());
+
assertThat(getUserMessages(new Exception())).isEmpty();
assertThat(getUserMessages(newExceptionWithCause(new Exception()))).isEmpty();
}
@@ -82,6 +92,11 @@
.value()
.isEqualTo(conflictStatus);
+ assertThat(getStatus(newInvalidPathException())).value().isEqualTo(conflictStatus);
+ assertThat(getStatus(newExceptionWithCause(newInvalidPathException())))
+ .value()
+ .isEqualTo(conflictStatus);
+
assertThat(getStatus(new Exception())).isEmpty();
assertThat(getStatus(newExceptionWithCause(new Exception()))).isEmpty();
}
@@ -109,4 +124,8 @@
private ConfigInvalidException newConfigInvalidException() {
return new ConfigInvalidException("message");
}
+
+ private InvalidPathException newInvalidPathException() {
+ return new InvalidPathException("input", "reason");
+ }
}
