opendev/gerrit
Code Issues Proposed changes

MergeValidationException: Allow arbitrary string messages

Browse Source 
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
This commit is contained in:
Dave Borowitz 2015-12-22 17:53:19 -05:00
parent b958579cb1
commit 63d6c82515
5 changed files with 38 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java
View File

@ -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;

9
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@ -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,

17
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidationException.java
View File

@ -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.
* <p>
* 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);
}
}

42
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@ -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);
}
}
}

2
plugins/cookbook-plugin

@ -1 +1 @@
Subproject commit eea84e7e07ecf6ebb70ea5a6b0cde67f5a5576af
Subproject commit 88b0984638857ac7139de83b18dc7cad23670b4d