Merge changes I0488a547,Iaef93cfc,Ice1e49e1 * changes: Validate label copy conditions LabelConfigValidator: Factor out validations into methods Mark ApprovalQueryBuilder as a singleton
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index d3b0c2d..5c7d524 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -66,6 +66,7 @@ import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectConfig; import com.google.gerrit.server.project.ProjectState; +import com.google.gerrit.server.query.approval.ApprovalQueryBuilder; import com.google.gerrit.server.ssh.HostKey; import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.util.MagicBranch; @@ -118,6 +119,7 @@ private final Config config; private final ChangeUtil changeUtil; private final MetricMaker metricMaker; + private final ApprovalQueryBuilder approvalQueryBuilder; @Inject Factory( @@ -134,7 +136,8 @@ ProjectCache projectCache, ProjectConfig.Factory projectConfigFactory, ChangeUtil changeUtil, - MetricMaker metricMaker) { + MetricMaker metricMaker, + ApprovalQueryBuilder approvalQueryBuilder) { this.gerritIdent = gerritIdent; this.urlFormatter = urlFormatter; this.config = config; @@ -149,6 +152,7 @@ this.projectConfigFactory = projectConfigFactory; this.changeUtil = changeUtil; this.metricMaker = metricMaker; + this.approvalQueryBuilder = approvalQueryBuilder; } public CommitValidators forReceiveCommits( @@ -181,7 +185,7 @@ .add(new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker, accountCache)) .add(new AccountCommitValidator(repoManager, allUsers, accountValidator)) .add(new GroupCommitValidator(allUsers)) - .add(new LabelConfigValidator()); + .add(new LabelConfigValidator(approvalQueryBuilder)); return new CommitValidators(validators.build()); } @@ -211,7 +215,7 @@ .add(new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker, accountCache)) .add(new AccountCommitValidator(repoManager, allUsers, accountValidator)) .add(new GroupCommitValidator(allUsers)) - .add(new LabelConfigValidator()); + .add(new LabelConfigValidator(approvalQueryBuilder)); return new CommitValidators(validators.build()); }
diff --git a/java/com/google/gerrit/server/project/LabelConfigValidator.java b/java/com/google/gerrit/server/project/LabelConfigValidator.java index 9038132..33e0a87 100644 --- a/java/com/google/gerrit/server/project/LabelConfigValidator.java +++ b/java/com/google/gerrit/server/project/LabelConfigValidator.java
@@ -24,6 +24,7 @@ import com.google.gerrit.entities.LabelFunction; import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.client.ChangeKind; +import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidationListener; @@ -31,6 +32,7 @@ import com.google.gerrit.server.git.validators.ValidationMessage; import com.google.gerrit.server.patch.DiffNotAvailableException; import com.google.gerrit.server.patch.gitdiff.ModifiedFile; +import com.google.gerrit.server.query.approval.ApprovalQueryBuilder; import com.google.inject.Singleton; import java.io.IOException; import java.util.List; @@ -100,6 +102,12 @@ .put(KEY_COPY_ALL_SCORES_IF_LIST_OF_FILES_DID_NOT_CHANGE, "has:unchanged-files") .build(); + private final ApprovalQueryBuilder approvalQueryBuilder; + + public LabelConfigValidator(ApprovalQueryBuilder approvalQueryBuilder) { + this.approvalQueryBuilder = approvalQueryBuilder; + } + @Override public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { @@ -133,93 +141,25 @@ } // Load the old config - Optional<Config> oldConfig = loadOldConfig(receiveEvent); + @Nullable Config oldConfig = loadOldConfig(receiveEvent).orElse(null); for (String labelName : newConfig.getSubsections(ProjectConfig.LABEL)) { - for (String deprecatedFlag : DEPRECATED_FLAGS.keySet()) { - if (flagChangedOrNewlySet(newConfig, oldConfig.orElse(null), labelName, deprecatedFlag)) { - validationMessageBuilder.add( - new CommitValidationMessage( - String.format( - "Parameter '%s.%s.%s' is deprecated and cannot be set," - + " use '%s' in '%s.%s.%s' instead.", - ProjectConfig.LABEL, - labelName, - deprecatedFlag, - DEPRECATED_FLAGS.get(deprecatedFlag), - ProjectConfig.LABEL, - labelName, - ProjectConfig.KEY_COPY_CONDITION), - ValidationMessage.Type.ERROR)); - } - } - - if (copyValuesChangedOrNewlySet(newConfig, oldConfig.orElse(null), labelName)) { - validationMessageBuilder.add( - new CommitValidationMessage( - String.format( - "Parameter '%s.%s.%s' is deprecated and cannot be set," - + " use 'is:<copy-value>' in '%s.%s.%s' instead.", - ProjectConfig.LABEL, - labelName, - KEY_COPY_VALUE, - ProjectConfig.LABEL, - labelName, - ProjectConfig.KEY_COPY_CONDITION), - ValidationMessage.Type.ERROR)); - } - - // Ban modifying label functions to any blocking function value - if (flagChangedOrNewlySet( - newConfig, oldConfig.orElse(null), labelName, ProjectConfig.KEY_FUNCTION)) { - String fnName = - newConfig.getString(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_FUNCTION); - Optional<LabelFunction> labelFn = LabelFunction.parse(fnName); - if (labelFn.isPresent() && !isLabelFunctionAllowed(labelFn.get())) { - validationMessageBuilder.add( - new CommitValidationMessage( - String.format( - "Value '%s' of '%s.%s.%s' is not allowed and cannot be set." - + " Label functions can only be set to {%s, %s, %s}." - + " Use submit requirements instead of label functions.", - fnName, - ProjectConfig.LABEL, - labelName, - ProjectConfig.KEY_FUNCTION, - LabelFunction.NO_BLOCK, - LabelFunction.NO_OP, - LabelFunction.PATCH_SET_LOCK), - ValidationMessage.Type.ERROR)); - } - } - - // Ban deletions of label functions as well since the default is MaxWithBlock - if (flagDeleted(newConfig, oldConfig.orElse(null), labelName, ProjectConfig.KEY_FUNCTION)) { - validationMessageBuilder.add( - new CommitValidationMessage( - String.format( - "Cannot delete '%s.%s.%s'." - + " Label functions can only be set to {%s, %s, %s}." - + " Use submit requirements instead of label functions.", - ProjectConfig.LABEL, - labelName, - ProjectConfig.KEY_FUNCTION, - LabelFunction.NO_BLOCK, - LabelFunction.NO_OP, - LabelFunction.PATCH_SET_LOCK), - ValidationMessage.Type.ERROR)); - } + rejectSettingDeprecatedFlags(newConfig, oldConfig, labelName, validationMessageBuilder); + rejectSettingCopyValues(newConfig, oldConfig, labelName, validationMessageBuilder); + rejectSettingBlockingFunction(newConfig, oldConfig, labelName, validationMessageBuilder); + rejectDeletingFunction(newConfig, oldConfig, labelName, validationMessageBuilder); + rejectNonParseableCopyCondition(newConfig, oldConfig, labelName, validationMessageBuilder); } ImmutableList<CommitValidationMessage> validationMessages = validationMessageBuilder.build(); - if (!validationMessages.isEmpty()) { + if (validationMessages.stream().anyMatch(CommitValidationMessage::isError)) { throw new CommitValidationException( String.format( "invalid %s file in revision %s", ProjectConfig.PROJECT_CONFIG, receiveEvent.commit.getName()), validationMessages); } - return ImmutableList.of(); + return validationMessages; } catch (IOException | DiffNotAvailableException e) { String errorMessage = String.format( @@ -233,6 +173,149 @@ } } + private void rejectSettingDeprecatedFlags( + Config newConfig, + @Nullable Config oldConfig, + String labelName, + ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder) { + for (String deprecatedFlag : DEPRECATED_FLAGS.keySet()) { + if (flagChangedOrNewlySet(newConfig, oldConfig, labelName, deprecatedFlag)) { + validationMessageBuilder.add( + new CommitValidationMessage( + String.format( + "Parameter '%s.%s.%s' is deprecated and cannot be set," + + " use '%s' in '%s.%s.%s' instead.", + ProjectConfig.LABEL, + labelName, + deprecatedFlag, + DEPRECATED_FLAGS.get(deprecatedFlag), + ProjectConfig.LABEL, + labelName, + ProjectConfig.KEY_COPY_CONDITION), + ValidationMessage.Type.ERROR)); + } + } + } + + private void rejectSettingCopyValues( + Config newConfig, + @Nullable Config oldConfig, + String labelName, + ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder) { + if (copyValuesChangedOrNewlySet(newConfig, oldConfig, labelName)) { + validationMessageBuilder.add( + new CommitValidationMessage( + String.format( + "Parameter '%s.%s.%s' is deprecated and cannot be set," + + " use 'is:<copy-value>' in '%s.%s.%s' instead.", + ProjectConfig.LABEL, + labelName, + KEY_COPY_VALUE, + ProjectConfig.LABEL, + labelName, + ProjectConfig.KEY_COPY_CONDITION), + ValidationMessage.Type.ERROR)); + } + } + + private void rejectSettingBlockingFunction( + Config newConfig, + @Nullable Config oldConfig, + String labelName, + ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder) { + if (flagChangedOrNewlySet(newConfig, oldConfig, labelName, ProjectConfig.KEY_FUNCTION)) { + String fnName = + newConfig.getString(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_FUNCTION); + Optional<LabelFunction> labelFn = LabelFunction.parse(fnName); + if (labelFn.isPresent() && !isLabelFunctionAllowed(labelFn.get())) { + validationMessageBuilder.add( + new CommitValidationMessage( + String.format( + "Value '%s' of '%s.%s.%s' is not allowed and cannot be set." + + " Label functions can only be set to {%s, %s, %s}." + + " Use submit requirements instead of label functions.", + fnName, + ProjectConfig.LABEL, + labelName, + ProjectConfig.KEY_FUNCTION, + LabelFunction.NO_BLOCK, + LabelFunction.NO_OP, + LabelFunction.PATCH_SET_LOCK), + ValidationMessage.Type.ERROR)); + } + } + } + + private void rejectDeletingFunction( + Config newConfig, + @Nullable Config oldConfig, + String labelName, + ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder) { + // Ban deletion of label function since the default is MaxWithBlock which is deprecated + if (flagDeleted(newConfig, oldConfig, labelName, ProjectConfig.KEY_FUNCTION)) { + validationMessageBuilder.add( + new CommitValidationMessage( + String.format( + "Cannot delete '%s.%s.%s'." + + " Label functions can only be set to {%s, %s, %s}." + + " Use submit requirements instead of label functions.", + ProjectConfig.LABEL, + labelName, + ProjectConfig.KEY_FUNCTION, + LabelFunction.NO_BLOCK, + LabelFunction.NO_OP, + LabelFunction.PATCH_SET_LOCK), + ValidationMessage.Type.ERROR)); + } + } + + private void rejectNonParseableCopyCondition( + Config newConfig, + @Nullable Config oldConfig, + String labelName, + ImmutableList.Builder<CommitValidationMessage> validationMessageBuilder) { + if (flagChangedOrNewlySet(newConfig, oldConfig, labelName, ProjectConfig.KEY_COPY_CONDITION)) { + String copyCondition = + newConfig.getString(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_COPY_CONDITION); + try { + @SuppressWarnings("unused") + var unused = approvalQueryBuilder.parse(copyCondition); + } catch (QueryParseException e) { + validationMessageBuilder.add( + new CommitValidationMessage( + String.format( + "Cannot parse copy condition '%s' of label %s (parameter '%s.%s.%s'): %s", + copyCondition, + labelName, + ProjectConfig.LABEL, + labelName, + ProjectConfig.KEY_COPY_CONDITION, + e.getMessage()), + // if the old copy condition is not parseable allow updating it even if the new copy + // condition is also not parseable, only emit a warning in this case + hasUnparseableOldCopyCondition(oldConfig, labelName) + ? ValidationMessage.Type.WARNING + : ValidationMessage.Type.ERROR)); + } + } + } + + private boolean hasUnparseableOldCopyCondition(@Nullable Config oldConfig, String labelName) { + if (oldConfig == null) { + return false; + } + + String oldCopyCondition = + oldConfig.getString(ProjectConfig.LABEL, labelName, ProjectConfig.KEY_COPY_CONDITION); + try { + @SuppressWarnings("unused") + var unused = approvalQueryBuilder.parse(oldCopyCondition); + return false; + } catch (QueryParseException e) { + return true; + } + } + /** * Whether the given file was changed in the given revision. *
diff --git a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java index ed876c1..6ae47ad 100644 --- a/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java +++ b/java/com/google/gerrit/server/query/approval/ApprovalQueryBuilder.java
@@ -27,10 +27,12 @@ import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.group.GroupResolver; import com.google.inject.Inject; +import com.google.inject.Singleton; import java.util.Arrays; import java.util.Locale; import java.util.Optional; +@Singleton public class ApprovalQueryBuilder extends QueryBuilder<ApprovalContext, ApprovalQueryBuilder> { private static final QueryBuilder.Definition<ApprovalContext, ApprovalQueryBuilder> mydef = new QueryBuilder.Definition<>(ApprovalQueryBuilder.class);
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java index 55e71ca..9a926ea 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -39,6 +39,7 @@ import com.google.gerrit.extensions.common.ChangeInput; import com.google.gerrit.git.ObjectIds; import com.google.gerrit.server.config.ProjectConfigEntry; +import com.google.gerrit.server.git.validators.ValidationMessage; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.project.GroupList; import com.google.gerrit.server.project.LabelConfigValidator; @@ -1181,7 +1182,100 @@ } @Test - public void setCopyCondition() throws Exception { + public void testSettingCopyCondition() throws Exception { + testChangingCopyCondition(/* initialCopyCondition= */ null, /* newCopyCondition= */ "is:ANY"); + } + + @Test + public void testRejectNonParseableCopyCondition_badSyntax() throws Exception { + testChangingCopyConditionExpectError( + /* initialCopyCondition= */ "is:ANY", + /* newCopyCondition= */ ":", + /* errorMessage= */ "Cannot parse copy condition ':' of label Foo (parameter" + + " 'label.Foo.copyCondition'): line 1:0 no viable alternative at input ':'"); + } + + @Test + public void testRejectNonParseableCopyCondition_unsupportedOperator() throws Exception { + testChangingCopyConditionExpectError( + /* initialCopyCondition= */ "is:ANY", + /* newCopyCondition= */ "foo:bar", + /* errorMessage= */ "Cannot parse copy condition 'foo:bar' of label Foo (parameter" + + " 'label.Foo.copyCondition'): unsupported operator foo:bar"); + } + + @Test + public void testFixNonParseableCopyCondition() throws Exception { + testChangingCopyCondition(/* initialCopyCondition= */ ":", /* newCopyCondition= */ "is:ANY"); + } + + @Test + public void testChangingCopyCondition() throws Exception { + testChangingCopyCondition( + /* initialCopyCondition= */ "is:ANY", /* newCopyCondition= */ "is:MAX"); + } + + @Test + public void testDeletingCopyCondition() throws Exception { + testChangingCopyCondition(/* initialCopyCondition= */ "is:ANY", /* newCopyCondition= */ null); + } + + @Test + public void testDeletingNonParseableCopyCondition() throws Exception { + testChangingCopyCondition(/* initialCopyCondition= */ ":", /* newCopyCondition= */ null); + } + + @Test + public void testChangingNonParseableCopyCondition() throws Exception { + testChangingCopyConditionExpectWarning( + /* initialCopyCondition= */ ":", + /* newCopyCondition= */ ":foo", + /* warningMessage= */ "Cannot parse copy condition ':foo' of label Foo (parameter" + + " 'label.Foo.copyCondition'): line 1:0 no viable alternative at input ':'"); + } + + private void testChangingCopyCondition( + String initialCopyCondition, @Nullable String newCopyCondition) throws Exception { + testChangingCopyCondition( + initialCopyCondition, newCopyCondition, /* type= */ null, /* message= */ null); + } + + private void testChangingCopyConditionExpectError( + String initialCopyCondition, @Nullable String newCopyCondition, String errorMessage) + throws Exception { + testChangingCopyCondition( + initialCopyCondition, newCopyCondition, ValidationMessage.Type.ERROR, errorMessage); + } + + private void testChangingCopyConditionExpectWarning( + String initialCopyCondition, @Nullable String newCopyCondition, String warningMessage) + throws Exception { + testChangingCopyCondition( + initialCopyCondition, newCopyCondition, ValidationMessage.Type.WARNING, warningMessage); + } + + private void testChangingCopyCondition( + @Nullable String initialCopyCondition, + @Nullable String newCopyCondition, + @Nullable ValidationMessage.Type type, + @Nullable String message) + throws Exception { + if (initialCopyCondition != null) { + try (TestRepository<Repository> testRepo = + new TestRepository<>(repoManager.openRepository(project))) { + testRepo + .branch(RefNames.REFS_CONFIG) + .commit() + .add( + ProjectConfig.PROJECT_CONFIG, + String.format( + "[label \"Foo\"]\n %s = %s\n", + ProjectConfig.KEY_COPY_CONDITION, initialCopyCondition)) + .parent(projectOperations.project(project).getHead(RefNames.REFS_CONFIG)) + .create(); + } + } + fetchRefsMetaConfig(); PushOneCommit push = pushFactory.create( @@ -1189,9 +1283,22 @@ testRepo, "Test Change", ProjectConfig.PROJECT_CONFIG, - String.format("[label \"Foo\"]\n %s = is:ANY", ProjectConfig.KEY_COPY_CONDITION)); + newCopyCondition == null + ? "[label \"Foo\"]\n" + : String.format( + "[label \"Foo\"]\n %s = %s\n", + ProjectConfig.KEY_COPY_CONDITION, newCopyCondition)); PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG); - r.assertOkStatus(); + if (!ValidationMessage.Type.ERROR.equals(type)) { + r.assertOkStatus(); + return; + } + r.assertErrorStatus( + String.format( + "invalid %s file in revision %s", ProjectConfig.PROJECT_CONFIG, r.getCommit().name())); + if (message != null) { + r.assertMessage(message); + } } @Test