CodeOwnerSubmitRule: Do not return RULE_ERROR for internal server errors
Change I018ce9e13 in Gerrit core redefined RULE_ERROR to be only used
for user-caused errors, but no longer for internal server errors.
Internal server errors in a submit rule should be thrown as a
RuntimeException now.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Id32b1212b4680f13368c070b3cff68ab01e84c54
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index c35d1b0..011e8cf 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -100,6 +100,11 @@
changeData.currentPatchSet().id().get(), changeData.change().getId().get()));
return Optional.of(notReady());
} catch (Throwable t) {
+ // Whether the exception should be treated as RULE_ERROR.
+ // RULE_ERROR must only be returned if the exception is caused by user misconfiguration (e.g.
+ // an invalid OWNERS file), but not for internal server errors.
+ boolean isRuleError = false;
+
String cause = t.getClass().getSimpleName();
String errorMessage = "Failed to evaluate code owner statuses";
if (changeData != null) {
@@ -113,9 +118,11 @@
Optional<InvalidCodeOwnerConfigException> invalidCodeOwnerConfigException =
CodeOwners.getInvalidCodeOwnerConfigCause(t);
if (invalidPathException.isPresent()) {
+ isRuleError = true;
cause = "invalid_path";
errorMessage += String.format(" (cause: %s)", invalidPathException.get().getMessage());
} else if (invalidCodeOwnerConfigException.isPresent()) {
+ isRuleError = true;
codeOwnerMetrics.countInvalidCodeOwnerConfigFiles.increment(
invalidCodeOwnerConfigException.get().getProjectName().get(),
invalidCodeOwnerConfigException.get().getRef(),
@@ -137,7 +144,11 @@
errorMessage += ".";
logger.atSevere().withCause(t).log(errorMessage);
codeOwnerMetrics.countCodeOwnerSubmitRuleErrors.increment(cause);
- return Optional.of(ruleError(errorMessage));
+
+ if (isRuleError) {
+ return Optional.of(ruleError(errorMessage));
+ }
+ throw new CodeOwnersInternalServerErrorException(errorMessage, t);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
index dc709aa..e493e40 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
@@ -14,8 +14,10 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.plugins.codeowners.testing.SubmitRecordSubject.assertThatOptional;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
@@ -120,7 +122,7 @@
}
@Test
- public void ruleError() throws Exception {
+ public void internalServerError() throws Exception {
ChangeData changeData = createChange().getChange();
// Create a ChangeData without change notes to trigger an error.
@@ -129,11 +131,12 @@
when(changeDataWithoutChangeNotes.change()).thenReturn(changeData.change());
when(changeDataWithoutChangeNotes.currentPatchSet()).thenReturn(changeData.currentPatchSet());
- SubmitRecordSubject submitRecordSubject =
- assertThatOptional(codeOwnerSubmitRule.evaluate(changeDataWithoutChangeNotes)).value();
- submitRecordSubject.hasStatusThat().isRuleError();
- submitRecordSubject
- .hasErrorMessageThat()
+ CodeOwnersInternalServerErrorException exception =
+ assertThrows(
+ CodeOwnersInternalServerErrorException.class,
+ () -> codeOwnerSubmitRule.evaluate(changeDataWithoutChangeNotes));
+ assertThat(exception)
+ .hasMessageThat()
.isEqualTo(
String.format(
"Failed to evaluate code owner statuses for patch set %d of change %d.",
@@ -141,11 +144,12 @@
}
@Test
- public void ruleError_changeDataIsNull() throws Exception {
- SubmitRecordSubject submitRecordSubject =
- assertThatOptional(codeOwnerSubmitRule.evaluate(/* changeData= */ null)).value();
- submitRecordSubject.hasStatusThat().isRuleError();
- submitRecordSubject.hasErrorMessageThat().isEqualTo("Failed to evaluate code owner statuses.");
+ public void internalServerError_changeDataIsNull() throws Exception {
+ CodeOwnersInternalServerErrorException exception =
+ assertThrows(
+ CodeOwnersInternalServerErrorException.class,
+ () -> codeOwnerSubmitRule.evaluate(/* changeData= */ null));
+ assertThat(exception).hasMessageThat().isEqualTo("Failed to evaluate code owner statuses.");
}
@Test
