fix(query): honor per-operation useMutation override for GET operations#3360
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThis PR fixes a bug where per-operation ChangesPer-operation GET-to-mutation routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
A per-operation `override.operations.<id>.query.useMutation: true` was silently discarded for GET operations: the operation stayed a Query hook instead of becoming a Mutation hook. This was the inverse asymmetry of the per-operation `useQuery: true` path, which already worked for non-GET verbs. `effectiveUseMutation` already encodes the per-verb default (`verb !== Verbs.GET`), so the extra `&& verb !== Verbs.GET` gate on `isMutation` only ever suppressed an explicit opt-in. Drop the gate so `isMutation` honors explicit intent for every verb, mirroring how `isQuery` honors `useQuery: true` for non-GET verbs. Closes orval-labs#3358
7c111d1 to
2e7a08c
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage and documentation for honoring useMutation: true overrides on GET operations in the React Query client generator.
Changes:
- Adds a new test fixture/config and snapshots validating per-operation
useMutation: trueon aGETendpoint. - Updates query hook generation to honor explicit
useMutationforGETverbs. - Clarifies documentation for
useMutationbehavior and precedence rules.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/configs/react-query.config.ts | Adds a new test config entry to generate snapshots for the useMutation-on-GET override scenario. |
| packages/query/src/query-generator.ts | Removes the GET verb gate so explicit useMutation: true is honored for GET operations. |
| docs/content/docs/reference/configuration/output.mdx | Documents that explicit useMutation applies regardless of HTTP verb and describes precedence. |
| tests/snapshots/react-query/per-operation-use-mutation-on-get/** | Adds generated snapshot outputs verifying the new behavior. |
Comments suppressed due to low confidence (1)
tests/snapshots/react-query/per-operation-use-mutation-on-get/model/listPetsParams.ts:1
- The generated JSDoc block has malformed formatting (missing
*alignment on lines 15–17), which degrades the readability of generated typings and can render poorly in editors/TS tooling. Fixing this likely requires normalizing multi-line description rendering in the generator so each line is prefixed with*(and avoiding stray blank lines).
/**
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Closes #3358 — a per-operation `override.operations.<id>.query. | ||
| // useMutation: true` now routes a GET operation to a Mutation hook. | ||
| // Previously the verb gate in `query-generator.ts` discarded explicit | ||
| // `useMutation: true` for GET, so the operation stayed a Query hook — | ||
| // the inverse asymmetry of the per-operation `useQuery: true` path | ||
| // that already worked for non-GET verbs. |
There was a problem hiding this comment.
Thanks for the catch! This is a // source-code comment rather than rendered Markdown, so there's no inline code-span rendering involved here. I deliberately kept the wrapping consistent with the sibling comment ~30 lines above — the perOperationFalseDisablesGlobalTrue block added in #3323 — which wraps override.operations.<id>.query.useQuery: false across a line break in exactly the same way. Reflowing only this block would make it inconsistent with that sibling, and normalizing both is out of scope for this fix. Leaving it as-is for consistency.
|
Hi @melloware, would you be able to review this when you have a moment? Thanks! |
|
Sure will review tomorrow |
Summary
Closes #3358.
A per-operation
override.operations.<id>.query.useMutation: truewas silently discarded forGEToperations — the operation stayed a Query hook instead of becoming a Mutation hook. This is the inverse asymmetry of the per-operationuseQuery: truepath, which already routes non-GEToperations to Query hooks.search(POST → Query) works;reprint(GET → Mutation) does not.Root cause
packages/query/src/query-generator.ts:effectiveUseMutationalready resolves precedence and encodes the per-verb default (operationQueryOptions?.useMutation ?? override.query.useMutation ?? verb !== Verbs.GET). The trailing&& verb !== Verbs.GETtherefore only ever suppressed an explicit opt-in: an unsetGETop already resolves tofalse, so the gate changed nothing for the default case and instead discardeduseMutation: true.The
isQuerybranch has no such verb gate, which is whyuseQuery: truealready works for any verb. The conflict-resolution block right below —if (verb === Verbs.GET && isMutation) { isQuery = false; }— was unreachable dead code because of this gate, confirming thatGET→ Mutation was the intended design.Fix
Drop the verb gate so
isMutationhonors explicit intent for every verb, mirroringisQuery:Behavior matrix:
GEToperationeffectiveUseMutationresolves tofalse)useMutation: trueuseMutation: trueNon-
GEToperations are unaffected:effectiveUseMutationstill defaults totruefor them.generateMutationHookdoes not assume a non-GETverb, so aGET-verb Mutation hook generates valid code (same shape as a param-onlyDELETE); the new snapshot type-checks viaorval-tests build.Breaking change
Lifting the gate also means a global
override.query.useMutation: truenow routesGEToperations to Mutation hooks — symmetric with how globaluseQuery: truealready routes non-GEToperations to Query hooks (the explicit-intent-propagates-to-every-verb philosophy of #3323). No existing test or sample setsuseMutation: trueglobally (the onlyuseMutationconfig in the repo isuseMutation: false), so no existing snapshot changes. Abreaking changelabel is likely appropriate, consistent with #3323.Test plan
perOperationUseMutationOnGet—showPetById(GET /pets/{petId}) with per-operationuseMutation: truenow emitsuseShowPetById/getShowPetByIdMutationOptions; other operations (GET listPets,POST createPets,DELETE deletePetById) are unchanged.bun run build/typecheck/lint/test/test:snapshots/format:check/bun run --filter orval-tests buildall pass locally.showPetByIdgenerated a Query hook (bug reproduced), post-fix a Mutation hook.Documentation
docs/content/docs/reference/configuration/output.mdx— theuseMutationsection now documents that an explicituseMutation: true(global or per-operation) applies to all HTTP verbs, including routing aGEToperation to a Mutation hook, mirroring the existing wording foruseQuery.Summary by CodeRabbit
Documentation
override.query.useMutationconfiguration documentation with clarification on default behavior, per-operation routing for GET requests, and conflict-resolution rules when both query and mutation are enabledImprovements