From 91375a26750603ebc1f24de48bb2c349d306e061 Mon Sep 17 00:00:00 2001 From: Chris Foote Date: Wed, 20 Jan 2021 00:58:14 +1300 Subject: [PATCH] Improve UI validation and add more tests. --- .../RequiredResourcesProperty.java | 59 ++++++++++----- .../lockableresources/queue/Utils.java | 48 +++++++++++- .../lockableresources/Messages.properties | 11 ++- .../FreeStyleProjectTest.java | 32 ++++++++ .../lockableresources/LockStepTest.java | 1 + .../LockableResourceManagerTest.java | 73 ++++++++++++++++--- 6 files changed, 190 insertions(+), 34 deletions(-) diff --git a/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java b/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java index 1a707518f..50218bb3b 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java @@ -159,9 +159,9 @@ public FormValidation doCheckResourceNames( @QueryParameter String value, @QueryParameter String labelName, @QueryParameter boolean script, - @AncestorInPath Item item) { + @AncestorInPath AbstractProject project) { // check permission, security first - checkPermission(item); + checkPermission(project); String labelVal = Util.fixEmptyAndTrim(labelName); String names = Util.fixEmptyAndTrim(value); @@ -173,24 +173,29 @@ public FormValidation doCheckResourceNames( } else { List wrongNames = new ArrayList<>(); List varNames = new ArrayList<>(); + List unknownParams = new ArrayList<>(); for (String name : names.split("\\s+")) { boolean found = LockableResourcesManager.get().resourceExist(name); if (!found) { - if (Utils.isVariable(name)) { + if (Utils.containsParameter(name)) { + List badParams = Utils.checkParameters(name, project); + if (!badParams.isEmpty()) { + unknownParams.addAll(badParams); + } varNames.add(name); } else { wrongNames.add(name); } } } - if (wrongNames.isEmpty() && varNames.isEmpty()) { + if (wrongNames.isEmpty() && varNames.isEmpty() && unknownParams.isEmpty()) { return FormValidation.ok(); - } else if (wrongNames.isEmpty()) { - return FormValidation - .warning("The following resources cannot be validated as they are the environment variables: " - + varNames); - } else { + } else if (!wrongNames.isEmpty()) { return FormValidation.error(Messages.error_resourceDoesNotExist(wrongNames)); + } else if (!unknownParams.isEmpty()) { + return FormValidation.error(Messages.error_parameterDoesNotExist(unknownParams)); + } else { + return FormValidation.warning(Messages.warn_resourceNotValidated(varNames)); } } } @@ -200,9 +205,9 @@ public FormValidation doCheckLabelName( @QueryParameter String value, @QueryParameter String resourceNames, @QueryParameter boolean script, - @AncestorInPath Item item) { + @AncestorInPath AbstractProject project) { // check permission, security first - checkPermission(item); + checkPermission(project); String label = Util.fixEmptyAndTrim(value); String names = Util.fixEmptyAndTrim(resourceNames); @@ -214,6 +219,12 @@ public FormValidation doCheckLabelName( } else { if (LockableResourcesManager.get().isValidLabel(label)) { return FormValidation.ok(); + } else if (Utils.containsParameter(label)) { + List badParams = Utils.checkParameters(label, project); + if (!badParams.isEmpty()) { + return FormValidation.error(Messages.error_parameterDoesNotExist(badParams)); + } + return FormValidation.warning(Messages.warn_labelNotValidated(label)); } else { return FormValidation.error(Messages.error_labelDoesNotExist(label)); } @@ -226,35 +237,45 @@ public FormValidation doCheckResourceNumber( @QueryParameter String resourceNames, @QueryParameter String labelName, @QueryParameter String resourceMatchScript, - @AncestorInPath Item item) { + @AncestorInPath AbstractProject project) { // check permission, security first - checkPermission(item); + checkPermission(project); String number = Util.fixEmptyAndTrim(value); String names = Util.fixEmptyAndTrim(resourceNames); String label = Util.fixEmptyAndTrim(labelName); String script = Util.fixEmptyAndTrim(resourceMatchScript); - if (number == null || number.isEmpty() || number.trim().equals("0")) { + if (number == null || number.equals("0")) { return FormValidation.ok(); } - int numAsInt; + int numAsInt = 0; try { numAsInt = Integer.parseInt(number); } catch (NumberFormatException e) { - return FormValidation.error(Messages.error_couldNotParseToint()); + if (Utils.isParameter(number)) { + List badParams = Utils.checkParameters(number, project); + if (!badParams.isEmpty()) { + return FormValidation.error(Messages.error_parameterDoesNotExist(badParams)); + } + return FormValidation.warning(Messages.warn_valueNotValidated(number)); + } + return FormValidation.error(Messages.error_couldNotParseToInt()); } + int numResources = 0; if (names != null) { numResources = names.split("\\s+").length; - } else if (label != null || script != null) { + } else if (label != null) { + numResources = LockableResourcesManager.get().getResourcesWithLabel(label, null).size(); + } else if (script != null) { numResources = Integer.MAX_VALUE; } - if (numResources < numAsInt) { + if (numAsInt > numResources) { return FormValidation.error( - String.format(Messages.error_givenAmountIsGreaterThatResurcesAmount(), numAsInt, numResources)); + String.format(Messages.error_givenAmountIsGreaterThanResourcesAmount(), numAsInt, numResources)); } return FormValidation.ok(); } diff --git a/src/main/java/org/jenkins/plugins/lockableresources/queue/Utils.java b/src/main/java/org/jenkins/plugins/lockableresources/queue/Utils.java index 605ce2e12..04074c473 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/queue/Utils.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/queue/Utils.java @@ -12,12 +12,22 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.EnvVars; import hudson.matrix.MatrixConfiguration; +import hudson.model.AbstractProject; import hudson.model.Job; +import hudson.model.ParametersDefinitionProperty; import hudson.model.Queue; import hudson.model.Run; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.regex.Matcher; import java.util.regex.Pattern; + import org.jenkins.plugins.lockableresources.RequiredResourcesProperty; +import javax.annotation.Nonnull; + public final class Utils { private Utils() {} @@ -26,6 +36,11 @@ private Utils() {} */ private static final Pattern VARIABLE = Pattern.compile("\\$([A-Za-z0-9_]+|\\{[A-Za-z0-9_.]+\\})"); + /** + * Pattern for capturing parameters. ${xyz} but not $${xyz} + */ + private static final Pattern PARAMETER = Pattern.compile("(? getProject(@NonNull Queue.Item item) { if (item.task instanceof Job) return (Job) item.task; @@ -38,8 +53,7 @@ private Utils() {} } @CheckForNull - public static LockableResourcesStruct requiredResources(@NonNull Job project, - EnvVars env) { + public static LockableResourcesStruct requiredResources(@NonNull Job project, EnvVars env) { if (project instanceof MatrixConfiguration) { env.putAll(((MatrixConfiguration) project).getCombination()); @@ -55,4 +69,34 @@ public static LockableResourcesStruct requiredResources(@NonNull Job proje public static boolean isVariable(String name) { return VARIABLE.matcher(name).matches(); } + + @Nonnull + public static List getProjectParameterNames(AbstractProject project) { + ParametersDefinitionProperty params = project.getProperty(ParametersDefinitionProperty.class); + if (params != null) + return params.getParameterDefinitionNames(); + return Collections.emptyList(); + } + + public static boolean isParameter(String s) { + return PARAMETER.matcher(s).matches(); + } + + public static boolean containsParameter(String s) { + return PARAMETER.matcher(s).find(); + } + + @Nonnull + public static List checkParameters(String s, AbstractProject project) { + List unknownParameters = new ArrayList<>(); + List paramNames = getProjectParameterNames(project); + Matcher m = PARAMETER.matcher(s); + while (m.find()) { + String param = m.group(1); + if (!paramNames.contains(param)) { + unknownParameters.add(param); + } + } + return unknownParameters; + } } diff --git a/src/main/resources/org/jenkins/plugins/lockableresources/Messages.properties b/src/main/resources/org/jenkins/plugins/lockableresources/Messages.properties index 7c85b5e1a..288df71f0 100644 --- a/src/main/resources/org/jenkins/plugins/lockableresources/Messages.properties +++ b/src/main/resources/org/jenkins/plugins/lockableresources/Messages.properties @@ -20,15 +20,19 @@ LockableResourcesRootAction.StealPermission.Description=This permission grants t LockableResourcesRootAction.ViewPermission=View LockableResourcesRootAction.ViewPermission.Description=This permission grants the ability to view \ lockable resources. -LockedResourcesBuildAction.displayName=Used lockable resources +# Java warnings +warn.labelNotValidated=The resource label cannot be validated as it contains parameters: {0}. +warn.resourceNotValidated=The resource cannot be validated as it contains parameters: {0}. +warn.valueNotValidated=The value cannot be validated as it is a parameter value: {0}. # Java errors error.labelDoesNotExist=The resource label does not exist: {0}. error.resourceDoesNotExist=The resource does not exist: {0}. +error.parameterDoesNotExist=The parameter does not exist: {0}. error.labelOrNameMustBeSpecified=Either resource label or resource name must be specified. error.labelAndNameSpecified=Resource label and resource name cannot be specified simultaneously. error.labelAndNameOrGroovySpecified=Only resource label, groovy expression, or resource names can be defined, not more than one. -error.couldNotParseToint=Could not parse the given value as integer. -error.givenAmountIsGreaterThatResurcesAmount=Given amount %d is greater than amount of resources: %d. +error.couldNotParseToInt=Could not parse the given value as integer. +error.givenAmountIsGreaterThanResourcesAmount=Given amount %d is greater than amount of resources: %d. error.resourceAlreadyLocked=Resource {0} already reserved or locked! error.invalidResourceSelectionStrategy=The strategy "{0}" is not supported. Valid options are {1}. # display-names @@ -36,4 +40,5 @@ LockStep.displayName=Lock shared resource LockStepResource.displayName=Resource LockableResource.displayName=Resource LockableResourcesManager.displayName=External Resources +LockedResourcesBuildAction.displayName=Used lockable resources RequiredResourcesProperty.displayName=Required Lockable Resources diff --git a/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java b/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java index 2207c3678..38df15aad 100644 --- a/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java +++ b/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java @@ -172,6 +172,38 @@ public void configRoundTripWithParam() throws Exception { assertNull(resourcesProp.getResourceMatchScript()); } + @Test + public void configRoundTripWithLabelParam() throws Exception { + FreeStyleProject withLabel = j.createFreeStyleProject("withLabelParam"); + withLabel.addProperty(new RequiredResourcesProperty(null, null, null, "${labelParam}", null)); + FreeStyleProject withLabelRoundTrip = j.configRoundtrip(withLabel); + + RequiredResourcesProperty withLabelProp = + withLabelRoundTrip.getProperty(RequiredResourcesProperty.class); + assertNotNull(withLabelProp); + assertNull(withLabelProp.getResourceNames()); + assertNull(withLabelProp.getResourceNamesVar()); + assertNull(withLabelProp.getResourceNumber()); + assertEquals("${labelParam}", withLabelProp.getLabelName()); + assertNull(withLabelProp.getResourceMatchScript()); + } + + @Test + public void configRoundTripWithNumParam() throws Exception { + FreeStyleProject withNum = j.createFreeStyleProject("withNumParam"); + withNum.addProperty(new RequiredResourcesProperty(null, null, "${resNum}", "some-resources", null)); + FreeStyleProject withNumRoundTrip = j.configRoundtrip(withNum); + + RequiredResourcesProperty withNumProp = + withNumRoundTrip.getProperty(RequiredResourcesProperty.class); + assertNotNull(withNumProp); + assertNull(withNumProp.getResourceNames()); + assertNull(withNumProp.getResourceNamesVar()); + assertEquals("${resNum}", withNumProp.getResourceNumber()); + assertEquals("some-resources", withNumProp.getLabelName()); + assertNull(withNumProp.getResourceMatchScript()); + } + @Test public void configRoundTripWithScript() throws Exception { FreeStyleProject withScript = j.createFreeStyleProject("withScript"); diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java index 3235bc9e0..d8a9bfb06 100644 --- a/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java +++ b/src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java @@ -1387,6 +1387,7 @@ public void lockWithInvalidLabel() throws Exception { p.setDefinition(new CpsFlowDefinition("lock(label: 'invalidLabel') {\n" + "}\n", true)); WorkflowRun b1 = p.scheduleBuild2(0).waitForStart(); j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b1)); + j.waitUntilNoActivity(); j.assertLogContains("The resource label does not exist: invalidLabel", b1); isPaused(b1, 0, 0); } diff --git a/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceManagerTest.java b/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceManagerTest.java index 073a72846..5ec845f53 100644 --- a/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceManagerTest.java +++ b/src/test/java/org/jenkins/plugins/lockableresources/LockableResourceManagerTest.java @@ -5,7 +5,10 @@ import static org.junit.Assert.assertThrows; import hudson.model.AutoCompletionCandidates; +import hudson.model.FreeStyleProject; import hudson.model.Item; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.StringParameterDefinition; import hudson.model.User; import hudson.security.AccessDeniedException3; import hudson.util.FormValidation; @@ -26,6 +29,12 @@ public class LockableResourceManagerTest { @Test public void validationFailure() throws Exception { + ParametersDefinitionProperty params = new ParametersDefinitionProperty( + new StringParameterDefinition("param1", "resource1", "parameter 1"), + new StringParameterDefinition("param2", "2", "parameter 2")); + FreeStyleProject p = j.createFreeStyleProject("p"); + p.addProperty(params); + RequiredResourcesProperty.DescriptorImpl d = new RequiredResourcesProperty.DescriptorImpl(); LockableResourcesManager.get().createResource("resource1"); LockableResource r = LockableResourcesManager.get().getFirst(); @@ -33,25 +42,69 @@ public void validationFailure() throws Exception { assertEquals( "Only resource label, groovy expression, or resource names can be defined, not more than one.", - d.doCheckResourceNames("resource1", null, true, null).getMessage()); + d.doCheckResourceNames("resource1", null, true, p).getMessage()); assertEquals( "Only resource label, groovy expression, or resource names can be defined, not more than one.", - d.doCheckResourceNames("resource1", "some-label", false, null).getMessage()); + d.doCheckResourceNames("resource1", "some-label", false, p).getMessage()); assertEquals( "Only resource label, groovy expression, or resource names can be defined, not more than one.", - d.doCheckResourceNames("resource1", "some-label", true, null).getMessage()); + d.doCheckResourceNames("resource1", "some-label", true, p).getMessage()); assertEquals( "Only resource label, groovy expression, or resource names can be defined, not more than one.", - d.doCheckLabelName("some-label", "resource1", false, null).getMessage()); + d.doCheckLabelName("some-label", "resource1", false, p).getMessage()); assertEquals( "Only resource label, groovy expression, or resource names can be defined, not more than one.", - d.doCheckLabelName("some-label", null, true, null).getMessage()); + d.doCheckLabelName("some-label", null, true, p).getMessage()); assertEquals( "Only resource label, groovy expression, or resource names can be defined, not more than one.", - d.doCheckLabelName("some-label", "resource1", true, null).getMessage()); + d.doCheckLabelName("some-label", "resource1", true, p).getMessage()); + + assertEquals(FormValidation.ok(), d.doCheckResourceNames("resource1", null, false, p)); + assertEquals(FormValidation.ok(), d.doCheckLabelName("some-label", null, false, p)); + assertEquals(FormValidation.ok(), d.doCheckResourceNumber("1", "resource1", null, null, p)); + + assertEquals( + "The resource does not exist: [resource3].", + d.doCheckResourceNames("${param5} resource3", null, false, p).getMessage()); + assertEquals( + "The parameter does not exist: [param5, param4].", + d.doCheckResourceNames("${param5} ${param4} resource1", null, false, p).getMessage()); + assertEquals( + "The resource label does not exist: other-label.", + d.doCheckLabelName("other-label", null, false, p).getMessage()); + + assertEquals( + "The resource cannot be validated as it contains parameters: [${param1}].", + d.doCheckResourceNames("${param1}", null, false, p).getMessage()); + assertEquals( + "The resource cannot be validated as it contains parameters: [xyz_${param1}].", + d.doCheckResourceNames("xyz_${param1}", null, false, p).getMessage()); + assertEquals( + "The resource label cannot be validated as it contains parameters: ${param1}.", + d.doCheckLabelName("${param1}", null, false, p).getMessage()); + assertEquals( + "The resource label cannot be validated as it contains parameters: ${param1}${param2}.", + d.doCheckLabelName("${param1}${param2}", null, false, p).getMessage()); + assertEquals( + "The resource label cannot be validated as it contains parameters: resource${param2}.", + d.doCheckLabelName("resource${param2}", null, false, p).getMessage()); + assertEquals( + "The value cannot be validated as it is a parameter value: ${param1}.", + d.doCheckResourceNumber("${param1}", null,null, null, p).getMessage()); + + assertEquals( + "Could not parse the given value as integer.", + d.doCheckResourceNumber("${param1}${param2}", null,null, null, p).getMessage()); + assertEquals( + "Could not parse the given value as integer.", + d.doCheckResourceNumber("Five", null,null, null, p).getMessage()); - assertEquals(FormValidation.ok(), d.doCheckResourceNames("resource1", null, false, null)); - assertEquals(FormValidation.ok(), d.doCheckLabelName("some-label", null, false, null)); + assertEquals( + "Given amount 4 is greater than amount of resources: 1.", + d.doCheckResourceNumber("4", "resource1", null,null, p).getMessage()); + assertEquals( + "Given amount 5 is greater than amount of resources: 1.", + d.doCheckResourceNumber("5", null, "some-label",null, p).getMessage()); j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy() @@ -65,8 +118,8 @@ public void validationFailure() throws Exception { .everywhere() .to("manager")); - j.buildAndAssertSuccess(j.createFreeStyleProject("aProject")); - Item item = j.jenkins.getItem("aProject"); + FreeStyleProject item = j.createFreeStyleProject("aProject"); + j.buildAndAssertSuccess(item); assertNotNull(item); User user = User.get("user", true, Collections.emptyMap());