Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
On Thu, 28 Apr 2022 18:29:35 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: > > Max and Brad comments round two LGTM, Pt2... - 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 [v4]
On Thu, 28 Apr 2022 16:23:25 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82: >> >>> 80: String type; >>> 81: type = GetPropertyAction.privilegedGetProperty( >>> 82: "ssl.TrustManagerFactory.algorithm"); >> >> Sorry I got it wrong here, this is a security property. > > Similar comment. Back to the original lambda expression. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
On Thu, 28 Apr 2022 18:29:35 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: > > Max and Brad comments round two Final changes LGTM. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
> 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: Max and Brad comments round two - Changes: - all: https://git.openjdk.java.net/jdk/pull/8384/files - new: https://git.openjdk.java.net/jdk/pull/8384/files/6997837f..15a9f3b8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8384=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8384=03-04 Stats: 14 lines in 3 files changed: 7 ins; 2 del; 5 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
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 17:29:53 GMT, Bradford Wetmore wrote: >> My mistake. It's only the trim that you wanted removed, line 94. > > No, the API for Security.getProperty doesn't specify trimming, so suggest > leaving the trim() part also. Okay. Line 94 is back. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:37:35 GMT, Mark Powers wrote: >> `Security.getProperty()` does not specify the value will be `trim()`. > > My mistake. It's only the trim that you wanted removed, line 94. No, the API for Security.getProperty doesn't specify trimming, so suggest leaving the trim() part also. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:27:08 GMT, Bradford Wetmore wrote: >> Just found the same. This needs to be reverted. You can set a Security >> Property to an "empty" string which won't work here. Suggest you revert to >> previous code, possibly using a lambda if that was the original intent. > > `Security.getProperty()` does not specify the value will be `trim()`. My mistake. It's only the trim that you wanted removed, line 94. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:22:43 GMT, Bradford Wetmore wrote:
>> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
>>
>>> 90: static String getSecurityProperty(final String name) {
>>> 91: return AccessController.doPrivileged((PrivilegedAction)
>>> () -> {
>>> 92: return Security.getProperty(name);
>>
>> I assume we still need to do the if-empty-then-null conversion?
>
> Just found the same. This needs to be reverted. You can set a Security
> Property to an "empty" string which won't work here. Suggest you revert to
> previous code, possibly using a lambda if that was the original intent.
`Security.getProperty()` does not specify the value will be `trim()`.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:14:01 GMT, Bradford Wetmore wrote: >> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: >> >>> 68: String type; >>> 69: type = GetPropertyAction.privilegedGetProperty( >>> 70: "ssl.KeyManagerFactory.algorithm"); >> >> So sorry I got it wrong here, this is a security property. >> `GetPropertyAction.privilegedGetProperty` is for system properties. > > I just noticed the same. Interesting that mach5 tier1 and tier2 tests passed. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:45:58 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains eight additional
>> commits since the last revision:
>>
>> - Merge
>> - Max and Brad comments
>> - jaikiran comments
>> - Alan Bateman comments
>> - second iteration
>> - Merge
>> - Merge
>> - first iteration
>
> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
>
>> 90: static String getSecurityProperty(final String name) {
>> 91: return AccessController.doPrivileged((PrivilegedAction)
>> () -> {
>> 92: return Security.getProperty(name);
>
> I assume we still need to do the if-empty-then-null conversion?
Just found the same. This needs to be reverted. You can set a Security
Property to an "empty" string which won't work here. Suggest you revert to
previous code, possibly using a lambda if that was the original intent.
> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:
>
>> 80: String type;
>> 81: type = GetPropertyAction.privilegedGetProperty(
>> 82: "ssl.TrustManagerFactory.algorithm");
>
> Sorry I got it wrong here, this is a security property.
Similar comment.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - Max and Brad comments > - jaikiran comments > - Alan Bateman comments > - second iteration > - Merge > - Merge > - first iteration These need to be addressed before integration. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:47:44 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains eight additional >> commits since the last revision: >> >> - Merge >> - Max and Brad comments >> - jaikiran comments >> - Alan Bateman comments >> - second iteration >> - Merge >> - Merge >> - first iteration > > src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: > >> 68: String type; >> 69: type = GetPropertyAction.privilegedGetProperty( >> 70: "ssl.KeyManagerFactory.algorithm"); > > So sorry I got it wrong here, this is a security property. > `GetPropertyAction.privilegedGetProperty` is for system properties. I just noticed the same. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 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 with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains eight additional
> commits since the last revision:
>
> - Merge
> - Max and Brad comments
> - jaikiran comments
> - Alan Bateman comments
> - second iteration
> - Merge
> - Merge
> - first iteration
I made some wrong suggestions earlier.
src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70:
> 68: String type;
> 69: type = GetPropertyAction.privilegedGetProperty(
> 70: "ssl.KeyManagerFactory.algorithm");
So sorry I got it wrong here, this is a security property.
`GetPropertyAction.privilegedGetProperty` is for system properties.
src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
> 90: static String getSecurityProperty(final String name) {
> 91: return AccessController.doPrivileged((PrivilegedAction)
> () -> {
> 92: return Security.getProperty(name);
I assume we still need to do the if-empty-then-null conversion?
src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:
> 80: String type;
> 81: type = GetPropertyAction.privilegedGetProperty(
> 82: "ssl.TrustManagerFactory.algorithm");
Sorry I got it wrong here, this is a security property.
-
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 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 [v4]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge - Max and Brad comments - jaikiran comments - Alan Bateman comments - second iteration - Merge - Merge - first iteration - Changes: - all: https://git.openjdk.java.net/jdk/pull/8384/files - new: https://git.openjdk.java.net/jdk/pull/8384/files/7624f4a9..6997837f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8384=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8384=02-03 Stats: 12840 lines in 345 files changed: 9325 ins; 1407 del; 2108 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
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 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: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 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 [v3]
> 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: jaikiran comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8384/files - new: https://git.openjdk.java.net/jdk/pull/8384/files/d964c05a..7624f4a9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8384=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8384=01-02 Stats: 3 lines in 1 file changed: 1 ins; 0 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
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=8384=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8384=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
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net
On Mon, 25 Apr 2022 18:48:31 GMT, Alan Bateman 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 > > src/java.base/share/classes/javax/net/ssl/SSLSessionContext.java line 110: > >> 108: */ >> 109: void setSessionTimeout(int seconds) >> 110: throws IllegalArgumentException; > > IllegalArgumentException is a runtime exception, it's unusual to have "throws > IllegalArgumentException". It would not impact compatibility to drop it. The > important thing is that the javadoc has the `@throws` describing when it is > thrown. I'll drop it then. Thanks for noticing this. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net
On Mon, 25 Apr 2022 17:40:13 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 src/java.base/share/classes/javax/net/ssl/SSLSessionContext.java line 110: > 108: */ > 109: void setSessionTimeout(int seconds) > 110: throws IllegalArgumentException; IllegalArgumentException is a runtime exception, it's unusual to have "throws IllegalArgumentException". It would not impact compatibility to drop it. The important thing is that the javadoc has the `@throws` describing when it is thrown. - PR: https://git.openjdk.java.net/jdk/pull/8384
RFR: JDK-8285504 Minor cleanup could be done in javax.net
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 - Commit messages: - second iteration - Merge - Merge - first iteration Changes: https://git.openjdk.java.net/jdk/pull/8384/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8384=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285504 Stats: 128 lines in 20 files changed: 1 ins; 21 del; 106 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
