On Tue, 13 May 2025 21:45:32 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:
>> Hi all, >> >> I need a code review of the PEM API. Privacy-Enhanced Mail (PEM) is a >> format for encoding and decoding cryptographic keys and certificates. It >> will be integrated into JDK24 as a Preview Feature. Preview features does >> not permanently define the API and it is subject to change in future >> releases until it is finalized. >> >> Details about this change can be seen at [PEM API >> JEP](https://bugs.openjdk.org/browse/JDK-8300911). >> >> Thanks >> >> Tony > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > comments Quickly going through public APIs. src/java.base/share/classes/java/security/PEMDecoder.java line 61: > 59: * </pre> > 60: * > 61: * <p> If the PEM does not have a JCE object representation, it returns a Not "it", but the two methods. src/java.base/share/classes/java/security/PEMDecoder.java line 69: > 67: * methods are useful when extracting or changing the return class. > 68: * The Class parameter can specify the returned > 69: * key object from a PEM containing a public and private key. If only Not always "key". If this is meant to be an example, prepend with "For example". src/java.base/share/classes/java/security/PEMDecoder.java line 89: > 87: * decoder not configured for decryption, an {@link > EncryptedPrivateKeyInfo} > 88: * object is returned. {@code EncryptedPrivateKeyInfo} methods must be > used to > 89: * retrieve the {@link PrivateKey}. Do we need this last sentence? "EncryptedPrivateKeyInfo methods must be used to...". src/java.base/share/classes/java/security/PEMDecoder.java line 97: > 95: * PEMDecoder pd = PEMDecoder.of(); > 96: * PrivateKey priKey = pd.decode(priKeyPEM, PrivateKey.class); > 97: * } Add examples on `withFactory` and `withDecryption`. src/java.base/share/classes/java/security/PEMDecoder.java line 133: > 131: * Returns an instance of {@code PEMDecoder}. > 132: * > 133: * @return a new {@code PEMDecoder} instance Not always new. src/java.base/share/classes/java/security/PEMDecoder.java line 234: > 232: * {@code leadingData}. > 233: * > 234: * <p>If no PEM data is found, an {@code IllegalArgumentException} is Duplicated sentence. Or do you mean to say the same exception is thrown if decoding fails? src/java.base/share/classes/java/security/PEMDecoder.java line 281: > 279: * @param is InputStream containing PEM data > 280: * @return a {@code DEREncodable} > 281: * @throws IOException on IO error with the {@code InputStream} Since IOE is not purely IO error and contains format errors that are not recoverable. Do you want to say more? src/java.base/share/classes/java/security/PEMDecoder.java line 283: > 281: * @throws IOException on IO error with the {@code InputStream} > 282: * @throws EOFException the end of {@code InputStream} has been > 283: * unexpectedly reached. Not really "unexpected". It looks like every program that tries to read multiple PEM blocks will encounter this. src/java.base/share/classes/java/security/PEMDecoder.java line 285: > 283: * unexpectedly reached. > 284: * @throws IllegalArgumentException on error in decoding or no PEM > data > 285: * found "no PEM data found". Isn't that EOFException? src/java.base/share/classes/java/security/PEMDecoder.java line 312: > 310: * {@link java.nio.charset.StandardCharsets#UTF_8 UTF-8}. > 311: * > 312: * <p> For all other class parameters, {@code > IllegalArgumentException} is "For all other class parameters", you mean not `PEMRecord`? There is a paragraph in between (UTF-8 thingy) and users have forgotten what "other" means. src/java.base/share/classes/java/security/PEMDecoder.java line 325: > 323: * @throws NullPointerException when any input values are null. > 324: * > 325: * @see PEMDecoder for how {@code tClass} can be used. Since it's a `@see`, let's make the title a little more formal. Maybe add a header (`<h2>`) in the spec. src/java.base/share/classes/java/security/PEMEncoder.java line 127: > 125: > 126: /** > 127: * Returns a new instance of {@code PEMEncoder}. Not always new. ------------- PR Review: https://git.openjdk.org/jdk/pull/17543#pullrequestreview-2838231399 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087706506 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087707481 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087708948 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087709526 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087709760 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087711953 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087714476 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087713003 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087713801 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087716326 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087717780 PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2087702103
