Better @DataBoundConstructor hygiene for ParameterDefinition’s (#5275)

* Better @DataBoundConstructor hygiene for ParameterDefinition’s * @SInCE TODO not @SInCE XXX * Fix JavaDoc generation * Treat empty ParameterDefinition.description & StringParameterDefinition.defaultValue like null, for better /directive-generator/ behavior * ChoiceParameterDefinitionTest reminds me to ensure that choices contains no null elements Co-authored-by: Tim Jacomb <timjacomb1+github@gmail.com>
jenkinsci · Feb 21, 2021 · 8152129 · 8152129
1 parent 38ce5ce
commit 8152129
Show file tree

Hide file tree

Showing 10 changed files with 105 additions and 25 deletions.
diff --git a/core/src/main/java/hudson/model/BooleanParameterDefinition.java b/core/src/main/java/hudson/model/BooleanParameterDefinition.java
@@ -30,19 +30,28 @@
 import hudson.Extension;
 
 import java.util.Objects;
+import org.kohsuke.stapler.DataBoundSetter;
 
 /**
  * {@link ParameterDefinition} that is either 'true' or 'false'.
  *
  * @author huybrechts
  */
 public class BooleanParameterDefinition extends SimpleParameterDefinition {
-    private final boolean defaultValue;
+    private boolean defaultValue;
 
+    /**
+     * @since TODO
+     */
     @DataBoundConstructor
+    public BooleanParameterDefinition(String name) {
+        super(name);
+    }
+
     public BooleanParameterDefinition(String name, boolean defaultValue, String description) {
-        super(name, description);
-        this.defaultValue = defaultValue;
+        this(name);
+        setDefaultValue(defaultValue);
+        setDescription(description);
     }
 
     @Override
@@ -59,6 +68,14 @@ public boolean isDefaultValue() {
         return defaultValue;
     }
 
+    /**
+     * @since TODO
+     */
+    @DataBoundSetter
+    public void setDefaultValue(boolean defaultValue) {
+        this.defaultValue = defaultValue;
+    }
+
     @Override
     public ParameterValue createValue(StaplerRequest req, JSONObject jo) {
         BooleanParameterValue value = req.bindJSON(BooleanParameterValue.class, jo);

diff --git a/core/src/main/java/hudson/model/ChoiceParameterDefinition.java b/core/src/main/java/hudson/model/ChoiceParameterDefinition.java
@@ -1,5 +1,6 @@
 package hudson.model;
 
+import hudson.Util;
 import hudson.util.FormValidation;
 import org.jenkinsci.Symbol;
 import org.kohsuke.accmod.Restricted;
@@ -20,6 +21,8 @@
 import java.util.List;
 import java.util.Arrays;
 import java.util.Objects;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * @author huybrechts
@@ -47,7 +50,7 @@ public ChoiceParameterDefinition(@NonNull String name, @NonNull String choices,
 
     public ChoiceParameterDefinition(@NonNull String name, @NonNull String[] choices, String description) {
         super(name, description);
-        this.choices = new ArrayList<>(Arrays.asList(choices));
+        this.choices = Stream.of(choices).map(Util::fixNull).collect(Collectors.toCollection(ArrayList::new));
         defaultValue = null;
     }
 
@@ -57,6 +60,7 @@ private ChoiceParameterDefinition(@NonNull String name, @NonNull List<String> ch
         this.defaultValue = defaultValue;
     }
 
+    // TODO consider switching @DataBoundConstructor to a ChoiceParameterDefinition(String) overload
     /**
      * Databound constructor for reflective instantiation.
      *

diff --git a/core/src/main/java/hudson/model/FileParameterDefinition.java b/core/src/main/java/hudson/model/FileParameterDefinition.java
@@ -45,9 +45,18 @@
  * @author Kohsuke Kawaguchi
  */
 public class FileParameterDefinition extends ParameterDefinition {
+
+    /**
+     * @since TODO
+     */
     @DataBoundConstructor
+    public FileParameterDefinition(String name) {
+        super(name);
+    }
+
     public FileParameterDefinition(String name, String description) {
-        super(name, description);
+        this(name);
+        setDescription(description);
     }
 
     public FileParameterValue createValue(StaplerRequest req, JSONObject jo) {

diff --git a/core/src/main/java/hudson/model/ParameterDefinition.java b/core/src/main/java/hudson/model/ParameterDefinition.java
@@ -36,9 +36,11 @@
 import java.util.logging.Logger;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.Util;
 
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
+import org.kohsuke.stapler.DataBoundSetter;
 
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.export.Exported;
@@ -93,8 +95,6 @@
  * is then fed to {@link ParameterDefinition#createValue(StaplerRequest, JSONObject)} to
  * create {@link ParameterValue}s.
  *
- * TODO: what Jelly pages does this object need for rendering UI?
- * TODO: {@link ParameterValue} needs to have some mechanism to expose values to the build
  * @see StringParameterDefinition
  */
 @ExportedBean(defaultVisibility=3)
@@ -103,19 +103,22 @@ public abstract class ParameterDefinition implements
 
     private final String name;
 
-    private final String description;
+    private String description;
 
     public ParameterDefinition(@NonNull String name) {
-        this(name, null);
-    }
-
-    public ParameterDefinition(@NonNull String name, String description) {
-        //Checking as pipeline does not enforce annotations
         if (name == null) {
             throw new IllegalArgumentException("Parameter name must be non-null");
         }
         this.name = name;
-        this.description = description;
+    }
+
+    /**
+     * @deprecated Prefer {@link #ParameterDefinition(String)} with a {@link org.kohsuke.stapler.DataBoundConstructor} and allow {@link #setDescription} to be used as needed
+     */
+    @Deprecated
+    public ParameterDefinition(@NonNull String name, String description) {
+        this(name);
+        setDescription(description);
     }
 
     /**
@@ -146,14 +149,22 @@ public String getDescription() {
         return description;
     }
 
+    /**
+     * @since TODO
+     */
+    @DataBoundSetter
+    public void setDescription(@CheckForNull String description) {
+        this.description = Util.fixEmpty(description);
+    }
+
     /**
      * return parameter description, applying the configured MarkupFormatter for jenkins instance.
      * @since 1.521
      */
     @CheckForNull
     public String getFormattedDescription() {
         try {
-            return Jenkins.get().getMarkupFormatter().translate(description);
+            return Jenkins.get().getMarkupFormatter().translate(getDescription());
         } catch (IOException e) {
             LOGGER.warning("failed to translate description using configured markup formatter");
             return "";

diff --git a/core/src/main/java/hudson/model/PasswordParameterDefinition.java b/core/src/main/java/hudson/model/PasswordParameterDefinition.java
@@ -54,6 +54,7 @@ public PasswordParameterDefinition(String name, String defaultValue, String desc
         this.defaultValue = Secret.fromString(defaultValue);
     }
 
+    // TODO consider switching @DataBoundConstructor to a PasswordParameterDefinition(String) overload
     @DataBoundConstructor
     public PasswordParameterDefinition(String name, Secret defaultValueAsSecret, String description) {
         super(name, description);

diff --git a/core/src/main/java/hudson/model/RunParameterDefinition.java b/core/src/main/java/hudson/model/RunParameterDefinition.java
@@ -63,6 +63,7 @@ public String getName() {
     private final String runId;
     private final RunParameterFilter filter;
 
+    // TODO consider a simplified @DataBoundConstructor using @DataBoundSetter for description & filter
     /**
      * @since 1.517
      */

diff --git a/core/src/main/java/hudson/model/SimpleParameterDefinition.java b/core/src/main/java/hudson/model/SimpleParameterDefinition.java
@@ -4,6 +4,7 @@
 import hudson.cli.CLICommand;
 
 import java.io.IOException;
+import org.kohsuke.stapler.DataBoundConstructor;
 
 /**
  * Convenient base class for {@link ParameterDefinition} whose value can be represented in a context-independent single string token.
@@ -15,6 +16,10 @@ protected SimpleParameterDefinition(String name) {
         super(name);
     }
 
+    /**
+     * @deprecated Prefer {@link #SimpleParameterDefinition(String)} with a {@link DataBoundConstructor} and allow {@link #setDescription} to be used as needed
+     */
+    @Deprecated
     protected SimpleParameterDefinition(String name, String description) {
         super(name, description);
     }

diff --git a/core/src/main/java/hudson/model/StringParameterDefinition.java b/core/src/main/java/hudson/model/StringParameterDefinition.java
@@ -34,28 +34,40 @@
 import org.kohsuke.stapler.StaplerRequest;
 
 import java.util.Objects;
+import org.kohsuke.stapler.DataBoundSetter;
 
 /**
  * Parameter whose value is a string value.
  */
 public class StringParameterDefinition extends SimpleParameterDefinition {
 
     private String defaultValue;
-    private final boolean trim;
+    private boolean trim;
 
+    /**
+     * @since TODO
+     */
     @DataBoundConstructor
+    public StringParameterDefinition(String name) {
+        super(name);
+    }
+
     public StringParameterDefinition(String name, String defaultValue, String description, boolean trim) {
-        super(name, description);
-        this.defaultValue = defaultValue;
-        this.trim = trim;
+        this(name);
+        setDefaultValue(defaultValue);
+        setDescription(description);
+        setTrim(trim);
     }
 
     public StringParameterDefinition(String name, String defaultValue, String description) {
-        this(name, defaultValue, description, false);
+        this(name);
+        setDefaultValue(defaultValue);
+        setDescription(description);
     }
 
     public StringParameterDefinition(String name, String defaultValue) {
-        this(name, defaultValue, null, false);
+        this(name);
+        setDefaultValue(defaultValue);
     }
 
     @Override
@@ -83,9 +95,10 @@ public String getDefaultValue4Build() {
         }
         return defaultValue;
     }
-
+
+    @DataBoundSetter
     public void setDefaultValue(String defaultValue) {
-        this.defaultValue = defaultValue;
+        this.defaultValue = Util.fixEmpty(defaultValue);
     }
 
     /**
@@ -98,6 +111,14 @@ public void setDefaultValue(String defaultValue) {
     public boolean isTrim() {
         return trim;
     }
+
+    /**
+     * @since TODO
+     */
+    @DataBoundSetter
+    public void setTrim(boolean trim) {
+        this.trim = trim;
+    }
 
     @Override
     public StringParameterValue getDefaultParameterValue() {

diff --git a/core/src/main/java/hudson/model/StringParameterValue.java b/core/src/main/java/hudson/model/StringParameterValue.java
@@ -24,6 +24,7 @@
 package hudson.model;
 
 import hudson.EnvVars;
+import hudson.Util;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.export.Exported;
 
@@ -48,7 +49,7 @@ public StringParameterValue(String name, String value) {
 
     public StringParameterValue(String name, String value, String description) {
         super(name, description);
-        this.value = value;
+        this.value = Util.fixNull(value);
     }
 
     /**

diff --git a/core/src/main/java/hudson/model/TextParameterDefinition.java b/core/src/main/java/hudson/model/TextParameterDefinition.java
@@ -35,9 +35,19 @@
  * {@link StringParameterDefinition} that uses textarea, instead of text box.
  */
 public class TextParameterDefinition extends StringParameterDefinition {
+
+    /**
+     * @since TODO
+     */
     @DataBoundConstructor
+    public TextParameterDefinition(String name) {
+        super(name);
+    }
+
     public TextParameterDefinition(String name, String defaultValue, String description) {
-        super(name, defaultValue, description);
+        this(name);
+        setDefaultValue(defaultValue);
+        setDescription(description);
     }
 
     @Extension @Symbol({"text","textParam"})