Copilot commented on code in PR #6456:
URL: https://github.com/apache/shenyu/pull/6456#discussion_r3683945444
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/transfer/DiscoveryTransfer.java:
##########
@@ -349,19 +349,12 @@ public DiscoveryUpstreamDTO mapToDTO(DiscoveryUpstreamDO
discoveryUpstreamDO) {
*/
public DiscoveryUpstreamData mapToDiscoveryUpstreamData(CommonUpstream
commonUpstream) {
String upstreamUrl = commonUpstream.getUpstreamUrl();
- String[] parts = Optional.ofNullable(upstreamUrl)
- .map(url -> url.split(":", 2))
- .orElseThrow(() -> new IllegalArgumentException("Upstream URL
must not be null"));
- if (parts.length < 2) {
- throw new IllegalArgumentException("Invalid upstream URL, expected
'host:port' format but was: " + upstreamUrl);
+ if (upstreamUrl == null) {
+ throw new IllegalArgumentException("Upstream URL must not be
null");
}
+ String[] parts = CommonUpstreamUtils.parseHostPort(upstreamUrl);
String host = parts[0];
- int port;
- try {
- port = Integer.parseInt(parts[1]);
- } catch (NumberFormatException ex) {
- throw new IllegalArgumentException("Invalid port in upstream URL:
" + upstreamUrl, ex);
- }
+ int port = Integer.parseInt(parts[1]);
Review Comment:
This change removes the previous explicit error handling for invalid ports
(which threw an `IllegalArgumentException` including the upstream URL) and will
now throw a raw `NumberFormatException` without context. Recommend catching
`NumberFormatException` here and rethrowing an `IllegalArgumentException` with
a clear message (and cause) that includes `upstreamUrl`.
##########
shenyu-common/src/test/java/org/apache/shenyu/common/utils/UpstreamCheckUtilsTest.java:
##########
@@ -43,6 +44,21 @@ public void testBlank() {
assertFalse(UpstreamCheckUtils.checkUrl(""));
}
+ @Test
+ public void testIpv6BareHostPortDoesNotThrowException() {
+ assertDoesNotThrow(() ->
UpstreamCheckUtils.checkUrl("[2001:db8::1]:8080"));
+ }
+
+ @Test
+ public void testIpv6UrlDoesNotThrowException() {
+ assertDoesNotThrow(() ->
UpstreamCheckUtils.checkUrl("http://[2001:db8::1]:8080";));
+ }
+
+ @Test
+ public void testIpv6BareHostPortWithoutPortDoesNotThrowException() {
+ assertDoesNotThrow(() -> UpstreamCheckUtils.checkUrl("[2001:db8::1]"));
+ }
Review Comment:
These tests call `checkUrl(...)`, which performs a real socket connect and
may wait up to the default timeout (and potentially behave differently across
environments with/without IPv6), making tests slower/flakier than needed.
Prefer calling the `checkUrl(url, timeout)` overload with a very small timeout
for these parsing-focused tests, or refactor to test the parsing logic without
doing network I/O.
##########
shenyu-admin/src/main/java/org/apache/shenyu/admin/utils/CommonUpstreamUtils.java:
##########
@@ -264,6 +265,19 @@ public static List<CommonUpstream>
convertCommonUpstreamList(final List<? extend
* @return the string
*/
public static String buildUrl(final String host, final Integer port) {
- return Optional.of(String.join(":", host,
String.valueOf(port))).orElse(null);
+ if (Objects.nonNull(host) && host.contains(":")) {
+ return String.format("[%s]:%d", host, port);
+ }
+ return String.join(":", host, String.valueOf(port));
Review Comment:
`buildUrl` will double-bracket if `host` is already bracketed (e.g., `host
== \"[::1]\"` becomes `\"[[::1]]:8080\"`). To avoid producing invalid upstream
URLs, detect already-bracketed hosts (starts with `[` and ends with `]`) and
only add brackets when they’re missing.
##########
shenyu-common/src/main/java/org/apache/shenyu/common/utils/UpstreamCheckUtils.java:
##########
@@ -62,16 +62,23 @@ public static boolean checkUrl(final String url, final int
timeout) {
if (StringUtils.isBlank(url)) {
return false;
}
- String[] hostPort;
+ final String host;
+ final int port;
if (url.startsWith(HTTP) || url.startsWith(HTTPS)) {
- final String[] http = StringUtils.split(url, "\\/\\/");
- hostPort = StringUtils.split(http[1], Constants.COLONS);
+ try {
+ URI uri = new URI(url);
+ host = uri.getHost();
+ port = uri.getPort() == -1 ? url.startsWith(HTTPS) ? 443 : 80
: uri.getPort();
+ } catch (Exception e) {
+ LOG.error("Invalid URL: {}", url, e);
+ return false;
+ }
} else {
- hostPort = StringUtils.split(url, Constants.COLONS);
+ String[] parts = IpUtils.parseHostPort(url);
+ host = parts[0];
+ port = Integer.parseInt(parts[1]);
Review Comment:
`checkUrl` now relies on `IpUtils.parseHostPort` and `Integer.parseInt`, but
exceptions from malformed inputs (e.g., missing `]`, empty/invalid port) will
currently propagate and can break callers expecting `checkUrl` to return
`false` on bad URLs. Consider wrapping the non-http(s) parsing branch in a
try/catch (similar to the URI path) and returning `false` with a log message on
parse failure.
##########
shenyu-common/src/main/java/org/apache/shenyu/common/utils/IpUtils.java:
##########
@@ -416,6 +416,27 @@ public static String getZookeeperHost(final String
zookeeperUrl) {
return null;
}
+ /**
+ * Parse host and port from a host:port string, supporting IPv6 bracket
notation.
+ *
+ * @param upstreamUrl the upstreamUrl in "host:port" or "[ipv6]:port"
format
+ * @return string array with [host, port], port defaults to "80" if not
present
+ */
+ public static String[] parseHostPort(final String upstreamUrl) {
+ if (upstreamUrl.startsWith("[")) {
+ int closingBracket = upstreamUrl.lastIndexOf(']');
+ String host = upstreamUrl.substring(1, closingBracket);
+ String port = closingBracket < upstreamUrl.length() - 1 &&
upstreamUrl.charAt(closingBracket + 1) == ':'
+ ? upstreamUrl.substring(closingBracket + 2) : "80";
+ return new String[]{host, port};
+ }
+ int lastColon = upstreamUrl.lastIndexOf(':');
+ if (lastColon == -1) {
+ return new String[]{upstreamUrl, "80"};
+ }
+ return new String[]{upstreamUrl.substring(0, lastColon),
upstreamUrl.substring(lastColon + 1)};
+ }
Review Comment:
`parseHostPort` can throw (e.g., missing `]` makes `substring(1, -1)` fail),
and it can return an empty port for inputs like `host:` or `[::1]:`, which will
later cause `NumberFormatException`. Make this method robust by validating
`closingBracket >= 0` and defaulting the port to `\"80\"` when the extracted
port substring is blank/empty (or alternatively throw a clear
`IllegalArgumentException` here).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
