RFR: 8272215: Add InetAddress methods for parsing IP address literals
Daniel Fuchs
dfuchs at openjdk.org
Mon Oct 16 16:18:15 UTC 2023
On Sun, 17 Sep 2023 13:38:08 GMT, Aleksei Efimov <aefimov at openjdk.org> wrote:
> ### Summary
>
> The changes in this PR add new API to `java.net.InetAddress`, `java.net.Inet4Address`, and
> `java.net.Inet6Address` classes to parse IP address literals:
> ```
> method public static java.net.InetAddress java.net.InetAddress.ofLiteral(java.lang.String)
> method public static java.net.Inet4Address java.net.Inet4Address.ofLiteral(java.lang.String)
> method public static java.net.InetAddress java.net.Inet6Address.ofLiteral(java.lang.String)
> ```
>
> ### How new methods differ from existing ones
>
> These methods differ from `InetAddress.getByName` and `InetAddress.getAllByName` in the following ways:
> 1. If a string supplied is not an address literal it is not forwarded to the system-wide resolver, but IllegalArgumentException is thrown instead. The system-wide resolver is never called from these new methods.
> 2. No reverse lookup is performed to resolve a hostname for the supplied address literal - the `InetAddress[46 ]` instances returned by the new `ofLiteral` API has no hostname set.
> 3. Each `ofLiteral` static method returns addresses of its class only. It gives the ability to check if an IP address literal is of a specific address type.
>
> ### The list of noteworthy changes
> - `IPv4-mapped IPv6 address` and `IPv4-compatible IPv6 addresses` require some special handling in the new API to implement all supported IP address types.
> - All address literal parsing code has been moved from `InetAddress.getAllByName` to address type-specific `Inet4Address.parseAddressString` and `Inet6Address.parseAddressString` methods.
> - The text with scoped IPv6 addresses architecture draft IETF file has been replaced from `[draft-ietf-ipngwg-scoping-arch-04.txt]` to reference `RFC 4007: IPv6 Scoped Address Architecture`. The "RFC 4007" has been also added as `@ spec` into Inet6Address class-level Javadoc.
>
> ### Testing
>
> `jdk-tier1`, `jdk-tier2`, and `jdk-tier3` test sets show no failure with the changes.
>
> `java/net` JCK tests are failing with new methods added failure (CSR is planned for this change):
>
> Added Methods
> -------------
>
> java.net.Inet4Address: method public static java.net.Inet4Address java.net.Inet4Address.ofLiteral(java.lang.String)
> java.net.Inet6Address: method public static java.net.InetAddress java.net.Inet6Address.ofLiteral(java.lang.String)
> java.net.InetAddress: method public static java.net.InetAddress java.net.InetAddress.ofLiteral(java.lang.String)
Good work Aleksei, certainly enough to get the discussion going :-)
I'm not entirely sure of some aspects of this proposal though. See my other comments inline.
Sorry for the back and forth suggestions - I hadn't noticed that "ambiguous" was already introduced earlier in the method description. I'd suggest the following refactoring to make it easier to read.
The main motivation here is to have a method that will create an InetAddress from an IP literal, and throw if the string given as parameters doesn't correspond to an IP literal. By comparison, `getByName` will try to parse the given name as an IP literal, and if it can't parse it as an IP literal, pass it to the resolver. The new method will never invoke the resolver and throw in that case. I find the name `ofLiteral` to be closer to what the method does than it would be if we provided it as an override to e.g. `getByAddress`, but naming can be discussed.
Well the name `ofLiteral` clearly conveys that it only accepts a string that represents an IP literal: I'm not sure what's cryptic about it?
src/java.base/share/classes/java/net/Inet4Address.java line 148:
> 146: * <blockquote><ul style="list-style-type:none">
> 147: * <li>{@code 1.2.3.4}</li>
> 148: * <li>{@code 06.07.08.09}</li>
It might be clearer to transform this in a kind of JShell like snippet, showing the Inet4Adress call and what is returned? I would also expect to see an example of IPv4-compatible IPv6 address literal here.
Something like:
* {@snippet:
* Inet4Address.ofLiteral("1.2.3.4") ==> /1.2.3.4
* Inet4Address.ofLiteral("06.07.08.09") ==> /6.7.8.9
* Inet4Address.ofLiteral(""::ffff:1020:3040") ==> /16.32.48.64
* }
As a side note, I suspect that Inet4Adress.ofLiteral() accepting IPv4-compatible IPv6 addresses will be surprising for most users. I wonder if we should change that, and have Inet6Address.ofLiteral() return InetAddress instead of Inet6Address, and take care of that edge case.
As a user of the API I could very well see myself trying to do something like:
if (addr.startsWith("[") && addr.endsWith("]")) {
var ipv6 = addr.substring(1, addr.length() - 1);
try {
Inet6Address.ofLiteral(ipv6);
} catch (...) {
throw new IllegalArgumentException("Invalid IPv6 literal between "[ ]"");
}
}
I'm not saying we shouldn't do what you're suggesting here, but I believe we should get more feedback.
src/java.base/share/classes/java/net/Inet4Address.java line 180:
> 178:
> 179: /**
> 180: * Parses string as an IPv4 address literal.
Suggestion:
* Parses the given string as an IPv4 address literal.
src/java.base/share/classes/java/net/Inet4Address.java line 182:
> 180: * Parses string as an IPv4 address literal.
> 181: * If string contains a non-parsable literal and {@code throwIAE} is set to {@code false}
> 182: * - {@code null} is returned.
Suggestion:
* If the given {@code addressLiteral} string cannot be parsed as an IPv4 address literal
* and {@code throwIAE} is {@code false}, {@code null} is returned.
src/java.base/share/classes/java/net/Inet4Address.java line 184:
> 182: * - {@code null} is returned.
> 183: * If string contains a non-parsable literal and {@code throwIAE} is set to {@code true}
> 184: * - {@code IllegalArgumentException} is thrown.
Suggestion:
* If the given {@code addressLiteral} string cannot be parsed as an IPv4 address literal
* and {@code throwIAE} is {@code true}, an {@code IllegalArgumentException} is thrown.
src/java.base/share/classes/java/net/Inet4Address.java line 189:
> 187: * and {@code throwIAE} is {@code true}, an {@code IllegalArgumentException} is thrown.
> 188: * If string contains an {@linkplain IPAddressUtil#validateNumericFormatV4(String, boolean)
> 189: * ambiguous literal} - {@code IllegalArgumentException} is thrown irrelevant to
Suggestion:
* Otherwise, if it can be considered as {@linkplain IPAddressUtil#validateNumericFormatV4(String, boolean)
* an ambiguous literal} - {@code IllegalArgumentException} is thrown irrelevant to
src/java.base/share/classes/java/net/Inet4Address.java line 190:
> 188: * @param addressLiteral IPv4 address literal to parse
> 189: * @param throwIAE throw {@code IllegalArgumentException} if the
> 190: * literal cannot be parsed as an IPv4 address literal.
Suggestion:
* @param throwIAE whether to throw {@code IllegalArgumentException} if the
* given {@code addressLiteral} string cannot be parsed as
* an IPv4 address literal.
src/java.base/share/classes/java/net/Inet4Address.java line 190:
> 188: * If string contains an {@linkplain IPAddressUtil#validateNumericFormatV4(String, boolean)
> 189: * ambiguous literal} - {@code IllegalArgumentException} is thrown irrelevant to
> 190: * {@code throwIAE} value.
Suggestion:
* {@code throwIAE} value.
*
* @apiNote
* The given {@code addressLiteral} string is considered ambiguous if it cannot be parsed as
* a valid IPv4 address literal using decimal notation, but could be
* interpreted as an IPv4 address in some other representation (octal, hexadecimal, or mixed).
src/java.base/share/classes/java/net/Inet4Address.java line 195:
> 193: * @throws IllegalArgumentException if ambiguous IPv4 literal is specified,
> 194: * or non-parsable IPv4 literal is specified with {@code throwIAE} set to
> 195: * {@code true}.
* @throws IllegalArgumentException if the given {@code addressLiteral} string
* cannot be parsed as an IPv4 address literal and {@code throwIAE} is {@code true}.
* An {@code IllegalArgumentException} is also thrown regardless of the value of
* {@code throwIAE} if the given {@code addressLiteral} string is ambiguous, that is,
* it cannot be parsed as a valid IPv4 address literal using decimal notation but could be
* interpreted as an IPv4 address in some other representation (octal, hexadecimal, or mixed)
src/java.base/share/classes/java/net/Inet4Address.java line 202:
> 200: * {@code throwIAE} if the given {@code addressLiteral} string is ambiguous, that is,
> 201: * it cannot be parsed as a valid IPv4 address literal using decimal notation but could be
> 202: * interpreted as an IPv4 address in some other representation (octal, hexadecimal, or mixed).
Suggestion:
* @throws IllegalArgumentException if the given {@code addressLiteral} string
* cannot be parsed as an IPv4 address literal and {@code throwIAE} is {@code true},
* or if it is considered ambiguous, regardless of the value of the value of
* {@code throwIAE}.
src/java.base/share/classes/java/net/Inet6Address.java line 490:
> 488: * java.net.spi.InetAddressResolver resolver} is not queried to resolve
> 489: * the provided literal, and no reverse lookup is performed.
> 490: * @implNote <a href="Inet6Address.html#special-ipv6-address-heading">
`@implNote` is not appropriate here. This should be part of the spec (if we keep it). See my other comment above in `Inet4Address`.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15775#pullrequestreview-1631177877
PR Review: https://git.openjdk.org/jdk/pull/15775#pullrequestreview-1676057283
PR Comment: https://git.openjdk.org/jdk/pull/15775#issuecomment-1723443367
PR Comment: https://git.openjdk.org/jdk/pull/15775#issuecomment-1724040184
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1328799014
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1352956961
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1352979935
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1352986216
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1358006767
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1353001177
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1357992180
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1353026855
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1357995298
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1328804396
More information about the net-dev
mailing list