Aias00 commented on PR #6530:
URL: https://github.com/apache/shenyu/pull/6530#issuecomment-5156775338
Reviewed #6530. The fix correctly closes the reported path in #6466 (POST
`/discovery-upstream` with a non-existent `discoveryHandlerId` → orphan row +
NPE in `fetchAll`). It reuses the existing
`@Existed`/`DiscoveryHandlerMapper.existed` validator the repo already uses for
the `id` and `namespaceId` fields and for the PUT path variable, so it's
consistent and correct for the HTTP create/update paths. No blocker from me.
Two non-blocking items:
1. **No regression test (should_fix).** Issue #6466 ships a 4-step repro and
the PR adds no test. The H2 `discovery_handler` table (`schema.sql:1249`),
`AbstractSpringIntegrationTest` (`@ActiveProfiles("h2")`), and
`DiscoveryUpstreamMapperTest.existed()` already provide the harness. Please add
a controller/validation test that POSTs a `DiscoveryUpstreamDTO` with a
non-existent `discoveryHandlerId` and asserts a 400 (plus a happy-path 200 with
an existing handler) — otherwise a future revert/refactor of this annotation
regresses silently. The PR checklist also leaves "submit test cases" unchecked.
2. **Update endpoint behavior (nit).** `PUT
/discovery-upstream/{discoveryHandlerId}` now `@Existed`-validates the
`discoveryHandlerId` inside each body element, but
`DiscoveryUpstreamServiceImpl.updateBatch` (`:115`) overwrites it with the path
id and ignores the body value. If the admin frontend sends a
placeholder/mismatched id in the PUT body, this becomes a new 400. Please
confirm the frontend sends the matching path id in each body element; if so, no
action needed.
Out of scope (follow-up, not for this PR): the import/programmatic paths
(`DiscoveryUpstreamServiceImpl.importData` at `:255`/`:293`,
`nativeCreateOrUpdate` at `:123`, fed by Gson in `ConfigsServiceImpl:440` and
`DiscoveryUpstreamDataConfigsExportImportHandler:59`) bypass `@Valid` and can
still insert orphan rows. They don't call `fetchAll`, so they dodge the NPE in
#6466 — hence not blocking here — but a service-level existence check would
fully match the issue title's intent.
--
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]
