You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "garydgregory (via GitHub)" <gi...@apache.org> on 2023/10/23 11:50:12 UTC
Re: [PR] [VFS-524] A URI with an IPv6 address can't be parsed out correctly [commons-vfs]
garydgregory commented on code in PR #438:
URL: https://github.com/apache/commons-vfs/pull/438#discussion_r1368539685
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/HostFileNameParser.java:
##########
@@ -357,4 +369,21 @@ public FileName parseUri(final VfsComponentContext context, final FileName base,
return new GenericFileName(auth.scheme, auth.hostName, auth.port, defaultPort, auth.userName, auth.password,
path, fileType);
}
+
+ private static boolean isIPv6HostHeadingChar(final char ch) {
+ return ch == '[';
+ }
+
+ private static boolean isHostNameTerminatingChar(final char ch, final boolean isIPv6Host) {
+ if (isIPv6Host) {
+ return ch == ']';
+ }
+
Review Comment:
Hi @fyudanov
Thank you for this PR! 😄
The expression below does not look right to me or the method is misnamed, but the code sure could use some comments 😉
Below, we have the test for the last char of a host name:
```
return ch == '/' || ch == ';' || ch == '?' || ch == ':' || ch == '@' || ch == '&' || ch == '=' || ch == '+'
|| ch == '$' || ch == ',';
```
Reading https://datatracker.ietf.org/doc/html/rfc3986#page-49:
```
host = IP-literal / IPv4address / reg-name
...
reg-name = *( unreserved / pct-encoded / sub-delims )
...
unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
/ "*" / "+" / "," / ";" / "="
```
For example, shouldn't `~` be in the test. Also the method needs comments if you intend to also test for a separator char with rest of the path like `/` and `?`. Or am I missing something?
At least this PR needs more testing with host names and URI that contain all the these special characters.
##########
commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/HostFileNameParser.java:
##########
@@ -357,4 +369,21 @@ public FileName parseUri(final VfsComponentContext context, final FileName base,
return new GenericFileName(auth.scheme, auth.hostName, auth.port, defaultPort, auth.userName, auth.password,
path, fileType);
}
+
+ private static boolean isIPv6HostHeadingChar(final char ch) {
+ return ch == '[';
+ }
+
+ private static boolean isHostNameTerminatingChar(final char ch, final boolean isIPv6Host) {
+ if (isIPv6Host) {
+ return ch == ']';
+ }
+
Review Comment:
Hi @fyudanov
Thank you for this PR! 😄
The expression below does not look right to me or the method is misnamed, but the code sure could use some comments 😉
Below, we have the test for the last char of a host name:
```
return ch == '/' || ch == ';' || ch == '?' || ch == ':' || ch == '@' || ch == '&' || ch == '=' || ch == '+'
|| ch == '$' || ch == ',';
```
Reading https://datatracker.ietf.org/doc/html/rfc3986#page-49:
```
host = IP-literal / IPv4address / reg-name
...
reg-name = *( unreserved / pct-encoded / sub-delims )
...
unreserved = ALPHA / DIGIT / "-" / "." / "_" / "~"
sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
/ "*" / "+" / "," / ";" / "="
```
For example, shouldn't `~` be in the test. Also the method needs comments if you intend to also test for a separator char with rest of the path like `/` and `?`. Or am I missing something?
At least this PR needs more testing with host names and URI that contain all the these special characters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org