Skip to content

Skip the same-URI refresh instead of throwing on a mismatched onPost - #220

Merged
koriym merged 3 commits into
bearsunday:1.xfrom
koriym:fix/219-unmatched-query-uncaught
Sep 16, 2026
Merged

koriym merged 3 commits into
bearsunday:1.xfrom
koriym:fix/219-unmatched-query-uncaught

Conversation

@koriym

@koriym koriym commented Sep 16, 2026 •

Copy link
Copy Markdown
Member

Closes #219.

#214 bound CommandInterceptor to onPost, so RefreshSameCommand's automatic same-URI refresh now runs MatchQuery against every write's own URI query - including a create whose parameters do not cover onGet's required ones (a collection onPost with no id yet). MatchQuery's UnmatchedQuery propagated uncaught through CommandInterceptor's try/finally (no catch), turning a request that used to reach onPost with no interceptor at all into a hard failure - and because CommandsProvider orders RefreshSameCommand before RefreshAnnotatedCommand, an explicit #[Refresh]/#[Purge] on the same method never ran either. #214 turned "the attribute is silently dropped" (#212) into "the attribute is silently dropped and the write 500s".

The identical bug existed on the donut command path: DonutCommandInterceptor::refreshDonutAndState() called MatchQuery with no catch either, inside an invoke() whose try is finally-only, and DonutCacheModule binds this interceptor to onPost too - so a #[CacheableResponse]/#[DonutCache] collection onPost with the same shape 500ed as well.

Both interceptors now catch UnmatchedQuery, scoped to onPost-prefixed methods (str_starts_with, matching the AOP matcher's own startsWith semantics - the same prefix-matched names like onPostItem the matcher intercepts): the write already succeeded, there is no entry to refresh, and the skip is recorded as cache_error{operation: write} rather than vanishing silently. RefreshSameCommand returning instead of throwing also lets CommandsProvider's next command run, so an explicit #[Refresh]/#[Purge] on the same onPost still fires.

onPut/onPatch/onDelete keep throwing on both interceptors: those act on an entity onGet already addresses, so a required parameter missing there is a real signature mismatch, not this case - BehaviorTest::testUnMatchQuery (value-cache side) pins that, and DonutUnmatchedQueryRefreshTest::testPutStillThrowsForAGenuineMismatch pins the same contract on the donut side.

Verification

  • New fixtures MismatchedWriter (value-cache, onGet(int $id) / onPost(string $title)) and MismatchedDonutWriter (#[CacheableResponse], same shape plus an onPut twin for the still-throws case), with tests/UnmatchedQueryRefreshTest.php and tests/DonutUnmatchedQueryRefreshTest.php: the onPost write still succeeds (no throw) and is logged as cache_error; an explicit #[Purge] on the same onPost still runs (value-cache side); onPut with a genuine mismatch still throws UnmatchedQuery on both interceptors.
  • Confirmed both new test files fail pre-fix with exactly the reported UnmatchedQuery propagating uncaught (reverted each source file locally via git stash, re-ran, reverted back).
  • BehaviorTest::testUnMatchQuery (the pre-existing onPut case) passes unmodified.
  • 100% line coverage on both changed interceptors; full suite (394 tests), cs, phpstan, psalm all clean; full CI matrix (PHP 8.2-8.5, 3 OSes) and codecov green.

Also: both interceptors' docblocks said onPut/onPatch/onDelete only, stale since #214 added onPost.

bearsunday#214 bound CommandInterceptor to onPost, so RefreshSameCommand's automatic
refresh now runs MatchQuery against every write's own URI query - including
a create whose parameters do not cover onGet's required ones (a collection
onPost with no id yet). MatchQuery's UnmatchedQuery propagated uncaught
through CommandInterceptor's try/finally (no catch), turning a request that
used to reach onPost with no interceptor at all into a hard failure.

RefreshSameCommand now catches UnmatchedQuery, but only for onPost: the
write already succeeded, there is no entry to refresh, and the skip is
recorded as cache_error{operation: write} rather than vanishing silently.
Returning instead of throwing lets CommandsProvider's next command run, so
an explicit #[Refresh]/#[Purge] on the same onPost still fires.

onPut/onPatch/onDelete keep throwing: those act on an entity onGet already
addresses, so a required parameter missing there is a real signature
mismatch, not this case - BehaviorTest::testUnMatchQuery pins that and
would have caught a fix that swallowed it too.

Fixes bearsunday#219. Also: CommandInterceptor's docblock said onPut/onPatch/onDelete
only, stale since bearsunday#214 added onPost.
@coderabbitai

coderabbitai Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 31a05289-0e9d-4b43-9709-4b9c049fc957

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (fe78fe0) to head (2ab9a92).

Additional details and impacted files
@@             Coverage Diff             @@
##                 1.x      #220   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       545       549    +4     
===========================================
  Files            100       100           
  Lines           1464      1472    +8     
===========================================
+ Hits            1464      1472    +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Kimi K3's review of this PR found the identical bug in
DonutCommandInterceptor::refreshDonutAndState(): MatchQuery called with
no catch, inside invoke()'s try/finally-only block - DonutCacheModule
binds this interceptor to onPost too (commandMethods()), so a
#[CacheableResponse]/#[DonutCache] collection onPost with a parameter
mismatch threw uncaught the same way RefreshSameCommand did.

Same fix, same scoping: catch UnmatchedQuery, but only when the
invoked method name starts with 'onPost' (str_starts_with, matching
the AOP matcher's own startsWith semantics - RefreshSameCommand's
exact-match check missed prefix-matched names like onPostItem, also
flagged in review). onPut/onPatch/onDelete keep throwing on both
interceptors.

New fixture (MismatchedDonutWriter, #[CacheableResponse]) and
DonutUnmatchedQueryRefreshTest mirror UnmatchedQueryRefreshTest for
this path. Confirmed failing pre-fix via git stash on
DonutCommandInterceptor.php alone (same UnmatchedQuery, same
uncaught propagation through DonutCommandInterceptor.php:67).
codecov flagged DonutCommandInterceptor.php:69 (the re-throw for a
non-onPost mismatch) as untested - the donut-side equivalent of
BehaviorTest::testUnMatchQuery had no test. MismatchedDonutWriter
gains an onPut with the same signature mismatch as its onPost;
testPutStillThrowsForAGenuineMismatch pins that it still throws.
@koriym
koriym merged commit a0072d8 into bearsunday:1.x Sep 16, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

onPost on a #[Cacheable] class with args not matching onGet now throws UnmatchedQuery uncaught

1 participant