Skip to content

Commit

Permalink
[JENKINS-72988] validate displayname only against items in same ItemG…
Browse files Browse the repository at this point in the history 
…roup

The check if a displayname collides with an existing job name or
displayname was always performing the check against the root. So if one
wanted to use a displayname for a job inside a folder the check would
wrongly fail if the name is used by a something at root level and not
fail when it is used by another item in the folder
  • Loading branch information
@mawinter69
mawinter69 committed Apr 9, 2024
1 parent 9d60a3b commit eb9afd1
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 17 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/model/AbstractProject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1942,7 +1942,7 @@ public boolean isApplicable(Descriptor descriptor) {

@Restricted(DoNotUse.class)
public FormValidation doCheckDisplayNameOrNull(@AncestorInPath AbstractProject project, @QueryParameter String value) {
return Jenkins.get().doCheckDisplayName(value, project.getName());
return Jenkins.get().doCheckDisplayName(value, project);
}

@Restricted(DoNotUse.class)
Expand Down
40 changes: 34 additions & 6 deletions core/src/main/java/jenkins/model/Jenkins.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5274,8 +5274,9 @@ public View getStaplerFallback() {
* job that the user is configuring though to prevent a validation warning
* if the user sets the displayName to what it currently is.
*/
boolean isDisplayNameUnique(String displayName, String currentJobName) {
Collection<TopLevelItem> itemCollection = items.values();
boolean isDisplayNameUnique(ItemGroup<?> itemGroup, String displayName, String currentJobName) {

Collection<TopLevelItem> itemCollection = itemGroup.getAllItems(TopLevelItem.class);

// if there are a lot of projects, we'll have to store their
// display names in a HashSet or something for a quick check
Expand All @@ -5299,8 +5300,8 @@ else if (displayName.equals(item.getDisplayName())) {
* @param name The name to test
* @param currentJobName The name of the job that the user is configuring
*/
boolean isNameUnique(String name, String currentJobName) {
Item item = getItem(name);
boolean isNameUnique(ItemGroup<?> itemGroup, String name, String currentJobName) {
Item item = itemGroup.getItem(name);

if (null == item) {
// the candidate name didn't return any items so the name is unique
Expand All @@ -5322,17 +5323,44 @@ else if (item.getName().equals(currentJobName)) {
* existing display names or project names
* @param displayName The display name to test
* @param jobName The name of the job the user is configuring
*
* @deprecated use {@link Jenkins#doCheckDisplayName(String, Item)}
*/
@Deprecated
public FormValidation doCheckDisplayName(@QueryParameter String displayName,
@QueryParameter String jobName) {
displayName = displayName.trim();

LOGGER.fine(() -> "Current job name is " + jobName);

if (!isNameUnique(displayName, jobName)) {
if (!isNameUnique(this, displayName, jobName)) {
return FormValidation.warning(Messages.Jenkins_CheckDisplayName_NameNotUniqueWarning(displayName));
}
else if (!isDisplayNameUnique(this, displayName, jobName)) {
return FormValidation.warning(Messages.Jenkins_CheckDisplayName_DisplayNameNotUniqueWarning(displayName));
}
else {
return FormValidation.ok();

Check warning on line 5343 in core/src/main/java/jenkins/model/Jenkins.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 5336-5343 are not covered by tests
}
}

/**
* Checks to see if the candidate displayName collides with any
* existing display names or project names in the items parent group
* @param displayName The display name to test
* @param item The item to check for duplicates
*/
public FormValidation doCheckDisplayName(String displayName,
Item item) {
displayName = displayName.trim();
String jobName = item.getName();

LOGGER.fine(() -> "Current job name is " + jobName);

if (!isNameUnique(item.getParent(), displayName, jobName)) {
return FormValidation.warning(Messages.Jenkins_CheckDisplayName_NameNotUniqueWarning(displayName));
}
else if (!isDisplayNameUnique(displayName, jobName)) {
else if (!isDisplayNameUnique(item.getParent(), displayName, jobName)) {
return FormValidation.warning(Messages.Jenkins_CheckDisplayName_DisplayNameNotUniqueWarning(displayName));
}
else {
Expand Down
20 changes: 10 additions & 10 deletions test/src/test/java/jenkins/model/JenkinsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ public void testIsDisplayNameUniqueTrue() throws Exception {
p.setDisplayName("displayName");

Jenkins jenkins = Jenkins.get();
assertTrue(jenkins.isDisplayNameUnique("displayName1", curJobName));
assertTrue(jenkins.isDisplayNameUnique(jobName, curJobName));
assertTrue(jenkins.isDisplayNameUnique(jenkins, "displayName1", curJobName));
assertTrue(jenkins.isDisplayNameUnique(jenkins, jobName, curJobName));
}

@Test
Expand All @@ -220,7 +220,7 @@ public void testIsDisplayNameUniqueFalse() throws Exception {
p.setDisplayName(displayName);

Jenkins jenkins = Jenkins.get();
assertFalse(jenkins.isDisplayNameUnique(displayName, curJobName));
assertFalse(jenkins.isDisplayNameUnique(jenkins, displayName, curJobName));
}

@Test
Expand All @@ -233,7 +233,7 @@ public void testIsDisplayNameUniqueSameAsCurrentJob() throws Exception {

Jenkins jenkins = Jenkins.get();
// should be true as we don't test against the current job
assertTrue(jenkins.isDisplayNameUnique(displayName, curJobName));
assertTrue(jenkins.isDisplayNameUnique(jenkins, displayName, curJobName));
}

@Test
Expand All @@ -244,7 +244,7 @@ public void testIsNameUniqueTrue() throws Exception {
j.createFreeStyleProject(jobName);

Jenkins jenkins = Jenkins.get();
assertTrue(jenkins.isNameUnique("jobName1", curJobName));
assertTrue(jenkins.isNameUnique(jenkins, "jobName1", curJobName));
}

@Test
Expand All @@ -255,7 +255,7 @@ public void testIsNameUniqueFalse() throws Exception {
j.createFreeStyleProject(jobName);

Jenkins jenkins = Jenkins.get();
assertFalse(jenkins.isNameUnique(jobName, curJobName));
assertFalse(jenkins.isNameUnique(jenkins, jobName, curJobName));
}

@Test
Expand All @@ -267,7 +267,7 @@ public void testIsNameUniqueSameAsCurrentJob() throws Exception {

Jenkins jenkins = Jenkins.get();
// true because we don't test against the current job
assertTrue(jenkins.isNameUnique(curJobName, curJobName));
assertTrue(jenkins.isNameUnique(jenkins, curJobName, curJobName));
}

@Test
Expand All @@ -281,7 +281,7 @@ public void testDoCheckDisplayNameUnique() throws Exception {
p.setDisplayName("displayName");

Jenkins jenkins = Jenkins.get();
FormValidation v = jenkins.doCheckDisplayName("1displayName", curJobName);
FormValidation v = jenkins.doCheckDisplayName("1displayName", curProject);
assertEquals(FormValidation.ok(), v);
}

Expand All @@ -297,7 +297,7 @@ public void testDoCheckDisplayNameSameAsDisplayName() throws Exception {
p.setDisplayName(displayName);

Jenkins jenkins = Jenkins.get();
FormValidation v = jenkins.doCheckDisplayName(displayName, curJobName);
FormValidation v = jenkins.doCheckDisplayName(displayName, curProject);
assertEquals(FormValidation.Kind.WARNING, v.kind);
}

Expand All @@ -313,7 +313,7 @@ public void testDoCheckDisplayNameSameAsJobName() throws Exception {
p.setDisplayName(displayName);

Jenkins jenkins = Jenkins.get();
FormValidation v = jenkins.doCheckDisplayName(jobName, curJobName);
FormValidation v = jenkins.doCheckDisplayName(jobName, curProject);
assertEquals(FormValidation.Kind.WARNING, v.kind);
}

Expand Down

0 comments on commit eb9afd1

Please sign in to comment.