On Wed, 20 Apr 2022 04:19:38 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Daniel Jeliński has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Replace remaining constructors with factory methods > > src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java > line 94: > >> 92: AlgorithmConstraints userSpecifiedConstraints, >> 93: boolean withDefaultCertPathConstraints) { >> 94: if (nullIfDefault(userSpecifiedConstraints) == null) { > > Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation? > The logic of the block is a little bit hard to understand to me. No I don't; it's for the same reason why I'm using `==` and not `equals`: `DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as `userSpecifiedConstraints` here. `DEFAULT` is used because [SSLConfiguration sets userSpecifiedAlgorithmConstraints to SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129). This feels wrong; the name suggests that the constraints should be specified by user, and should be null if the user doesn't touch them. `userSpecifiedAlgorithmConstraints` are accessible by `getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would be concerned that setting my own algorithm constraints would replace the default ones. It doesn't, but that is not immediately apparent. We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out all the other changes from this PR. The only reason why I didn't do that was because it would change the observable behavior (`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you think we can live with that, I'll be happy to do that change. ------------- PR: https://git.openjdk.java.net/jdk/pull/8199