Merge changes Ibebb6b55,Iafb1d32f,I2486c293,I2695d16d,If579300d, ...
* changes:
List code owners: Add option to only get code owners with the highest score
AbstractGetCodeOwnersForPath: Make code owner scorings accessible
AbstractGetCodeOwnersForPath: Cleanup how subclasses add further scorings
AbstractGetCodeOwnersForPath: Add a comment
Add distance scores for code owners that are added for resolving all users
Fix sorting in code owner suggestion if file is owned by all users
GetOwnedPath: Limit the number of returned paths
ChangeCodeOwners: Add request object for computing the code owner status
Adapt projectOwnersAreNoCodeOwnersIfDefaultCodeOwnerConfigExists test
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/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index 9a3cc98..cf6e515 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.common.Nullable;
@@ -50,6 +51,7 @@
/** Acceptance test for {@code com.google.gerrit.plugins.codeowners.backend.CodeOwnerSubmitRule}. */
public class CodeOwnerSubmitRuleIT extends AbstractCodeOwnersIT {
@Inject private ProjectOperations projectOperations;
+ @Inject private TestMetricMaker testMetricMaker;
@Test
public void changeIsSubmittableIfCodeOwnersFuctionalityIsDisabled() throws Exception {
@@ -496,6 +498,37 @@
assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.MERGED);
}
+ @Test
+ public void submitRuleIsInvokedOnlyOnceWhenGettingChangeDetails() throws Exception {
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ testMetricMaker.reset();
+ gApi.changes()
+ .id(changeId)
+ .get(ListChangesOption.ALL_REVISIONS, ListChangesOption.CURRENT_ACTIONS);
+
+ // Submit rules are computed freshly, but only once.
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_submit_rule_runs"))
+ .isEqualTo(1);
+ }
+
+ @Test
+ public void submitRuleIsNotInvokedWhenQueryingChange() throws Exception {
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ testMetricMaker.reset();
+ gApi.changes()
+ .query(changeId)
+ .withOptions(ListChangesOption.ALL_REVISIONS, ListChangesOption.CURRENT_ACTIONS)
+ .get();
+
+ // Submit rule evaluation results from the change index are reused
+ assertThat(testMetricMaker.getCount("plugins/code-owners/count_code_owner_submit_rule_runs"))
+ .isEqualTo(0);
+ }
+
private void deleteAutoMergeBranch(ObjectId mergeCommit) throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
RefUpdate ru = repo.updateRef(RefNames.refsCacheAutomerge(mergeCommit.name()));
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
