Allow all immutable List subclasses from Java 11

[mtughan]

A previous commit specifically allowed one of the two subclasses used by List.of and List.copyOf, but not the other, which can result in unexpected errors and bugs. Add the other to the default allow list of classes to avoid these.

Testing done

None.

Proposed changelog entries

• Allow all immutable List subclasses from Java 11 over remoting

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

1. The Jira issue, if it exists, is well-described.
2. The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
6. If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[welcome]

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

[timja]

seems ok, do you have a specific need for them though?

[timja]

@jglick / @dwnusbaum any opinion?

[mtughan]

seems ok, do you have a specific need for them though?

I used it recently in the Scriptler plugin, which is how I came across this. By only having List12 in there, it only permits the usage of List.of or List.copyOf where exactly 1 or 2 elements are present. Calls with 0 elements or 3 or more will result in using ListN, which wasn't permitted before (hence the need for the class filter file in that commit).

And then while I was here, I also thought that adding the Set and Map versions of the built-in immutable collections would be good. I don't specifically use them right now, but could potentially in the future. There should be no safety issues when using these as they are specifically immutable collections and cannot change; from that point of view, they should be equivalent to Guava's ImmutableList, ImmutableMap, and ImmutableSet, all 3 of which are already allowed in this file.

[timja]

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

[jglick]

-0 I guess. It is not a security threat, but neither is it necessary: jenkinsci/scriptler-plugin#134 (review)

Most of the classes listed in the whitelist were there not because they should be serialized but because they were being serialized by some plugin at the time https://jenkins.io/jep/200 was developed, and it was simpler to allow the class temporarily than to wait for a plugin release removing the usage. New code should only use simple types like ArrayList. Always look at your actual config.xml serial form.

[mtughan]

New code should only use simple types like ArrayList. Always look at your actual config.xml serial form.

Why should something like ArrayList be preferred over a JDK built-in immutable collection? Immutable collections are very handy for ensuring that objects don't unexpectedly change and can be used in operations that require hashing, such as sets and maps. I can certainly understand why we would prefer JDK classes over 3rd-party libraries such as Guava, but this is also JDK 9+. I suppose I could call code like Collections.unmodifiableList(new ArrayList<>(parameters)) to accomplish something similar, but that looks much more complicated than List.copyOf(parameters).

In terms of the serialized forms, that's currently fair: using an ArrayList results in

<parameters>
  <org.jenkinsci.plugins.scriptler.config.Parameter>
    <name>test</name>
    <value>value1</value>
  </org.jenkinsci.plugins.scriptler.config.Parameter>
</parameters>

while using List.copyOf currently results in

<parameters class="java.util.ImmutableCollections$List12" resolves-to="java.util.CollSer" serialization="custom">
  <java.util.CollSer>
    <default>
      <tag>1</tag>
    </default>
    <int>1</int>
    <org.jenkinsci.plugins.scriptler.config.Parameter>
      <name>test</name>
      <value>value</value>
    </org.jenkinsci.plugins.scriptler.config.Parameter>
  </java.util.CollSer>
</parameters>

which appears to be because it implements custom serialization methods. But I would argue that the second form should be improved (if possible) rather than always preferring the first form.

[jglick]

the second form should be improved (if possible)

Core could get additional custom XStream converters if necessary. But it is not necessary: the field in an XStream-serialized class should always use the plainest collection possible types—generally ArrayList or PersistedList or DescribableList. (In the case of a Describable, Set or Map should not generally be used, since core form controls do not really support these types of collections—only a List<T> where T is another nested Describable. The only other recommended field types would be non-collections: String, boolean, int, or long, and maybe enum though I am not sure how well that works.)

A @DataBoundConstructor, @DataBoundSetter, getter matching one of those two, or any other method used programmatically (not called from databinding) may of course include logic to make defensive copies, make collections immutable, add synchronization, etc.

[mtughan]

I think I see why ArrayList should be preferred now: XStream has built in support for ArrayList specifically (along with HashSet, LinkedHashSet, LinkedList, and Vector) through the CollectionConverter class. So from that standpoint, it makes sense to not add the additional Set and Map implementations here.

I would still argue that we should probably add ListN in addition to List12 here. With List12 being allowed, people may get the idea that List.of or List.copyOf are supported, and it will work in some circumstances (i.e., with 1 or 2 elements being used). However, once a scenario where 0 or 3+ elements are used, then it may unexpectedly break and may be a source of bugs. I recognize that you (@jglick) had already commented on the original change at #6700 (comment), but with that code being part of Jenkins 2.358 and later, I feel it would be better to finish the job and add both.

[mtughan]

I've pushed a new commit and updated the PR description to reflect that we're only allowing immutable List classes now, not all immutable collections.

As a rule, nothing should ever be added to this whitelist ever again. The damage has already been partially done in this case so this PR is more a matter of completing the mistake. No plugin code should be written which relies on it.

[mtughan]

@timja and @jglick, is there anything else preventing this PR from progressing? Or can we have it merged?