Skip to content

Commit

Permalink
Improve UI validation and add more tests.
Browse files Browse the repository at this point in the history
  • Loading branch information
@cfoote
cfoote committed Jan 29, 2024
1 parent ef307a1 commit 91375a2
Show file tree
Hide file tree
Showing 6 changed files with 190 additions and 34 deletions.
59 changes: 40 additions & 19 deletions src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -173,24 +173,29 @@ public FormValidation doCheckResourceNames(
} else {
List<String> wrongNames = new ArrayList<>();
List<String> varNames = new ArrayList<>();
List<String> 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<String> 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));
}
}
}
Expand All @@ -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);
Expand All @@ -214,6 +219,12 @@ public FormValidation doCheckLabelName(
} else {
if (LockableResourcesManager.get().isValidLabel(label)) {
return FormValidation.ok();
} else if (Utils.containsParameter(label)) {
List<String> 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));
}
Expand All @@ -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<String> 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();
}
Expand Down
48 changes: 46 additions & 2 deletions src/main/java/org/jenkins/plugins/lockableresources/queue/Utils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}

Expand All @@ -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("(?<!\\$)\\$\\{([A-Za-z0-9_.]+)\\}");

@CheckForNull
public static Job<?, ?> getProject(@NonNull Queue.Item item) {
if (item.task instanceof Job) return (Job<?, ?>) item.task;
Expand All @@ -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());
Expand All @@ -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<String> 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<String> checkParameters(String s, AbstractProject<?, ?> project) {
List<String> unknownParameters = new ArrayList<>();
List<String> 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;
}
}
11 changes: 8 additions & 3 deletions src/main/resources/org/jenkins/plugins/lockableresources/Messages.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,25 @@ 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
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
32 changes: 32 additions & 0 deletions src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
1 change: 1 addition & 0 deletions src/test/java/org/jenkins/plugins/lockableresources/LockStepTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
73 changes: 63 additions & 10 deletions src/test/java/org/jenkins/plugins/lockableresources/LockableResourceManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -26,32 +29,82 @@ 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();
r.setLabels("some-label");

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()
Expand All @@ -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());
Expand Down

0 comments on commit 91375a2

Please sign in to comment.