Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Wed, 27 Apr 2022 20:22:42 GMT, Mark Powers wrote: >> JDK-6725221 is about obtaining boolean properties, so not an exact match. >> The suggested change is so easy, I'm going to do it. > > sun.security.action.GetPropertyAction::privilegedGetProperty doesn't trim the > return value. Could this be a problem? Answer is no. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Wed, 27 Apr 2022 19:12:37 GMT, Mark Powers wrote: >> No problem. > > JDK-6725221 is about obtaining boolean properties, so not an exact match. The > suggested change is so easy, I'm going to do it. sun.security.action.GetPropertyAction::privilegedGetProperty doesn't trim the return value. Could this be a problem? - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 19:09:46 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan Bateman comments > > src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 81: > >> 79: String type; >> 80: type = AccessController.doPrivileged((PrivilegedAction) >> () -> >> 81: Security.getProperty( "ssl.TrustManagerFactory.algorithm")); > > Another `GetPropertyAction::privilegedGetProperty` candidate. Making the change. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 22:08:42 GMT, Weijun Wang wrote: >> Perhaps as part of >> [JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)? > > No problem. JDK-6725221 is about obtaining boolean properties, so not an exact match. The suggested change is so easy, I'm going to do it. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 19:08:50 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan Bateman comments > > src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 96: > >> 94: s = s.trim(); >> 95: if (s.isEmpty()) { >> 96: s = null; > > According to the implementation of `Security::getProperty`. The return value > is already trimmed. Removed those lines. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Wed, 27 Apr 2022 15:22:08 GMT, Mark Powers wrote: >> src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line >> 37: >> >>> 35: * {@link SSLSession#putValue(String, Object)} >>> 36: * or {@link SSLSession#removeValue(String)}, objects which >>> 37: * implement the SSLSessionBindingListener will receive an >> >> If you're up for it, you could fix the missing `` or `@link` >> throughout this small file. Ignore otherwise. > > I'll ignore for now. Javadoc issues are already being tracked for > javax.crypto with JDK-8284851. This bug could easily be expanded to include > javax.net. Ok. I'm going to do a little surgery in a java.security this morning, but JDK-8284851 could probably be expanded to address both the JCA/JCE code. (java/security, javax/crypto). - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 18:46:02 GMT, Bradford Wetmore wrote: >> Mark Powers has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan Bateman comments > > src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line 37: > >> 35: * {@link SSLSession#putValue(String, Object)} >> 36: * or {@link SSLSession#removeValue(String)}, objects which >> 37: * implement the SSLSessionBindingListener will receive an > > If you're up for it, you could fix the missing `` or `@link` > throughout this small file. Ignore otherwise. I'll ignore for now. Javadoc issues are already being tracked for javax.crypto with JDK-8284851. This bug could easily be expanded to include javax.net. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 20:40:50 GMT, Bradford Wetmore wrote: >> Wasn't there another bug to address this? > > Perhaps as part of > [JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)? No problem. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 19:49:11 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69: >> >>> 67: String type; >>> 68: type = AccessController.doPrivileged((PrivilegedAction) >>> () -> >>> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm")); >> >> We can probably use >> `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is >> inside `java.base` so that class is always available. > > Wasn't there another bug to address this? Perhaps as part of [JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)? - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 19:05:05 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan Bateman comments > > src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69: > >> 67: String type; >> 68: type = AccessController.doPrivileged((PrivilegedAction) >> () -> >> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm")); > > We can probably use > `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is > inside `java.base` so that class is always available. Wasn't there another bug to address this? - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 18:39:33 GMT, Bradford Wetmore wrote: >> Mark Powers has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan Bateman comments > > src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line > 72: > >> 70: >> 71: this.parameters = List.copyOf(parameters); >> 72: } > > If you leave this as is, you can use `<>` Using <>. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Alan Bateman comments Looks fine. Some small comments. src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69: > 67: String type; > 68: type = AccessController.doPrivileged((PrivilegedAction) > () -> > 69: Security.getProperty("ssl.KeyManagerFactory.algorithm")); We can probably use `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is inside `java.base` so that class is always available. src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 96: > 94: s = s.trim(); > 95: if (s.isEmpty()) { > 96: s = null; According to the implementation of `Security::getProperty`. The return value is already trimmed. src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 81: > 79: String type; > 80: type = AccessController.doPrivileged((PrivilegedAction) > () -> > 81: Security.getProperty( "ssl.TrustManagerFactory.algorithm")); Another `GetPropertyAction::privilegedGetProperty` candidate. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Alan Bateman comments Other than the comments mentioned, LGTM. src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line 37: > 35: * {@link SSLSession#putValue(String, Object)} > 36: * or {@link SSLSession#removeValue(String)}, objects which > 37: * implement the SSLSessionBindingListener will receive an If you're up for it, you could fix the missing `` or `@link` throughout this small file. Ignore otherwise. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Alan Bateman comments src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 72: > 70: > 71: this.parameters = List.copyOf(parameters); > 72: } If you leave this as is, you can use `<>` - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 04:37:58 GMT, Jaikiran Pai wrote: >> Mark Powers has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Alan Bateman comments > > src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line > 71: > >> 69: } >> 70: >> 71: this.parameters = List.copyOf(parameters); > > Hello Mark, this would actually be a change in behaviour. The `List.copyOf` > says: > >> The given Collection must not be null and it must not contain any null >> elements. > > The current implementation of the public constructor on the public > `KeyStoreBuilderParameters` mandates no such requirement. So if there's some > code which currently passes a list with a null element in it, then this > change will now end up throwing a `NullPointerException` as per the contract > of `List.copyOf`. You are correct. This is not a good change since it changes behavior. Going back to the original. Thanks for your review! - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Alan Bateman comments src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 71: > 69: } > 70: > 71: this.parameters = List.copyOf(parameters); Hello Mark, this would actually be a change in behaviour. The `List.copyOf` says: > The given Collection must not be null and it must not contain any null > elements. The current implementation of the public constructor on the public `KeyStoreBuilderParameters` mandates no such requirement. So if there's some code which currently passes a list with a null element in it, then this change will now end up throwing a `NullPointerException` as per the contract of `List.copyOf`. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
> https://bugs.openjdk.java.net/browse/JDK-8285504 > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/net Mark Powers has updated the pull request incrementally with one additional commit since the last revision: Alan Bateman comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8384/files - new: https://git.openjdk.java.net/jdk/pull/8384/files/5ac7f1ec..d964c05a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=00-01 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8384.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8384/head:pull/8384 PR: https://git.openjdk.java.net/jdk/pull/8384