fix: Discovery_upstream_import_checks_duplicates_before_remapping_handler_ids

[hengyuss]

Fixes: #6465

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

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.

[Aias00]

Both fixes look correct and well-scoped. The duplicate-check key fix is right — discoveryHandlerUpstreamMap is keyed by DiscoveryUpstreamDO::getDiscoveryHandlerId (L303, i.e. the stored/remapped id), so looking it up via discoveryHandlerIdMapping.getOrDefault(id, id) is the correct way to find existing upstreams for the same handler. The getOrDefault(id, id) on the remap (L320) is a real data-integrity fix — .get(...) returning null on an unmapped id previously wrote rows with a null discoveryHandlerId. Adding @Transactional(rollbackFor = Exception.class) is a reasonable defensive add.

One coverage gap: the fallback path for the second fix (an unmapped handler id kept as-is via getOrDefault(id, id)) isn't tested. The new test only uses a mapped id (old_handler_id→new_handler_id). A second case where discoveryHandlerId is not in discoveryHandlerIdMapping — asserting the inserted row's discoveryHandlerId equals the original — would pin that behavior and prevent a future regression to .get(...).

Minor (out of scope for #6465): the 2-arg importData (~L250-290) has the same multi-insert loop without @Transactional, so a mid-loop failure leaves partial inserts. Not addressed here, but the asymmetry is worth a note.

[Aias00]

Summary

This PR fixes duplicate detection during discovery-upstream import when handler ids are remapped by ConfigsImportContext.discoveryHandlerIdMapping.

What is correct

• The existing-url lookup now keys off the remapped handler id: discoveryHandlerIdMapping.getOrDefault(discoveryHandlerId, discoveryHandlerId) instead of the raw discoveryHandlerId. This makes the duplicate check consistent with the id that is actually persisted, so an incoming upstream whose URL already exists under the target (mapped) handler is correctly treated as a duplicate.
• The insert no longer produces a null handler id: setDiscoveryHandlerId(... .getOrDefault(..., original)) replaces the previous get(...) which could return null for unmapped handlers.
• @Transactional(rollbackFor = Exception.class) on the importData(namespace, list, context) overload gives the multi-row import atomicity.
• Tests testImportDataWithNamespaceAndContext and testImportDataWithNamespaceUnmappedHandlerId cover both the mapped and unmapped paths and assert the duplicate is rejected (successCount == 0).

Verification

• ConfigImportResult is asserted for both success and duplicate scenarios; the mock selectByNamespaceId returns rows keyed by the remapped id, exercising exactly the fixed path.

Non-blocking suggestions

• The single-arg importData(List) overload is still non-transactional. Consider annotating it too for consistency, or have it delegate to the 3-arg overload so both share the same atomicity guarantee.

Conclusion

Approved. The change is a clear correctness fix with covering tests and low risk.

gh pr review 6532 --repo apache/shenyu --approve --body-file review_6532.md