RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]

[Mark Powers]

On Wed, 8 Jun 2022 15:53:06 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Mark Powers has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
>> 
>>  - Merge
>>  - fourth iteration
>>  - Merge
>>  - Merge
>>  - third iteration
>>  - Merge
>>  - Merge
>>  - second iteration
>>  - Merge
>>  - Merge
>>  - ... and 1 more: https://git.openjdk.org/jdk/compare/4d6fb515...44140a01
>
> src/java.base/share/classes/java/security/Provider.java line 1098:
> 
>> 1096:         boolean matches(String type, String algorithm) {
>> 1097:             return (Objects.equals(this.type, type)) &&
>> 1098:                 (Objects.equals(this.originalAlgorithm, algorithm));
> 
> In fact, I'm not sure why the original code is brave enough to use `==` instead of `equals`. If you do believe it's a real bug (and can write a regression test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a comment saying it's correct.

The `ServiceKey` class has `equals` and `matches` methods. The `matches` method returns true if two objects contain identical String pointers. The `equals` method returns true if String pointers are different but have the same value. This is by design, so it would be a mistake to take IntelliJ's suggestion here.

Keeping the `==` and adding a comment.
Good catch.

> src/java.base/share/classes/java/security/SecureClassLoader.java line 221:
> 
>> 219:         // only), and the fragment is not considered.
>> 220:         CodeSourceKey key = new CodeSourceKey(cs);
>> 221:         /* not used */
> 
> What is not used? `key1`? How about just rename it to `unused`?

Renamed `key1` to `unused`.

> src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
> 
>> 233:     }
>> 234: 
>> 235:     private record CodeSourceKey(CodeSource cs) {
> 
> Really necessary?

No. It was just an IntelliJ suggestion. I have no preference.

> src/java.base/share/classes/java/security/SecureRandom.java line 905:
> 
>> 903:         private static final Pattern pattern =
>> 904:             Pattern.compile(
>> 905:                 "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?");
> 
> Don't quite understand this change. Can you explain?

The author must have thought `:` and `,` were metacharacters that needed to be escaped to turn them into ordinary characters. I verified with a small java program that` \:` and` \,` are equivalent to `;` and `,`.
The IntelliJ complaint is "Redundant character escape `'\:'`".

> src/java.base/share/classes/java/security/UnresolvedPermission.java line 369:
> 
>> 367:             this.certs != null && that.certs == null ||
>> 368:             this.certs != null &&
>> 369:                this.certs.length != that.certs.length) {
> 
> I see you keep a lot of parentheses in other places. For example, `(!r)`.

This is an IntelliJ suggestion. I don't know why it complains about some cases and ignores others. Perhaps it has something to do with readability.

> src/java.base/share/classes/java/security/cert/CertPathValidator.java line 335:
> 
>> 333:         String cpvtype =
>> 334:             AccessController.doPrivileged((PrivilegedAction<String>) () ->
>> 335:                     Security.getProperty(CPV_TYPE));
> 
> See too many, maybe we need a dedicated method in `sun.security.util.SecurityProperties`?

A privilegedGetProperty method? That would be the subject of a new bug I think.

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

PR: https://git.openjdk.org/jdk/pull/8319