Aias00 commented on PR #6534: URL: https://github.com/apache/shenyu/pull/6534#issuecomment-5156774758
Reviewed #6534 — fix: handle null discoveryUpstreams in proxy selector update. The fix is correct and well-scoped. `ProxySelectorServiceImpl.update` previously called `deleteByDiscoveryHandlerId` and then `getDiscoveryUpstreams().forEach(...)` unconditionally; a null/omitted `discoveryUpstreams` (an optional field per `ProxySelectorAddDTO`) wiped rows inside the transaction and then NPE'd. Moving the delete+insert block inside `if (!CollectionUtils.isEmpty(...))` mirrors the existing create path (`addUpstreamList`, lines 255-278) and resolves issue #6517. On the null/empty path the existing DB rows are now re-fetched and re-synced via `changeUpstream`, which is a safe no-op for `LocalDiscoveryProcessor`/`AbstractDiscoveryProcessor` (they just publish a sync event from the supplied list). Log statements were relocated inside the guard, which is an improvement (the previous "insert count" line was effectively dead on the null path). No blockers. Two nits: - The new `testUpdateWithNullDiscoveryUpstreams` only exercises the `null` case; an empty-list variant (`setDiscoveryUpstreams(Collections.emptyList())`) would cover the other side of `CollectionUtils.isEmpty`, though both collapse to the same path. - The test asserts `deleteByDiscoveryHandlerId` is never called but does not assert `changeUpstream` is invoked with the preserved rows, so the "existing upstreams are re-synced" contract isn't pinned. A `verify(discoveryProcessor).changeUpstream(any(), any())` would lock that. Out-of-scope note (not blocking this PR): `proxySelectorAddDTO.getDiscovery()` is still dereferenced without a null check a few lines above (365-368), so a client omitting both `discovery` and `discoveryUpstreams` would still NPE one line earlier. Worth a follow-up hardening issue. CI is green (ci/e2e/it/it-k8s/CodeQL all SUCCESS); mergeState is BLOCKED only pending review. -- 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]
