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

[Sean Mullan]

On Wed, 26 Jul 2023 09:28:46 GMT, John Jiang <jjiang at openjdk.org> wrote:

>> Some java/security classes apply the below coding style,
>> 
>> Set<T> set = ...;
>> Set<T> unmodifiableSet = Collections.unmodifiableSet(new HashSet<>(set));
>> 
>> It may be unnecessary to wrap that `set` with HashSet before creating `unmodifiableSet`.
>> Some usages on `Collections.unmodifiableList` and `Collections.unmodifiableMap` have the same issue.
>
> John Jiang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   use copyOf instead of modifiableXXX

src/java.base/share/classes/java/security/DomainLoadStoreParameter.java line 137:

> 135:         }
> 136:         this.configuration = configuration;
> 137:         this.protectionParams = Map.copyOf(protectionParams);

This change is no longer strictly compliant with the specification. On line 126, it says "It is cloned to prevent subsequent modification." Now, if the set passed in is unmodifiable, that is no longer true. 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.

So, this change would require specification changes and a CSR.

Can you provide more details on whether this is a real performance issue in practice? 

I would tend to avoid changes like this that affect the API specification unless there is a very strong compelling case where it improves performance for a real use case.

src/java.base/share/classes/java/security/KeyStore.java line 569:

> 567:             }
> 568: 
> 569:             this.attributes = Set.copyOf(attributes);

Similar comments and concerns as noted above in DomainLoadStoreParameter.

src/java.base/share/classes/java/security/KeyStore.java line 687:

> 685:             }
> 686:             this.sKey = secretKey;
> 687:             this.attributes = Set.copyOf(attributes);

Similar comments again.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275080672
PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275082327
PR Review Comment: https://git.openjdk.org/jdk/pull/15008#discussion_r1275082978