From 2431b7c9b328b0bc883cfb9f9a0c52b6e9cfb79c Mon Sep 17 00:00:00 2001 From: Markus Winter Date: Mon, 9 Sep 2024 17:13:20 +0200 Subject: [PATCH] [JENKINS-72988] validate displayname against items in the same ItemGroup (#9152) Co-authored-by: Kris Stern --- .../hudson/model/TopLevelItemDescriptor.java | 2 +- core/src/main/java/jenkins/model/Jenkins.java | 41 ++++++++++++++++--- .../test/java/jenkins/model/JenkinsTest.java | 20 ++++----- 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/hudson/model/TopLevelItemDescriptor.java b/core/src/main/java/hudson/model/TopLevelItemDescriptor.java index 817c1194a3d5..b6177005c2d2 100644 --- a/core/src/main/java/hudson/model/TopLevelItemDescriptor.java +++ b/core/src/main/java/hudson/model/TopLevelItemDescriptor.java @@ -293,6 +293,6 @@ public static ExtensionList all() { @Restricted(NoExternalUse.class) public FormValidation doCheckDisplayNameOrNull(@AncestorInPath TopLevelItem item, @QueryParameter String value) { - return Jenkins.get().doCheckDisplayName(value, item.getName()); + return Jenkins.get().checkDisplayName(value, item); } } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 6b3500e6b66a..3c756bbb7ba0 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -5372,8 +5372,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 itemCollection = items.values(); + boolean isDisplayNameUnique(ItemGroup itemGroup, String displayName, String currentJobName) { + + Collection itemCollection = (Collection) itemGroup.getItems(t -> t instanceof TopLevelItem); // if there are a lot of projects, we'll have to store their // display names in a HashSet or something for a quick check @@ -5397,8 +5398,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 @@ -5420,17 +5421,45 @@ 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 TopLevelItemDescriptor#doCheckDisplayNameOrNull(TopLevelItem, String)} */ + @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(); + } + } + + /** + * 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 + */ + @Restricted(NoExternalUse.class) + public FormValidation checkDisplayName(String displayName, + TopLevelItem 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 { diff --git a/test/src/test/java/jenkins/model/JenkinsTest.java b/test/src/test/java/jenkins/model/JenkinsTest.java index a8d846c5d79b..c44b12cdd037 100644 --- a/test/src/test/java/jenkins/model/JenkinsTest.java +++ b/test/src/test/java/jenkins/model/JenkinsTest.java @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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 @@ -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.checkDisplayName("1displayName", curProject); assertEquals(FormValidation.ok(), v); } @@ -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.checkDisplayName(displayName, curProject); assertEquals(FormValidation.Kind.WARNING, v.kind); } @@ -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.checkDisplayName(jobName, curProject); assertEquals(FormValidation.Kind.WARNING, v.kind); }