Aias00 commented on PR #6532: URL: https://github.com/apache/shenyu/pull/6532#issuecomment-5193353539
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. -- 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]
