Aias00 commented on PR #6456:
URL: https://github.com/apache/shenyu/pull/6456#issuecomment-5156775975
Reviewed #6456 — IPv6 address parsing in upstream URL handling.
Thanks for this — the IPv6-aware parsing in `IpUtils.parseHostPort` and the
`URI`-based `checkUrl` rewrite look correct (verified `URI.getHost()` returns
the bracketed IPv6 form and `InetSocketAddress` accepts it). But there's a
blocking issue plus a few consistency gaps to address before merge.
**Blocker — CI is red (NPE regression in `nativeCreateOrUpdate`)**
`DiscoveryUpstreamServiceImpl.nativeCreateOrUpdate` now calls
`CommonUpstreamUtils.normalizeUrl(...)` unconditionally, and `normalizeUrl` →
`IpUtils.parseHostPort` does `upstreamUrl.startsWith("[")` with no null guard.
The existing `DiscoveryUpstreamServiceTest.testNativeCreate` builds a DTO with
no URL set, so the PR now throws:
```
java.lang.NullPointerException: Cannot invoke "String.startsWith(String)"
because "upstreamUrl" is null
at
DiscoveryUpstreamServiceTest.testNativeCreate(DiscoveryUpstreamServiceTest.java:205)
at
DiscoveryUpstreamServiceTest.testNativeCreateOrUpdate(DiscoveryUpstreamServiceTest.java:127)
```
This is the actual cause of the `build (17, windows-latest)` FAILURE (`ci`
workflow); the ubuntu matrix was cancelled by fail-fast. `mergeStateStatus` is
BLOCKED. Beyond the test, it's a runtime regression: master inserted the row
(with null url) without throwing, the PR now 500s. The new
`DiscoveryUpstreamServiceImplTest` always sets a URL via `buildDTO`, so it
masks the regression; the pre-existing `DiscoveryUpstreamServiceTest` was not
updated.
Fix options: null/blank-guard `IpUtils.parseHostPort` (or `normalizeUrl`),
and add the same exception guard to the service path that you already added to
the listener's `handleAdded/Updated/Deleted`. Then fix `testNativeCreate` to
set a URL (or assert the new typed exception).
**Should-fix**
1. Listener swallows exceptions inside `@Transactional`.
`handleUpdated`/`handleDeleted` catch `Exception`, log, and continue, but
`onChange` is `@Transactional(rollbackFor=Exception.class)` — so partial
batches now commit instead of rolling back (master propagated → rollback).
Either rethrow after logging to keep atomicity, or drop the `@Transactional`
and document per-item best-effort; don't swallow-and-commit silently.
2. Service-layer write paths lack the host:port fallback.
`deleteBySelectorIdAndUrl` and `changeStatusBySelectorIdAndUrl` only
exact-match the normalized URL; the listener additionally falls back to
`matchByHostAndPort` for old-format records. So old `example.com` (no port) or
unbracketed `2001:db8::1:8080` rows can't be deleted/status-toggled via the
register/REST path until migrated, while the event path can. Reuse
`matchByHostAndPort` in both service methods (delete/update by id) so all write
paths match.
3. `mapToCommonUpstream` is now throwable in unguarded streams. Replacing
`url.split(":")[0]` with `parseHostPort(url)[0]` means malformed bracketed
input (`[broken`, `[]`, `[localhost]`) throws `IllegalArgumentException`.
Callers in `UpstreamCheckService` (lines 482-484, 509) map with no try/catch —
one bad row breaks the whole DIVIDE stream. Skip-and-log per item, or return
null on parse failure and filter.
**Nits**
- `normalizeUrl` does `Integer.parseInt(parts[1])` but `parseHostPort`
doesn't validate the port token is numeric (`example.com:abc` → uncaught
NumberFormatException in the service path). Validate the port in
`parseHostPort` or guard callers.
- IPv6 coverage stops at admin/common. `ShenyuResolverHelper` (grpc) and
`PrxInfoUtil` (tars) still `split(":")` and will NumberFormatException on
`[ipv6]:port`. Pre-existing (unbracketed form was already broken), but please
scope the PR description to admin/common, or open a follow-up for the
gateway-side consumers.
Tests: the new `IpUtilsTest` / `CommonUpstreamUtilsTest` cases are good and
cover IPv4/IPv6/hostname/bracket edge cases. Please also add round-trip
`normalizeUrl` tests for hostname-without-port and IPv6-with-zone. And re-run
CI with `fail-fast: false` so the ubuntu matrix actually executes.
--
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]
