Repository navigation
[ISSUE #7452] skip caching when cache rule handle is null - #7541
Conversation
CachePluginDataHandler skips rules whose handle is null, so CachePlugin could not find a CacheRuleHandle for such a rule and CacheHttpResponse threw a NullPointerException on getTimeoutSeconds() while writing an otherwise successful downstream response. When no rule handle is cached, CachePlugin now passes the request to the rest of the chain without reading or writing the cache, the same way the Hystrix, Resilience4J, Sentinel and RateLimiter plugins handle a missing handle (apache#6877). Add regression tests for a rule without a handle and for a rule with a handle. Fixes apache#7452
Aias00
left a comment
There was a problem hiding this comment.
Review: skip caching when cache rule handle is null
Clean, minimal fix. CachePlugin.doExecute now builds the rule handle once up front and short-circuits to chain.execute(exchange) when the cache rule handle is null, instead of passing a null CacheRuleHandle into CacheHttpResponse (which would NPE / misbehave). It also removes the now-duplicated buildRuleHandle(rule) calls that previously appeared in two branches.
Verification
buildRuleHandlereturnsCACHED_HANDLE.get().obtainHandle(CacheKeyUtils.INST.getKey(rule)), which isnullwhen the rule has no configured handle — so the original code could passnullintoCacheHttpResponse. The new null guard is the correct fix.java.util.Objectswas already imported (used elsewhere in the file), so the addedObjects.isNull(...)compiles.- Tests added for both paths:
testDoExecuteSkipsCacheWhenRuleHandleIsNull(asserts body flows through unchanged and nothing is written to the cache) andtestDoExecuteCachesResponseWhenRuleHandleIsPresent(asserts the response is cached).
CI note
No checks are reported on the fork branch fix/issue-7452 — the fork hasn't triggered CI, so there is no CI evidence for this change. The modification is single-module (shenyu-plugin-cache-handler), self-contained, and well-tested, so risk is low, but please confirm pr_build is green before merging.
Approving — the fix is correct and the added tests cover the regression.
|
if you are using wechat, pls add mine: aias00 |
Fixes #7452
Problem
CachePluginDataHandler.handlerRule()skips a rule whosehandleis null, so noCacheRuleHandleis cached for it. When such a rule matches,CachePluginstill wraps the response inCacheHttpResponsewith a null handle, andcacheResponse()throws aNullPointerExceptiononcacheRuleHandle.getTimeoutSeconds()while writing an otherwise successful downstream response.Fix
CachePlugin.doExecute()now looks up the rule handle first. If none is cached, it passes the request to the rest of the chain without reading or writing the cache. This is how the Hystrix, Resilience4J, Sentinel and RateLimiter plugins handle a missing handle since #6877. No default timeout is applied. Rules with a handle behave as before. The handle is now looked up once instead of in both branches.Tests
Added two tests to
CachePluginTest. In both, the downstream chain writes a real response body:testDoExecuteSkipsCacheWhenRuleHandleIsNull: a rule with no cached handle completes without error, returns the downstream body, and writes nothing to the cache. Without the fix, this test fails with the NPE from the issue.testDoExecuteCachesResponseWhenRuleHandleIsPresent: a rule with a handle still returns the body and caches it.mvn -pl shenyu-plugin/shenyu-plugin-cache/shenyu-plugin-cache-handler -am testpasses locally, checkstyle included.Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.