Merge "Document when using the code-owners plugin is not recommended"
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
index 2bf9bff..a0df99d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackend.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -113,10 +114,10 @@
fileName, codeOwnerConfigParser, revWalk, revision, codeOwnerConfigKey);
}
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("failed to load code owner config %s", codeOwnerConfigKey), e);
} catch (ConfigInvalidException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"invalid code owner config file %s (project = %s, branch = %s)",
codeOwnerConfigKey.filePath(defaultFileName),
@@ -209,7 +210,7 @@
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("failed to upsert code owner config %s", codeOwnerConfigKey), e);
}
}
@@ -257,7 +258,7 @@
return codeOwnerConfigFile.getLoadedCodeOwnerConfig();
} catch (IOException | ConfigInvalidException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("failed to upsert code owner config %s", codeOwnerConfigKey), e);
}
}
@@ -279,7 +280,7 @@
}
return metaDataUpdate;
} catch (Exception e) {
- throw new CodeOwnersInternalServerErrorException("Failed to create MetaDataUpdate", e);
+ throw newInternalServerError("Failed to create MetaDataUpdate", e);
} finally {
metaDataUpdate.close();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index 97ece59..a04c1ad 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -172,7 +173,7 @@
.orElse(null)))
.collect(toImmutableList());
} catch (IOException | DiffNotAvailableException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to compute owned paths of patch set %s for account %d",
patchSet.id(), accountId.get()),
@@ -474,7 +475,7 @@
return changeNotes.getChange().getRevertOf() != null
&& pureRevertCache.isPureRevert(changeNotes);
} catch (BadRequestException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to check if change %s in project %s is a pure revert",
changeNotes.getChangeId(), changeNotes.getProjectName()),
@@ -740,7 +741,7 @@
implicitApprover, reviewerAccountIds, approverAccountIds, absolutePath, reason);
}
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("unknown fallback code owners configured: %s", fallbackCodeOwners));
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
index 81e6ab5..d0bd003 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Objects.requireNonNull;
@@ -140,7 +141,7 @@
updateBranch(branchNameKey.branch(), repository, revision, commitId);
return Optional.of(rw.parseCommit(commitId));
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to scan for code owner configs in branch %s of project %s",
branchNameKey.branch(), branchNameKey.project()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index 8fb6d09..28dd5e1 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
@@ -293,8 +294,7 @@
logger.atFine().log("code owner config %s not found", metaCodeOwnerConfigKey);
}
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
- String.format("failed to read %s", metaCodeOwnerConfigKey), e);
+ throw newInternalServerError(String.format("failed to read %s", metaCodeOwnerConfigKey), e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
index 20d7a4c..d55ff8f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigScanner.java
@@ -15,6 +15,7 @@
package com.google.gerrit.plugins.codeowners.backend;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidCodeOwnerConfigCause;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.flogger.FluentLogger;
@@ -162,7 +163,7 @@
}
}
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to scan for code owner configs in branch %s of project %s",
branchNameKey.branch(), branchNameKey.project()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 254b3a6..20cba7d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.annotations.VisibleForTesting;
@@ -595,7 +596,7 @@
});
return extIdsByEmail;
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("cannot resolve code owner emails: %s", emails), e);
}
}
@@ -868,7 +869,7 @@
return true;
}
} catch (PermissionBackendException ex) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to test the %s global capability", GlobalPermission.MODIFY_ACCOUNT),
ex);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index 58a500e..80300d6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.common.collect.ImmutableList;
@@ -150,7 +151,7 @@
logger.atWarning().log("%s", errorMessage);
return Optional.of(ruleError(errorMessage));
}
- throw new CodeOwnersInternalServerErrorException(errorMessage, e);
+ throw newInternalServerError(errorMessage, e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
index c7da676..5eb316c 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHook.java
@@ -52,9 +52,7 @@
@Override
public boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
- return isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)
- || isInvalidPathException(throwable);
+ return isCausedByConfigurationError(throwable);
}
@Override
@@ -102,9 +100,7 @@
@Override
public Optional<Status> getStatus(Throwable throwable) {
- if (isInvalidPluginConfigurationException(throwable)
- || isInvalidCodeOwnerConfigException(throwable)
- || isInvalidPathException(throwable)) {
+ if (isCausedByConfigurationError(throwable)) {
return Optional.of(Status.create(409, "Conflict"));
}
return Optional.empty();
@@ -115,6 +111,28 @@
return getCause(CodeOwnersInternalServerErrorException.class, throwable);
}
+ public static boolean isCausedByConfigurationError(Throwable throwable) {
+ return isInvalidPluginConfigurationException(throwable)
+ || isInvalidCodeOwnerConfigException(throwable)
+ || isInvalidPathException(throwable);
+ }
+
+ public static Optional<? extends Exception> getCauseOfConfigurationError(Throwable throwable) {
+ Optional<InvalidPathException> invalidPathException =
+ CodeOwnersExceptionHook.getInvalidPathException(throwable);
+ if (invalidPathException.isPresent()) {
+ return invalidPathException;
+ }
+
+ Optional<InvalidPluginConfigurationException> invalidPluginConfigurationException =
+ CodeOwnersExceptionHook.getInvalidPluginConfigurationCause(throwable);
+ if (invalidPluginConfigurationException.isPresent()) {
+ return invalidPluginConfigurationException;
+ }
+
+ return CodeOwners.getInvalidCodeOwnerConfigCause(throwable);
+ }
+
private static boolean isInvalidPluginConfigurationException(Throwable throwable) {
return getInvalidPluginConfigurationCause(throwable).isPresent();
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java
index 13b1a9d..580414a 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorException.java
@@ -14,21 +14,65 @@
package com.google.gerrit.plugins.codeowners.backend;
+import com.google.common.base.Throwables;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+
/** Exception signaling an internal server error in the code-owners plugin. */
public class CodeOwnersInternalServerErrorException extends RuntimeException {
private static final long serialVersionUID = 1L;
private static final String USER_MESSAGE = "Internal server in code-owners plugin";
- public CodeOwnersInternalServerErrorException(String message) {
+ /**
+ * Creates a {@link CodeOwnersInternalServerErrorException} to signal an internal server error
+ * caused by an issue in the code-owners plugin.
+ *
+ * @param message the exception message
+ * @return the created exception
+ */
+ public static CodeOwnersInternalServerErrorException newInternalServerError(String message) {
+ return new CodeOwnersInternalServerErrorException(message);
+ }
+
+ /**
+ * Creates a {@link RuntimeException} to signal an internal server error.
+ *
+ * <p>By default it is assumed that the internal server error is caused by an issue in the
+ * code-owners plugin and a {@link CodeOwnersInternalServerErrorException} is returned.
+ *
+ * <p>However for some known causes that are unrelated to code owners a {@link StorageException}
+ * is thrown. This is to avoid that the code-owners plugin is mistakenly assumed to be the cause
+ * of these errors.
+ *
+ * @param message the exception message
+ * @param cause the exception cause
+ * @return the created exception
+ */
+ public static RuntimeException newInternalServerError(String message, Throwable cause) {
+ if (isNonCodeOwnersCause(cause)) {
+ return new StorageException(message, cause);
+ }
+ return new CodeOwnersInternalServerErrorException(message, cause);
+ }
+
+ private CodeOwnersInternalServerErrorException(String message) {
super(message);
}
- public CodeOwnersInternalServerErrorException(String message, Throwable cause) {
+ private CodeOwnersInternalServerErrorException(String message, Throwable cause) {
super(message, cause);
}
public String getUserVisibleMessage() {
return USER_MESSAGE;
}
+
+ private static boolean isNonCodeOwnersCause(Throwable throwable) {
+ return hasCause(DiffNotAvailableException.class, throwable);
+ }
+
+ private static boolean hasCause(Class<?> exceptionClass, Throwable throwable) {
+ return Throwables.getCausalChain(throwable).stream().anyMatch(exceptionClass::isInstance);
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
index 4e81f46..ae1f477 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnersOnAddReviewer.java
@@ -165,9 +165,17 @@
})
.call();
} catch (Exception e) {
- logger.atSevere().withCause(e).log(
- "Failed to post code-owners change message for reviewer on change %s in project %s.",
- changeId, projectName);
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for reviewer on change %s in project %s: %s",
+ changeId, projectName, configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for reviewer on change %s in project %s.",
+ changeId, projectName);
+ }
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
index b538ebd..907f065 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/OnCodeOwnerApproval.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableList;
+import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
@@ -51,6 +52,8 @@
*/
@Singleton
class OnCodeOwnerApproval implements OnPostReview {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerApprovalCheck codeOwnerApprovalCheck;
private final CodeOwnerMetrics codeOwnerMetrics;
@@ -103,6 +106,23 @@
approvals,
requiredApproval,
maxPathsInChangeMessage);
+ } catch (Exception e) {
+ Optional<? extends Exception> configurationError =
+ CodeOwnersExceptionHook.getCauseOfConfigurationError(e);
+ if (configurationError.isPresent()) {
+ logger.atWarning().log(
+ "Failed to post code-owners change message for code owner approval on change %s"
+ + " in project %s: %s",
+ changeNotes.getChangeId(),
+ changeNotes.getProjectName(),
+ configurationError.get().getMessage());
+ } else {
+ logger.atSevere().withCause(e).log(
+ "Failed to post code-owners change message for code owner approval on change %s"
+ + " in project %s.",
+ changeNotes.getChangeId(), changeNotes.getProjectName());
+ }
+ return Optional.empty();
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
index e8a1ab7..b1a38fa 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/TransientCodeOwnerConfigCache.java
@@ -14,6 +14,8 @@
package com.google.gerrit.plugins.codeowners.backend;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
+
import com.google.auto.value.AutoValue;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -124,7 +126,7 @@
}
return Optional.of(ref.getObjectId());
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"failed to get revision of branch %s in project %s",
branchNameKey.shortName(), branchNameKey.project()),
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index dea86af..f3b6418 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -14,6 +14,7 @@
package com.google.gerrit.plugins.codeowners.backend.config;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static java.util.Objects.requireNonNull;
@@ -30,7 +31,6 @@
import com.google.gerrit.entities.Project;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.EnableImplicitApprovals;
import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners;
import com.google.gerrit.plugins.codeowners.backend.PathExpressions;
@@ -332,7 +332,7 @@
return ImmutableSet.copyOf(exemptedAccounts.values());
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to resolve exempted users %s on project %s", exemptedUsers, projectName),
e);
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index 56aa627..677e67c 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -18,6 +18,7 @@
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.IS_EXPLICITLY_MENTIONED_SCORING_VALUE;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore.NOT_EXPLICITLY_MENTIONED_SCORING_VALUE;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
@@ -46,7 +47,6 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.account.AccountControl;
@@ -580,7 +580,7 @@
throw new IllegalStateException("unknown account visibility setting: " + accountVisibility);
} catch (IOException | PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException("failed to get visible users", e);
+ throw newInternalServerError("failed to get visible users", e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 07828b9..c4fd816 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -18,6 +18,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwners.getInvalidCodeOwnerConfigCause;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
import com.google.auto.value.AutoValue;
@@ -462,7 +463,7 @@
"failed to validate code owner config files in revision %s"
+ " (project = %s, branch = %s)",
revCommit.getName(), branchNameKey.project(), branchNameKey.branch());
- throw new CodeOwnersInternalServerErrorException(errorMessage, e);
+ throw newInternalServerError(errorMessage, e);
}
}
@@ -891,7 +892,7 @@
.filter(Optional::isPresent)
.map(Optional::get);
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format("Failed to validate imports for %s in ", codeOwnerConfig.key()), e);
}
}
@@ -1061,7 +1062,7 @@
.project(keyOfImportedCodeOwnerConfig.project())
.test(ProjectPermission.ACCESS);
} catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
"failed to check read permission for project of imported code owner config", e);
}
}
@@ -1074,7 +1075,7 @@
.ref(keyOfImportedCodeOwnerConfig.ref())
.test(RefPermission.READ);
} catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
"failed to check read permission for branch of imported code owner config", e);
}
}
@@ -1095,8 +1096,7 @@
return Optional.ofNullable(repo.exactRef(keyOfImportedCodeOwnerConfig.ref()))
.map(Ref::getObjectId);
} catch (IOException e) {
- throw new CodeOwnersInternalServerErrorException(
- "failed to read revision of import code owner config", e);
+ throw newInternalServerError("failed to read revision of import code owner config", e);
}
}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
index bc7ac6b..4417295 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
@@ -14,11 +14,12 @@
package com.google.gerrit.plugins.codeowners.validation;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
+
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.server.git.receive.PluginPushOption;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -103,7 +104,7 @@
.currentUser()
.check(skipCodeOwnerConfigValidationCapability.getPermission());
} catch (PermissionBackendException e) {
- throw new CodeOwnersInternalServerErrorException(
+ throw newInternalServerError(
String.format(
"Failed to check %s capability", SkipCodeOwnerConfigValidationCapability.ID),
e);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
index bfc0283..70fb164 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnersOnAddReviewerIT.java
@@ -83,6 +83,18 @@
}
@Test
+ public void noChangeMessageAddedIfInvalidCodeOwnerConfigFilesExist() throws Exception {
+ createNonParseableCodeOwnerConfig(getCodeOwnerConfigFileName());
+
+ String changeId = createChange("Test Change", "foo/bar.baz", "file content").getChangeId();
+
+ gApi.changes().id(changeId).addReviewer(user.email());
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Uploaded patch set 1.");
+ }
+
+ @Test
public void changeMessageListsOwnedPaths() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
index efd2166..d3fe19a 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/OnCodeOwnerApprovalIT.java
@@ -65,6 +65,19 @@
}
@Test
+ public void changeMessageNotExtendedIfInvalidCodeOwnerConfigFilesExist() throws Exception {
+ createNonParseableCodeOwnerConfig(getCodeOwnerConfigFileName());
+
+ String path = "foo/bar.baz";
+ String changeId = createChange("Test Change", path, "file content").getChangeId();
+
+ recommend(changeId);
+
+ Collection<ChangeMessageInfo> messages = gApi.changes().id(changeId).get().messages;
+ assertThat(Iterables.getLast(messages).message).isEqualTo("Patch Set 1: Code-Review+1");
+ }
+
+ @Test
public void changeMessageListsNewlyApprovedPaths() throws Exception {
codeOwnerConfigOperations
.newCodeOwnerConfig()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
index 1c0f9c1..e1786d3 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersExceptionHookTest.java
@@ -49,10 +49,14 @@
assertThat(skipRetryWithTrace(newInvalidPathException())).isTrue();
assertThat(skipRetryWithTrace(newExceptionWithCause(newInvalidPathException()))).isTrue();
- assertThat(skipRetryWithTrace(new CodeOwnersInternalServerErrorException("msg"))).isFalse();
assertThat(
skipRetryWithTrace(
- newExceptionWithCause(new CodeOwnersInternalServerErrorException("msg"))))
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg")))
+ .isFalse();
+ assertThat(
+ skipRetryWithTrace(
+ newExceptionWithCause(
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg"))))
.isFalse();
assertThat(skipRetryWithTrace(new Exception())).isFalse();
@@ -82,7 +86,7 @@
.containsExactly(invalidPathException.getMessage());
CodeOwnersInternalServerErrorException codeOwnersInternalServerErrorException =
- new CodeOwnersInternalServerErrorException("msg");
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg");
assertThat(getUserMessages(codeOwnersInternalServerErrorException))
.containsExactly(codeOwnersInternalServerErrorException.getUserVisibleMessage());
assertThat(getUserMessages(newExceptionWithCause(codeOwnersInternalServerErrorException)))
@@ -130,8 +134,12 @@
assertThat(getStatus(new Exception())).isEmpty();
assertThat(getStatus(newExceptionWithCause(new Exception()))).isEmpty();
- assertThat(getStatus(new CodeOwnersInternalServerErrorException("msg"))).isEmpty();
- assertThat(getStatus(newExceptionWithCause(new CodeOwnersInternalServerErrorException("msg"))))
+ assertThat(getStatus(CodeOwnersInternalServerErrorException.newInternalServerError("msg")))
+ .isEmpty();
+ assertThat(
+ getStatus(
+ newExceptionWithCause(
+ CodeOwnersInternalServerErrorException.newInternalServerError("msg"))))
.isEmpty();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorExceptionTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorExceptionTest.java
new file mode 100644
index 0000000..ac1e583
--- /dev/null
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnersInternalServerErrorExceptionTest.java
@@ -0,0 +1,49 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.plugins.codeowners.backend;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
+
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
+import org.junit.Test;
+
+/** Tests for {@link CodeOwnersInternalServerErrorException}. */
+public class CodeOwnersInternalServerErrorExceptionTest extends AbstractCodeOwnersTest {
+ @Test
+ public void codeOwnersInternalServerErrorExceptionIsCreatedByDefault() {
+ assertThat(newInternalServerError("foo", new NullPointerException("bar")))
+ .isInstanceOf(CodeOwnersInternalServerErrorException.class);
+ assertThat(
+ newInternalServerError("foo", newExceptionWithCause(new NullPointerException("bar"))))
+ .isInstanceOf(CodeOwnersInternalServerErrorException.class);
+ }
+
+ @Test
+ public void storageExceptionIsCreatedForNonCodeOwnerErrors() {
+ assertThat(newInternalServerError("foo", new DiffNotAvailableException("bar")))
+ .isInstanceOf(StorageException.class);
+ assertThat(
+ newInternalServerError(
+ "foo", newExceptionWithCause(new DiffNotAvailableException("bar"))))
+ .isInstanceOf(StorageException.class);
+ }
+
+ private Exception newExceptionWithCause(Exception cause) {
+ return new Exception("exception1", new Exception("exception2", cause));
+ }
+}
