Review: #7984 fix(plugin): cache negative match results only when the candidate set is path-determined (#7977)

Verdict: approved. The diagnosis is right, the guard is placed at the right two call sites, and the tests are the correct shape — two red cases and two positive controls.

The bug is real and the fix addresses it

MatchDataCache keys negative entries by (pluginName, path) only. defaultMatchSelector / defaultMatchRule wrote those entries on any miss, including a miss caused purely by a missing header, so a single header-less request poisoned the path for the whole TTL. The new gates are:

• isPathDeterminedSelectorSet — skips disabled selectors (matching already ignores them), rejects matchRestful, and rejects CUSTOM_FLOW selectors whose conditions are not all uri. Non-CUSTOM_FLOW selectors are correctly not condition-checked, since FULL_FLOW ignores conditions.
• isPathDeterminedRuleSet — rejects matchRestful and any non-uri condition.
• The rule path additionally requires the whole selector set to be path-determined. That is the part that is easy to miss and it is the part that matters: rules here comes from BaseDataCache.obtainRuleData(selectorData.getId()), i.e. the rules of one selector, while the cache key carries no selector id — so a negative rule entry written while selector matching is dynamic would also suppress a different selector on the same path. negativeRuleCacheMustRequirePathDeterminedSelectors pins exactly that, and it is the test I would have asked for.

Treating an empty condition list as path-determined is sound in both directions: a CUSTOM_FLOW selector with no conditions never matches (so a negative entry is stable), and empty rule conditions are request-independent. Either way no wrong entry can be produced.

Positive caching is untouched, which is correct — this PR should not absorb #7981.

Verified

• defaultMatchRule now receives the plugin's full selector list. In execute the local selectors at the call site is the same list used by defaultMatchSelector, and rules is the selected selector's rule list, so both predicates see the sets they claim to.
• MatchDataCache.cleanRuleDataData() exists (MatchDataCache:248), so the added teardown in clearCache compiles and the singleton caches are actually reset between tests.
• The two positive controls (negativeSelectorCacheStillAppliesToPathOnlyCandidates, negativeRuleCacheStillAppliesToPathOnlyCandidates) assert a cached entry with a null id, so the optimisation is proven to still fire rather than merely not-crash.

Non-blocking

1. Cost on the miss path. isPathDeterminedSelectorSet walks every selector for the plugin and isPathDeterminedRuleSet every rule of the selected selector, on each miss that reaches the cache write. That is cheap next to matching, but it runs on the hot path for plugins with many selectors; if this shows up in profiles, memoising the predicate per BaseDataCache generation would remove it entirely.
2. Old entries survive the upgrade. Entries written before this change stay in MatchDataCache until their TTL expires (60 s by default), so the bug is still observable for one TTL after a rolling restart. Worth one line in the PR description so nobody reports it as "the fix does not work".
3. isUriOnlyConditionList relies on URI_CONDITION_TYPE.equals(condition.getParamType()). A null paramType is treated as non-uri, which fails safe (no negative cache) — fine, just noting the direction.

Approving — this now covers what I asked for, and does it in the right place.

Verified in detail:

• Positive and negative entries are gated for both the selector and the rule cache, so #7981's positive-cache variant is covered here and not only #7977's negative one.
• defaultMatchRule takes the selector set and requires both sets to be path-determined, which is necessary because the rule cache key carries no selector id.
• Objects.equals(SelectorTypeEnum.CUSTOM_FLOW.getCode(), selector.getType()) is safe: getCode() returns a primitive int, so the comparison unboxes.
• MatchDataCache.cleanRuleDataData() exists on master, and adding cleanSelectorData() / cleanRuleDataData() to clearCache() is a genuine isolation fix — without it the new rule tests could read an entry left behind by an earlier test.
• An empty condition list is correctly treated as cacheable: for a CUSTOM_FLOW selector the match outcome is request-independent either way, and for FULL_FLOW the condition list is not consulted at all.
• Non-CUSTOM_FLOW selectors are correctly exempt from the condition check.

Two follow-ups:

1. The matchRestful branch you added has no test. #7990 (the duplicate PR against #7981) has two parameterized tests that fit it exactly — restfulSelectorCandidateShouldPreventResultCaching and restfulRuleCandidateShouldPreventResultCaching. Please port them so the new branch is pinned.

2. #7984 and #7990 are now the same change. Both touch the same four call sites in AbstractShenyuPlugin and are functionally equivalent, so they conflict textually and, worse, one could land on top of the other's gate. I recommend landing this one — it has the stronger positive/negative control coverage — and closing #7990; I am leaving that comment there now.

Nit (shared with #7990): selector.getType() is a boxed Integer, and a row with a null type is treated as non-CUSTOM_FLOW, which skips the condition gate entirely. If type can be null in any persisted data, make the guard null-safe (or treat null as CUSTOM_FLOW); otherwise a one-line comment stating that type is never null would be enough.

CI is currently all queued on this branch, so nothing to judge yet.

Re-approving: the two tests requested in #7990 are now ported and the gate is complete.

Verified:

• isPathDeterminedSelectorSet skips disabled selectors, rejects matchRestful and non-URI conditions on CUSTOM_FLOW selectors. Objects.equals(int, Integer) boxes the left side, so the comparison is safe.
• The rule cache key still carries no selector id, which is why gating on both the selector set and the rule set is required; that reasoning is correct and documented in the comment.
• verify(..., never()).getConditionList() on the second pass proves the cache really short-circuits matching, and the two @ParameterizedTest cases pin the RESTful gating for both hit and miss.
• Empty condition list is treated as path-determined, which is right: such a selector matches every path, so a per-path entry stays consistent.

Note: the gate now walks the whole candidate set on every cache write (only on a miss), which is acceptable but worth keeping in mind if selector counts grow.

Requesting changes. The intent - do not cache positive matches whose candidate set is not path-determined - is correct, but isUriOnlyConditionList() only checks paramType == uri and does not inspect the operator. URI time predicates such as TimeAfter/TimeBefore have paramType == uri yet their match result depends on the current time, so this PR would still cache a positive result for a selector/rule carrying a time predicate and shadow the competing candidate as time advances. #8106 already implements the correct fix (a PATH_ONLY_OPERATORS whitelist that excludes TimeAfter/TimeBefore and header/query conditions) with comprehensive tests. Please either adopt that operator-allowlist approach here, or close this PR in favor of #8106. Happy to help align the implementation.

Re-reviewed after my earlier operator-eligibility change request. The current negative-cache logic uses the path-only operator allowlist, checks all enabled selector candidates, and additionally checks enabled rule candidates before caching a rule miss. TimeAfter/TimeBefore and dynamic header/query conditions now have regressions; path-only misses retain caching. No remaining blocking finding in this patch.

NegativeMatchCacheEligibilityTest and AbstractShenyuPluginTest passed all 30 tests, zero failures/errors/skips. Validation baseline: PR head plus a clean, uncommitted merge of master 52339b2 in the isolated worktree. No source was manually patched or pushed.

Used isolated worktrees and the Maven wrapper with JDK 21/project compiler settings. Checkstyle passed for the targeted Maven runs; RAT and JaCoCo were skipped. No full reactor/e2e pass is claimed.