From 63d6c82515f0995eb8e4cc0d4fa9054dcb179694 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 22 Dec 2015 17:53:19 -0500 Subject: [PATCH] MergeValidationException: Allow arbitrary string messages The purpose of these exceptions is to be thrown by merge validation listeners, which may come from plugins. It doesn't make sense to force plugins to choose a status message from a list of messages in CommitMergeStatus that is a hard-coded enum in core. This restriction on MergeValidationException is how we ended up with a bunch of very specific error messages tightly coupled to the ProjectConfigValidator. Instead, allow arbitrary string messages, which we are now able to effectively report from MergeOp. We don't even need to set the status on the CodeReviewCommit when handling this exception from within MergeOp, since failFast causes the process to abort before we ever inspect the CodeReviewCommits. Change-Id: Iaef3e0a5080522b9812e4709217955c877809567 --- .../gerrit/server/git/CommitMergeStatus.java | 31 +------------- .../com/google/gerrit/server/git/MergeOp.java | 9 +--- .../validators/MergeValidationException.java | 17 ++++---- .../git/validators/MergeValidators.java | 42 ++++++++++++------- plugins/cookbook-plugin | 2 +- 5 files changed, 38 insertions(+), 63 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java index da64332b6e..d8beddce0c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java @@ -65,36 +65,7 @@ public enum CommitMergeStatus { /** */ NOT_FAST_FORWARD("Project policy requires all submissions to be a fast-forward.\n" + "\n" - + "Please rebase the change locally and upload again for review."), - - /** */ - INVALID_PROJECT_CONFIGURATION("Change contains an invalid project configuration."), - - /** */ - INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_PERMITTED( - "Change contains an invalid project configuration:\n" - + "One of the plugin configuration parameters has a value that is not permitted."), - - /** */ - INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_EDITABLE( - "Change contains an invalid project configuration:\n" - + "One of the plugin configuration parameters is not editable."), - - /** */ - INVALID_PROJECT_CONFIGURATION_PARENT_PROJECT_NOT_FOUND( - "Change contains an invalid project configuration:\n" - + "Parent project does not exist."), - - /** */ - INVALID_PROJECT_CONFIGURATION_ROOT_PROJECT_CANNOT_HAVE_PARENT( - "Change contains an invalid project configuration:\n" - + "The root project cannot have a parent."), - - /** */ - SETTING_PARENT_PROJECT_ONLY_ALLOWED_BY_ADMIN( - "Change contains a project configuration that changes the parent project.\n" - + "The change must be submitted by a Gerrit administrator."); - + + "Please rebase the change locally and upload again for review."); private String message; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 414e2e832a..eeeb729bf2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -692,8 +692,7 @@ public class MergeOp implements AutoCloseable { mergeValidators.validatePreMerge( or.repo, commit, or.project, destBranch, ps.getId()); } catch (MergeValidationException mve) { - commit.setStatusCode(mve.getStatus()); - problems.put(changeId, mve.getStatus().toString()); + problems.put(changeId, mve.getMessage()); continue; } @@ -881,12 +880,6 @@ public class MergeOp implements AutoCloseable { case MANUAL_RECURSIVE_MERGE: case CANNOT_CHERRY_PICK_ROOT: case NOT_FAST_FORWARD: - case INVALID_PROJECT_CONFIGURATION: - case INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_PERMITTED: - case INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_EDITABLE: - case INVALID_PROJECT_CONFIGURATION_PARENT_PROJECT_NOT_FOUND: - case INVALID_PROJECT_CONFIGURATION_ROOT_PROJECT_CANNOT_HAVE_PARENT: - case SETTING_PARENT_PROJECT_ONLY_ALLOWED_BY_ADMIN: // TODO(dborowitz): Reformat these messages to be more appropriate for // short problem descriptions. problems.put(id, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidationException.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidationException.java index bfad0e3932..018ec1bc20 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidationException.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidationException.java @@ -14,19 +14,18 @@ package com.google.gerrit.server.git.validators; -import com.google.gerrit.server.git.CommitMergeStatus; import com.google.gerrit.server.validators.ValidationException; +/** + * Exception that occurs during a validation step before merging changes. + *

+ * Used by {@link MergeValidationListener}s provided by plugins. Messages should + * be considered human-readable. + */ public class MergeValidationException extends ValidationException { private static final long serialVersionUID = 1L; - private final CommitMergeStatus status; - public MergeValidationException(CommitMergeStatus status) { - super(status.toString()); - this.status = status; - } - - public CommitMergeStatus getStatus() { - return status; + public MergeValidationException(String msg) { + super(msg); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java index 6f70d46c75..e6585370ba 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java @@ -30,7 +30,6 @@ import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.config.ProjectConfigEntry; import com.google.gerrit.server.git.CodeReviewCommit; -import com.google.gerrit.server.git.CommitMergeStatus; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; @@ -75,6 +74,26 @@ public class MergeValidators { public static class ProjectConfigValidator implements MergeValidationListener { + private static final String INVALID_CONFIG = + "Change contains an invalid project configuration."; + private static final String PARENT_NOT_FOUND = + "Change contains an invalid project configuration:\n" + + "Parent project does not exist."; + private static final String PLUGIN_VALUE_NOT_EDITABLE = + "Change contains an invalid project configuration:\n" + + "One of the plugin configuration parameters is not editable."; + private static final String PLUGIN_VALUE_NOT_PERMITTED = + "Change contains an invalid project configuration:\n" + + "One of the plugin configuration parameters has a value that is not" + + " permitted."; + private static final String ROOT_NO_PARENT = + "Change contains an invalid project configuration:\n" + + "The root project cannot have a parent."; + private static final String SET_BY_ADMIN = + "Change contains a project configuration that changes the parent" + + " project.\n" + + "The change must be submitted by a Gerrit administrator."; + private final AllProjectsName allProjectsName; private final ReviewDb db; private final ProjectCache projectCache; @@ -119,27 +138,23 @@ public class MergeValidators { if (oldParent == null) { // update of the 'All-Projects' project if (newParent != null) { - throw new MergeValidationException(CommitMergeStatus. - INVALID_PROJECT_CONFIGURATION_ROOT_PROJECT_CANNOT_HAVE_PARENT); + throw new MergeValidationException(ROOT_NO_PARENT); } } else { if (!oldParent.equals(newParent)) { PatchSetApproval psa = approvalsUtil.getSubmitter(db, commit.notes(), patchSetId); if (psa == null) { - throw new MergeValidationException(CommitMergeStatus. - SETTING_PARENT_PROJECT_ONLY_ALLOWED_BY_ADMIN); + throw new MergeValidationException(SET_BY_ADMIN); } final IdentifiedUser submitter = identifiedUserFactory.create(psa.getAccountId()); if (!submitter.getCapabilities().canAdministrateServer()) { - throw new MergeValidationException(CommitMergeStatus. - SETTING_PARENT_PROJECT_ONLY_ALLOWED_BY_ADMIN); + throw new MergeValidationException(SET_BY_ADMIN); } if (projectCache.get(newParent) == null) { - throw new MergeValidationException(CommitMergeStatus. - INVALID_PROJECT_CONFIGURATION_PARENT_PROJECT_NOT_FOUND); + throw new MergeValidationException(PARENT_NOT_FOUND); } } } @@ -155,19 +170,16 @@ public class MergeValidators { if ((value == null ? oldValue != null : !value.equals(oldValue)) && !configEntry.isEditable(destProject)) { - throw new MergeValidationException(CommitMergeStatus. - INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_EDITABLE); + throw new MergeValidationException(PLUGIN_VALUE_NOT_EDITABLE); } if (ProjectConfigEntry.Type.LIST.equals(configEntry.getType()) && value != null && !configEntry.getPermittedValues().contains(value)) { - throw new MergeValidationException(CommitMergeStatus. - INVALID_PROJECT_CONFIGURATION_PLUGIN_VALUE_NOT_PERMITTED); + throw new MergeValidationException(PLUGIN_VALUE_NOT_PERMITTED); } } } catch (ConfigInvalidException | IOException e) { - throw new MergeValidationException(CommitMergeStatus. - INVALID_PROJECT_CONFIGURATION); + throw new MergeValidationException(INVALID_CONFIG); } } } diff --git a/plugins/cookbook-plugin b/plugins/cookbook-plugin index eea84e7e07..88b0984638 160000 --- a/plugins/cookbook-plugin +++ b/plugins/cookbook-plugin @@ -1 +1 @@ -Subproject commit eea84e7e07ecf6ebb70ea5a6b0cde67f5a5576af +Subproject commit 88b0984638857ac7139de83b18dc7cad23670b4d