Aias00 commented on PR #6532: URL: https://github.com/apache/shenyu/pull/6532#issuecomment-5156775073
Reviewing #6532 (fixes #6465) — one-line change to `DiscoveryUpstreamServiceImpl#importData(String, List, ConfigsImportContext)`. **The fix is correct.** `discoveryHandlerUpstreamMap` is keyed by target/DB `discoveryHandlerId` (grouped from `selectByNamespaceId`), but the DTO carries the *source* handler id, and `discoveryHandlerIdMapping` (populated in `DiscoveryServiceImpl#importData`, `put(oldId, newId)`) is the old→new map. The old code looked up the source id in a DB-id-keyed map, so the duplicate check always returned an empty set and the insert then collided with `UNIQUE(discovery_handler_id, upstream_url)`. Using `discoveryHandlerIdMapping.getOrDefault(id, id)` to remap before the lookup is the right fix. Import order is also sound: `ConfigsExportImportEnum` runs Discovery (9) before DiscoveryUpstream (10), so the mapping is populated in time. No blockers. Two things I'd like addressed before merge: 1. **No test for the fixed overload.** `DiscoveryUpstreamServiceTest#testImportData` only exercises `importData(List)`, not the `(namespace, list, context)` method this PR actually changes — and the PR checklist leaves the test box unchecked. Please add a unit test that stubs `selectByNamespaceId` with an existing upstream under the *remapped* handler id and asserts the DTO (carrying the *source* id + duplicate url) is skipped with `successCount=0`. That's the precise regression being fixed and it's currently unguarded. 2. **Namespace overload is still not `@Transactional`.** Issue #6465's "Expected Behavior" explicitly asks for transactional safety so partial imports aren't left behind on insert failure. The `importData(List)` overload has `@Transactional(rollbackFor = Exception.class)`; the namespace overload this PR touches does not. Adding the annotation would close the issue's secondary requirement and is consistent with the sibling overload. Nits (non-blocking, pre-existing): - `discoveryHandlerUpstreamMap` is built once before the loop and not updated after each insert, so two DTOs in the same batch with the same (remapped handler id, url) will: first inserts OK, second bypasses the stale in-memory check and hits the DB unique key. Same pattern exists in the List overload, so not a regression — but worth a follow-up to fold the import batch into the dedup set. - Minor asymmetry: L308 uses `getOrDefault(id, id)` (falls back to source id) while L320 uses `.get(id)` (returns null on miss). If a source handler id were ever absent from the mapping, the dup-check would run against the source id while the insert would null the field. I couldn't find a reachable path that produces such an upstream (the export always includes the handler), but a hand-edited import JSON could, so making the two lookups consistent (or documenting the invariant) would be safer. -- 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]
