On Mon, 18 May 2026 19:12:41 GMT, Artur Barashev <[email protected]> wrote:
> I wonder if we need to essentially duplicate new(String) and new(byte[])
> javadocs ...
I'd like to take a step back and reflect a bit on this work.
### Problem statement
The problem we're aiming to address with this PR is, since `SNIHostName::new`
_incorrectly_ accepts IP literal addresses, use-sites need to be amended with:
if (isIPLiteralAddress(hostName)) {
var sni = new SNIHostName(hostName);
// ...
}
Iff corpus search would have shown that use-sites in the wild have the
following shape:
SNIHostName sni = null;
try {
sni = new SNIHostName(hostName);
catch (IllegalArgumentException iae) { /* Maybe log? */ }
if (sni != null) { /* ... */ }
we could have retrofitted `SNIHostName::new`, but they don't. Again, what
exactly is the problem? It is two-fold:
1. Use-sites don't catch `IAE`
2. `isIPLiteralAddress(String)` is non-trivial to implement, particularly for
IPv6
### Workarounds
There're relatively simple hacks one can reach to workaround the problem.
#### Workaround 1: Use regex for IPv4, and catch IAE for IPv6
This workaround is already [exercised by
Log4j](https://github.com/apache/logging-log4j2/blob/3bb440af8e7da6b5b9030391e91bc47f25d95f83/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SslSocketManager.java#L406-L432):
SNIHostName sni = null;
if (!hostName.matches("\\d+[.]\\d+[.]\\d+[.]\\d+")) {
try {
sni = new SNIHostName(hostName);
} catch (IllegalArgumentException iae) { /* Maybe log? */ }
}
if (sni != null) { /* ... */ }
This works, because `SNIServerName::new` throws on IPv6 literal addresses,
since they don't qualify as a valid host name by `IDN::toASCII`, which is used
by `SNIServerName::new` under the hood. Hence, checking against only for IPv4
is fine.
#### Workaround 2: Use regex for IPv4, and magic character check for IPv6
if (hostName.matches("\\d+[.]\\d+[.]\\d+[.]\\d+") || hostName.contains(":")) {
/* Maybe log? */ }
else {
var sni = new SNIHostName(hostName);
// ...
}
This works, because `:` (used by IPv6 literal addresses) is an invalid
character. This pretty much resembles Workaround 1.
### Proposed solution
In b8a7cdd, we removed the guidance advising users to catch `IAE`. If we copy
the Javadoc from `SNIHostName::new`, we revert to our original problem:
constructors (in disguise of static factory methods) with overly rigid API
specifications and insufficient guidance.
Considering the existence of practical workarounds, and the marginal benefit of
new factory methods, we can alternatively consider shelving this work off, and,
maybe, only keep the tests.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/30747#issuecomment-4486048194
