dengliming commented on PR #7067: URL: https://github.com/apache/shenyu/pull/7067#issuecomment-5709706770
This change introduces a behavior change beyond thread-safety: `ConcurrentHashMap` does not allow `null` keys, while the previous `HashMap` did. In the current code path, `AiResponseTransformerPlugin#doExecute` reads/writes the cache with `rule.getId()`, and the failing CI job already shows this case is reachable: job `105087971904` failed in `AiResponseTransformerPluginTest.testDoExecute` with a `NullPointerException` at line 144 after this branch switched the cache implementation. Could you please either: 1. guard the cache access with a non-null key (for example, validate `rule.getId()` and fall back to a default key or skip caching), and/or 2. update the affected tests to reflect the new contract explicitly? For reference, the failing CI job is: - https://github.com/apache/shenyu/actions/runs/95286805343/job/105087971904 A concrete shape would be something like: ```java String cacheKey = Objects.nonNull(rule) && Objects.nonNull(rule.getId()) ? rule.getId() : "default"; ``` and then consistently use `cacheKey` for `getClient(...)` / `init(...)`. The same concern likely applies to the request-transformer side too, since this PR changes both cache implementations. -- 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]
