RFR: 8369995: Implement extra logging and/or propagate errors in X509KeyManagerImpl and fix StringIndexOutOfBoundsException [v9]

[Mikhail Yankelevich]

On Thu, 23 Oct 2025 20:06:30 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:

>> Mikhail Yankelevich has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   line length changed
>
> test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 103:
> 
>> 101:         final X509KeyManager km = generateNullKm();
>> 102: 
>> 103:         km.getServerAliases(null, null);
> 
> I know that this section is not part of this change, but shouldn't we be testing that all these are indeed returning null?  
> 
> It's done in spots later on, but I feel it should be done here also to test all of the APIs.

I don't think this test focuses on this, but in any case it's easy to add a null verification, so I think it would be better to have it. Done in the next commit

> test/jdk/sun/security/ssl/X509KeyManager/NullCases.java line 203:
> 
>> 201:                         "Should return null if the alias is invalid <%s>",
>> 202:                         alias));
>> 203:     }
> 
> Based on the description of the bug, I was expecting to see checking of the debug output (`Implement extra logging...`), but appears that this is not the primary focus.  Maybe change the ordering of the bug description.  (`...and also implement extra logging`)

I have removed log verification from the tests. I agree, changing the title might be a good idea

> test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java line 70:
> 
>> 68: 
>> 69:         exceptionThrowingKMF
>> 70:                 .init((KeyStore) exceptionThrowingKS, null);
> 
> This cast seems unnecessary.
> 
> (Test runs without it.)

Yes, I've removed it in the next commit, thanks.

> test/jdk/sun/security/ssl/X509KeyManager/X509KeyManagerNegativeTests.java line 83:
> 
>> 81:     @Test
>> 82:     public void ksExceptionTest() {
>> 83:         // recording logs to the output stream
> 
> I don't understand this comment.  There doesn't seem to be any recording to the debug log stream in this test.

That's a typo, I have modified a test which used to verify logs. Removed in the next commit

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459620637
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459632554
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459627208
PR Review Comment: https://git.openjdk.org/jdk/pull/27851#discussion_r2459636148