Validate label copy conditions When updating copy conditions of labels validate them and reject non-parseable copy conditions, except if the old copy condition was also not parseable (in this case just emit a warning). This prevents project owners from accidentally breaking copy conditions, which would cause errors when creating new patch sets later. Bug: Google b/241366765 Release-Notes: Added validation for label copy conditions Change-Id: I0488a54722b0080dad27634aa9281ab600c6873c Signed-off-by: Edwin Kempin <ekempin@google.com>
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 9944401..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 { @@ -140,17 +148,18 @@ 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( @@ -260,6 +269,53 @@ } } + 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/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