JENKINS-70066: Fix null check of parameters #134
Conversation
The `@DataBoundConstructor` for `ScriptlerBuilder` takes a list of parameters for the given script. It was annotated as `@NotNull`, but `Descriptor.bindJSON` could occasionally pass `null` for the parameters list. Fix this by annotating the parameter as `@CheckForNull` and performing the appropriate null checks.
mtughan
force-pushed
the
parameters-null-check
branch
from
de21387 to
440b59b
Compare
There was a problem hiding this comment.
This is only meant to be temporary until jenkinsci/jenkins#10026 is merged, as per the JEP-200 blog post:
- If the class(es) are defined in a third-party library bundled in your plugin, create a resource file
META-INF/hudson.remoting.ClassFilterlisting them. (example)
- You may also do this for Java or Jenkins core library classes, as a hotfix until your core baseline includes the whitelist entry proposed above.
| this.builderId = builderId; | ||
| this.scriptId = scriptId; | ||
| this.parameters = new ArrayList<>(parameters); | ||
| this.parameters = parameters == null ? List.of() : List.copyOf(parameters); |
There was a problem hiding this comment.
Do not do this. Rather create a new ArrayList for the internal field used in the serial form.
| @@ -175,7 +180,7 @@ public Parameter[] getParameters() { | |||
|
|
|||
| @NonNull | |||
| public List<Parameter> getParametersList() { | |||
| return Collections.unmodifiableList(parameters); | |||
| return parameters; | |||
There was a problem hiding this comment.
Whether you should revert this or not would depend on whether or not you want callers to modify the list. Probably you do not, so this should be reverted.
There was a problem hiding this comment.
This is currently safe as returning an unmodifiable view of an immutable list is a no-op.
There was a problem hiding this comment.
Let's finish the discussion in jenkinsci/jenkins#10026 first before deciding the next steps here.
| @@ -175,7 +180,7 @@ public Parameter[] getParameters() { | |||
|
|
|||
| @NonNull | |||
| public List<Parameter> getParametersList() { | |||
| return Collections.unmodifiableList(parameters); | |||
| return parameters; | |||
There was a problem hiding this comment.
This is currently safe as returning an unmodifiable view of an immutable list is a no-op.
There was a problem hiding this comment.
This is only meant to be temporary until jenkinsci/jenkins#10026 is merged, as per the JEP-200 blog post:
- If the class(es) are defined in a third-party library bundled in your plugin, create a resource file
META-INF/hudson.remoting.ClassFilterlisting them. (example)
- You may also do this for Java or Jenkins core library classes, as a hotfix until your core baseline includes the whitelist entry proposed above.
is meant for plugins historically using some unusual class in XStream serialized forms, where that usage cannot readily be replaced with a simple form without significant compatibility code. |
I was more pointing out the second part,
I created this file as a hotfix until the core includes support for this class. |
|
I think you misunderstood. That was for the period of time when JEP-200 first shipped, and for existing plugin code. |
Ah, gotcha. Thanks for the clarification. |
The
@DataBoundConstructorforScriptlerBuildertakes a list of parameters for the given script. It was annotated as@NotNull, butDescriptor.bindJSONcould occasionally passnullfor the parameters list. Fix this by annotating the parameter as@CheckForNulland performing the appropriate null checks.Testing done
Static analysis only.
Submitter checklist