RFR: 8312630: java/security should not create unmodifiable collections with redundant wrapping [v2]

[Sean Mullan]

On Wed, 26 Jul 2023 16:03:31 GMT, John Jiang <jjiang at openjdk.org> wrote:

> > it says "It is cloned to prevent subsequent modification."
> > Now, if the set passed in is unmodifiable, that is no longer true.
> 
> My understanding: The parameter `protectionParams` is cloned as `this.protectionParams`, and `this.protectionParams` should be prevented from subsequent modification.

The latter part is true (prevented from subsequent modification) but, unless I am mistaken, the former (making a clone/copy) is not. For example, before your change, this assert would pass:


Map m = Collections.unmodifiableMap(map);
DomainLoadStoreParameters params = new DomainLoadStoreParameters(uri, m);
assert m != params.getProtectionParams();


After your change, I think it fails (can you check?). Even though the protection in both cases should be adequate, it is a subtle behavior change that I don't think the current specification covers.

> 
> `Map::copyOf` looks meet this requirement, no matter the parameter `protectionParams` is modifiable or not.
> 
> > It also will now throw NPE if any of the keys or values are null, whereas before it did not,
> > and the specification does not specify that in the @throws clause.
> 
> Yes, this is a problem. I don't want to change any behavior.

As mentioned, I don't think this change is critical unless you have a stronger case.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275244650