CodeOwnersExceptionHook: Fix extracting message from exception cause
Instead of returning the message of the outer exception, return the
message of the cause. The cause should have a more detailed message,
e.g. if a code owner config file is invalid, it includes which line is
invalid.
For invalid code owner config files some information (project, branch)
was only available in the outer exception. Include this info into the
cause exception message too, so that we do not lose this info.
Change-Id: Ia26d880fd6574fe00cd76d3fcf5931b23f3e761b
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigParseException.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigParseException.java
index 3de255d..b8fc54b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigParseException.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigParseException.java
@@ -40,7 +40,12 @@
/** Returns all validation as a single, formatted string. */
public String getFullMessage(String defaultCodeOwnerConfigFileName) {
StringBuilder sb = new StringBuilder(getMessage());
- sb.append(" '").append(codeOwnerConfigKey.filePath(defaultCodeOwnerConfigFileName)).append("'");
+ sb.append(
+ String.format(
+ " '%s' (project = %s, branch = %s)",
+ codeOwnerConfigKey.filePath(defaultCodeOwnerConfigFileName),
+ codeOwnerConfigKey.project(),
+ codeOwnerConfigKey.shortBranchName()));
if (!messages.isEmpty()) {
sb.append(':');
for (ValidationError msg : messages) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
index abfc9f5..53d7cd2 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -20,6 +20,7 @@
import com.google.gerrit.plugins.codeowners.backend.config.InvalidPluginConfigurationException;
import com.google.gerrit.server.ExceptionHook;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/**
* Class to define the HTTP response status code and message for exceptions that can occur for all
@@ -43,10 +44,18 @@
@Override
public ImmutableList<String> getUserMessages(Throwable throwable, @Nullable String traceId) {
- if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)) {
- return ImmutableList.of(throwable.getMessage());
+ Optional<InvalidPluginConfigurationException> invalidPluginConfigurationException =
+ getInvalidPluginConfigurationCause(throwable);
+ if (invalidPluginConfigurationException.isPresent()) {
+ return ImmutableList.of(invalidPluginConfigurationException.get().getMessage());
}
+
+ Optional<ConfigInvalidException> configInvalidException =
+ CodeOwners.getInvalidConfigCause(throwable);
+ if (configInvalidException.isPresent()) {
+ return ImmutableList.of(configInvalidException.get().getMessage());
+ }
+
return ImmutableList.of();
}
@@ -60,8 +69,15 @@
}
private static boolean isInvalidPluginConfigurationException(Throwable throwable) {
+ return getInvalidPluginConfigurationCause(throwable).isPresent();
+ }
+
+ private static Optional<InvalidPluginConfigurationException> getInvalidPluginConfigurationCause(
+ Throwable throwable) {
return Throwables.getCausalChain(throwable).stream()
- .anyMatch(t -> t instanceof InvalidPluginConfigurationException);
+ .filter(t -> t instanceof InvalidPluginConfigurationException)
+ .map(t -> (InvalidPluginConfigurationException) t)
+ .findFirst();
}
private static boolean isInvalidCodeOwnerConfigException(Throwable throwable) {
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
index 91e8ca2..55dee9b 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesIT.java
@@ -169,8 +169,10 @@
ImmutableList.of(
fatal(
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s,"
+ + " branch = master):\n %s",
codeOwnerConfigPath,
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
@@ -560,8 +562,9 @@
ImmutableList.of(
fatal(
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
pathOfNonParseableCodeOwnerConfig,
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
index 3d71b53..0a6c5ad 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CheckCodeOwnerConfigFilesInRevisionIT.java
@@ -129,8 +129,9 @@
ImmutableList.of(
fatal(
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
codeOwnerConfigPath,
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
@@ -458,8 +459,9 @@
ImmutableList.of(
fatal(
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
pathOfNonParseableCodeOwnerConfig,
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index 314747c..82c6127 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -407,8 +407,9 @@
r,
"invalid code owner config files",
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
@@ -526,8 +527,9 @@
r,
"invalid code owner config files",
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
@@ -640,8 +642,9 @@
r,
"invalid code owner config files",
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
@@ -670,8 +673,9 @@
r,
"invalid code owner config files",
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).getFilePath(),
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
@@ -679,8 +683,9 @@
ProtoBackend.class,
"1:8: expected \"{\""))),
String.format(
- "invalid code owner config file '%s':\n %s",
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).getFilePath(),
+ project,
getParsingErrorMessage(
ImmutableMap.of(
FindOwnersBackend.class,
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 7894bf2..a4e6437 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/restapi/GetCodeOwnerStatusRestIT.java
@@ -83,7 +83,8 @@
assertThat(r.getEntityContent())
.contains(
String.format(
- "invalid code owner config file %s (project = %s, branch = refs/heads/master)",
+ "* invalid code owner config file '%s' (project = %s, branch = master):\n"
+ + " invalid line: INVALID",
filePath, project.get()));
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
index 3c04fd0..e05f8cd 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
@@ -54,18 +54,14 @@
newInvalidPluginConfigurationException();
assertThat(getUserMessages(invalidPluginConfigurationException))
.containsExactly(invalidPluginConfigurationException.getMessage());
-
- // TODO(ekempin): Make this assertion work
- // assertThat(getUserMessages(newExceptionWithCause(invalidPluginConfigurationException)))
- // .containsExactly(invalidPluginConfigurationException.getMessage());
+ assertThat(getUserMessages(newExceptionWithCause(invalidPluginConfigurationException)))
+ .containsExactly(invalidPluginConfigurationException.getMessage());
ConfigInvalidException configInvalidException = newConfigInvalidException();
assertThat(getUserMessages(configInvalidException))
.containsExactly(configInvalidException.getMessage());
-
- // TODO(ekempin): Make this assertion work
- // assertThat(getUserMessages(newExceptionWithCause(configInvalidException)))
- // .containsExactly(configInvalidException.getMessage());
+ assertThat(getUserMessages(newExceptionWithCause(configInvalidException)))
+ .containsExactly(configInvalidException.getMessage());
assertThat(getUserMessages(new Exception())).isEmpty();
assertThat(getUserMessages(newExceptionWithCause(new Exception()))).isEmpty();
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
index 7864b84..6499fa7 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -140,10 +140,12 @@
EMAIL_1, "@example.com", "admin@", "admin@example@com", EMAIL_2)));
assertThat(exception.getFullMessage(FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME))
.isEqualTo(
- "invalid code owner config file '/OWNERS':\n"
- + " invalid line: @example.com\n"
- + " invalid line: admin@\n"
- + " invalid line: admin@example@com");
+ String.format(
+ "invalid code owner config file '/OWNERS' (project = %s, branch = master):\n"
+ + " invalid line: @example.com\n"
+ + " invalid line: admin@\n"
+ + " invalid line: admin@example@com",
+ project));
}
@Test
@@ -158,9 +160,11 @@
getCodeOwnerConfig(EMAIL_1, "INVALID", "NOT_AN_EMAIL", EMAIL_2)));
assertThat(exception.getFullMessage(FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME))
.isEqualTo(
- "invalid code owner config file '/OWNERS':\n"
- + " invalid line: INVALID\n"
- + " invalid line: NOT_AN_EMAIL");
+ String.format(
+ "invalid code owner config file '/OWNERS' (project = %s, branch = master):\n"
+ + " invalid line: INVALID\n"
+ + " invalid line: NOT_AN_EMAIL",
+ project));
}
@Test
@@ -596,7 +600,9 @@
assertThat(exception.getFullMessage(FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME))
.isEqualTo(
String.format(
- "invalid code owner config file '/OWNERS':\n" + " invalid line: %s", invalidLine));
+ "invalid code owner config file '/OWNERS' (project = %s, branch = master):\n"
+ + " invalid line: %s",
+ project, invalidLine));
}
@Test
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java
index 3518a10..3febb50 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java
@@ -90,7 +90,8 @@
"owners_config {\n owner_sets {\nINVALID_LINE\n }\n}\n"));
assertThat(exception.getFullMessage(ProtoBackend.CODE_OWNER_CONFIG_FILE_NAME))
.isEqualTo(
- "invalid code owner config file '/OWNERS_METADATA':\n" + " 4:3: Expected \"{\".");
+ "invalid code owner config file '/OWNERS_METADATA' (project = project, branch = master):\n"
+ + " 4:3: Expected \"{\".");
}
@Test
