On Sun, 17 Sep 2023 13:38:08 GMT, Aleksei Efimov <aefi...@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)
Hello Aleksei, it's good to see the addition of these new APIs. On the naming
front, I like the proposal of using either `ofLiteral` or `parseLiteral`.
The new APIs are proposing to throw an `IllegalArgumentException` if the passed
literal is not an IP address literal. That seems logical. However, the existing
`getByAddress(byte[] ...)` API which accepts a byte form of the raw IP address,
currently throws `UnknownHostException` in the case where the IP address isn't
of a valid length. Should we consider throwing `UnknownHostException` from
these new APIs too? I prefer `IllegalArgumentException` in these new APIs, but
`UnknownHostException` might be something that would need to be considered?
The proposed changes also have text which talks about "ambiguous" address
literals. The term ambiguous seems new in the InetAddress (and sub-classes).
The package-info.java too doesn't have previous references to it. Should we add
some details on what addresses are considered ambiguous (and how/if such
addresses are treated by existing APIs)?
src/java.base/share/classes/java/net/Inet4Address.java line 181:
> 179: /**
> 180: * Parses string with an IPv4 address literal.
> 181: * If string doesn't contain a valid literal - null is returned.
This sentence is a bit confusing, given the implementation details of this
method. I think `null` is only returned if `throwIAE` is false?
src/java.base/share/classes/sun/net/util/IPAddressUtil.java line 135:
> 133: * to {@code false}, or is not set then validation of the address
> string is performed as follows:
> 134: * If string can't be parsed by following IETF IPv4 address string
> literals
> 135: * formatting style rules (default one), but can be parsed by
> following BSD formatting
I realize that this is an existing javadoc comment in an internal utility class
(which is used by public InetAddress API implementation); do you think we
should add a more formal reference to the BSD formatting of IP address, like we
currently do for other RFCs and such in the class level javadoc of
`InetAddress`?
src/java.base/share/classes/sun/net/util/IPAddressUtil.java line 141:
> 139: * @param src input string
> 140: * @param throwIAE {@code "true"} - throw {@code
> IllegalArgumentException} when cannot be parsed as IPv4 address string;
> 141: * {@code "false"} - throw {@code
> "IllegalArgumentException"} only when IPv4 address string is ambiguous.
Are these double quotes around `true`, `false`, `null` and
`IllegalArgumentException` intentional? It seems odd to have those double
quotes when (rightly) using `{@code}`.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/15775#issuecomment-1752441558
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1349928593
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1349926001
PR Review Comment: https://git.openjdk.org/jdk/pull/15775#discussion_r1349930995
