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.