hengyuss commented on PR #7067: URL: https://github.com/apache/shenyu/pull/7067#issuecomment-5711550358
> 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.这一变化引入了超越线程安全的行为变化： `ConcurrentHashMap` 不允许 `null` 键，而之前的 `HashMap` 可以。在当前的代码路径中， `AiResponseTransformerPlugin#doExecute` 用 `rule.getId()` 读写缓存，失败的 CI 作业已经显示该情况是可达的：作业 `105087971904` 在该分支切换缓存实现后，在第 144 行以 `NullPointerException` 在 `AiResponseTransformerPluginTest.testDoExecute` 中失败。 > > 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用非空键保护缓存访问（例如，验证 `rule.getId()` 并退回默认键或跳过缓存），和/或 > 2. update the affected tests to reflect the new contract explicitly?更新受影响的测试，明确反映新合同？ > > For reference, the failing CI job is:供参考，失败的 CI 任务是： > > * 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(...)`.然后始终使用 `cacheKey` 表示 `getClient(...)` / `init(...)` 。 > > The same concern likely applies to the request-transformer side too, since this PR changes both cache implementations.同样的担忧很可能也适用于请求变换器端，因为这个 PR 会改变两个缓存的实现。 I think the error cause by the test call doExcute() directly with the rule has null id. We should just fix the test rather than modify the business code. Because the code logic is user want use the configuration configured by rule not default. If the null id actually enters the business code, we should throw the exception clearly rather than back to default without the user being aware of it. what do you think? -- 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]
