RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss [v4]

[Mark Powers]

On Tue, 3 May 2022 01:19:20 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:
>> 
>>  - sixth iteration
>>  - Merge
>>  - fifth iteration
>>  - Merge
>>  - fourth iteration
>>  - third iteration
>>  - Merge
>>  - Merge
>>  - Merge
>>  - second iteration
>>  - ... and 1 more: https://git.openjdk.java.net/jdk/compare/64225e19...5ce46ffb
>
> src/java.security.jgss/share/classes/org/ietf/jgss/GSSException.java line 333:
> 
>> 331:     public String getMajorString() {
>> 332: 
>> 333:         return Objects.requireNonNullElseGet(majorString, () -> messages[major - 1]);
> 
> Isn't this too fancy? Inside JDK this method is only used once in `java.time`. The method name has a `requireNonNull` inside so my understanding is that the object is mostly not null and the else case is just a fallback (maybe in an exceptional case?) At least in this class, most `GSSExceptions` are created without a major string.

It's been around since JDK 9. I like the functionality, but I hate the name. It returns the first argument if non-null, otherwise it runs the lambda expression. I'll keep the change if you don't mind - one less thing for IntelliJ to nag about.

Thanks for taking time to review this!
I've accepted two of your suggestions and will be pushing an update shortly.

> src/java.security.jgss/share/classes/sun/security/jgss/GSSHeader.java line 267:
> 
>> 265:                 throw new IOException("DerInputStream.getLength(): lengthTag="
>> 266:                                       + tmp + ", "
>> 267:                                       + "too big.");
> 
> Combine the 2 lines above.

Fixed.

> src/java.security.jgss/share/classes/sun/security/jgss/krb5/SubjectComber.java line 154:
> 
>> 152:                     while (iterator.hasNext()) {
>> 153:                         Object obj = iterator.next();
>> 154:                         if (!(obj instanceof @SuppressWarnings("unchecked")KerberosTicket ticket)) {
> 
> Does not look fluent to me. Maybe add a new line after the annotation?

Added a new line.

> src/java.security.jgss/share/classes/sun/security/jgss/spnego/SpNegoContext.java line 876:
> 
>> 874: 
>> 875:         // pass token
>> 876:         tok = Objects.requireNonNullElseGet(token, () -> new byte[0]);
> 
> Is this how `requireNonNullElseGet` is meant to be used?

I think so - return the token, or if the token is null, return an empty byte array.

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

PR: https://git.openjdk.java.net/jdk/pull/7746