On Wed, 26 Apr 2023 20:56:38 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Every other crypto engine class throws IKE when initing with a null key, >> including `Signature`, `Cipher`, `KeyAgreement`, `Mac`. So I follow the same >> style. > > Hmm, while I understand your thinking, this seems like the wrong pattern to > follow though (ex: see Effective Java, Item 70), and it doesn't seem like > something we should have to adhere to for API consistency. There are various > bugs filed over the years on confusion over this style (search for "Cipher > init NullPointerException"), where providers threw NPE or users thought the > APIs should. Actually, there is still an open bug: > https://bugs.openjdk.org/browse/JDK-4955097 I checked for our implementations, and until now all I found is IKE. Even the PBEWithMD5AndDES algorithm mentioned in https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE. I'd rather follow the existing style. If it’s not clear enough, I can update the line to @throws InvalidKeyException if {@code publicKey} is {@code null} or invalid If we are worried that a security provider might throw an NPE, how about we add a check on the user API side: public Encapsulator newEncapsulator(PublicKey publicKey, AlgorithmParameterSpec spec, SecureRandom secureRandom) throws InvalidAlgorithmParameterException, InvalidKeyException { if (publicKey == null) { throw new InvalidKeyException("Null key provided"); } return delayed != null ? delayed.newEncapsulator(publicKey, spec, secureRandom) : new Encapsulator(spi.engineNewEncapsulator(publicKey, spec, secureRandom), provider); } ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178422376
