On Fri, 9 May 2025 18:06:10 GMT, Weijun Wang <[email protected]> wrote:
>> Anthony Scarpino has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - comments
>> - toString update
>> - non-sealed
>> Better X509 KeyPair parsing
>
> src/java.base/share/classes/java/security/PEMDecoder.java line 66:
>
>> 64: * For decode methods that accept a {@code Class<S> tClass} as input,
>> they can
>> 65: * modify the return type to a specific {@code DEREncodable} subclass.
>> 66: * For example, {@code ECPublicKey.class} can be used to cast a
>
> The "modify" and "cast" words seem to reveal some implementation detail. It's
> just "parsing the input as the specified type".
If the cast is wrong, it throws a CastClassException. I don't see why we need
to hide this from the user.
> src/java.base/share/classes/java/security/PEMDecoder.java line 146:
>
>> 144:
>> 145: try {
>> 146: return switch (pem.type()) {
>
> Do you still allow `type` being null?
>
> `PEMDecoder.of().decode("s")` throws an NPE.
Hmm... I'm surprised it wouldn't trigger the default case or allow a case `null`
> src/java.base/share/classes/java/security/PEMDecoder.java line 407:
>
>> 405: * the default provider configuration.
>> 406: *
>> 407: * @param provider the factory provider.
>
> This is a little awkward because the argument of `withFactory` is a provider.
> Shall we rename it?
>
> Also, can we add some more description on how this method is used? For
> example, suppose a provider named `P1` extends `ECPublicKey` to
> `P1ECPublicKey`, then users should call `withFactory(p1).decode(pem,
> P1ECPublicKey.class)`. I assume we are not ready to do some kind of "delayed
> provider selection" trick to make it possible with the "original" decoder.
I think the javadoc explains the name clearly with links to what these
factories do.
The class javadoc has details about using the decode class parameter and I
don't think the javadoc should explaining how to use a 3rd party provider with
3rd party classes.
I would wish this API to never use a delayed provider solution.
> src/java.base/share/classes/java/security/PEMRecord.java line 69:
>
>> 67: * @see PEMEncoder
>> 68: */
>> 69: public record PEMRecord(String type, String pem, byte[] leadingData)
>
> Add `@PreviewFeature`.
yep
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083274944
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083401700
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083389219
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2083279739
