fix(mock): use operationName for MSW handler names under splitByContentType - #3566
Conversation
…ntType When splitByContentType splits a multi content-type requestBody into sibling verb options, those siblings share one operationId but carry distinct suffixed operationNames (e.g. *WithJson / *WithFormData). The client side already names functions from operationName, but generateMSW derived the handler and responseMock names from operationId, so both variants emitted identical names and the generated mock file failed to compile with TS2451 (redeclared block-scoped variable). Derive the MSW handler/responseMock name bases from operationName so they match the client split. In the non-split case operationName normalizes to the same pascal-cased string as operationId, so existing output is unchanged. Closes orval-labs#3342
📝 WalkthroughWalkthroughThis PR fixes duplicate MSW handler exports when using ChangesMSW Handler Naming Fix for splitByContentType
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression test fixture to ensure MSW mock generation produces distinct handler/response-mock names when splitByContentType is enabled, preventing duplicate declarations during TypeScript compilation.
Changes:
- Update MSW name derivation to use
operationName(content-type-suffixed) instead ofoperationId. - Add a new test config entry and snapshot outputs for issue #3342.
- Extend MSW unit tests to assert naming behavior for
splitByContentTypevariants.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/configs/mock.config.ts | Adds a dedicated config case to generate mocks for the split-by-content-type regression. |
| tests/snapshots/mock/issue-3342/endpoints.ts | New snapshot proving generated handlers/mocks are uniquely named per content type variant. |
| tests/snapshots/mock/issue-3342/model/*.ts | New snapshot models for the issue #3342 generated output. |
| packages/mock/src/msw/index.ts | Switches MSW handler/response-mock naming from operationId to operationName. |
| packages/mock/src/msw/index.test.ts | Adds coverage to ensure naming derives from operationName for split-by-content-type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): ClientMockGeneratorBuilder { | ||
| const { pathRoute, override, mock } = generatorOptions; | ||
| const { operationId, response } = generatorVerbOptions; | ||
| const { operationName, response } = generatorVerbOptions; |
There was a problem hiding this comment.
operationName is a required field on GeneratorVerbOptions (packages/core/src/types.ts), not optional, and buildVerbOption always populates it in the generation pipeline. Both production call sites — packages/mock/src/index.ts and packages/mock/src/faker/index.ts — pass pipeline-built verb options, so it's never undefined here. The client generators already derive their function names from operationName; this change only aligns the MSW side with them. A ?? operationId fallback would guard a state the type system already prevents.
Minor: pascal(undefined) returns '' via its default parameter, so a missing name would yield getMockHandler, not getUndefinedMockHandler.
| // Derive names from operationName (not operationId): splitByContentType keeps | ||
| // one operationId across variants but suffixes operationName (e.g. *WithJson / | ||
| // *WithFormData), and the client side already names functions from it. Using | ||
| // operationId here would emit duplicate handler names and break tsc. See #3342. | ||
| const handlerName = `get${pascal(operationName)}MockHandler`; | ||
| const getResponseMockFunctionName = `get${pascal(operationName)}ResponseMock`; |
There was a problem hiding this comment.
Same as the line 438 thread: operationName is a required field on GeneratorVerbOptions and is always populated by buildVerbOption in the pipeline, so no fallback is needed here. This only mirrors how the client generators already name their functions.
| export interface Error { | ||
| code: number; | ||
| message: string; | ||
| } |
There was a problem hiding this comment.
The Error schema comes from the existing shared test spec tests/specifications/split-by-content-type.yaml, which is also consumed by the split-by-content-type config in tests/configs/default.config.ts (the original #3201 test) and already emits this Error type there. Renaming the schema would churn an unrelated committed snapshot and is out of scope for this fix — it's a generated test fixture, not shipped code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/mock/src/msw/index.test.ts (1)
1500-1526: 💤 Low valueConsider moving this test to its own describe block.
This test verifies the fix for issue
#3342(splitByContentType naming), but it's nested insidedescribe('strict mock types (#3525)'). For better discoverability, consider moving it to a separatedescribe('splitByContentType handler naming (#3342)')block or into the maindescribe('generateMSW')suite.The test implementation itself is well-structured and correctly verifies that handler and response mock names are derived from
operationNamerather thanoperationId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mock/src/msw/index.test.ts` around lines 1500 - 1526, Move the specific it('derives responseMock and handler names from operationName (splitByContentType)') test out of the nested describe('strict mock types (`#3525`)') block into its own describe block (e.g., describe('splitByContentType handler naming (`#3342`)')) or into the top-level describe('generateMSW') suite so it’s discoverable; locate the test that calls generateMSW with operationId 'getPet' and operationName 'getPetWithFormData' and wrap that it(...) in the new describe, preserving the assertions that reference result.implementation.handlerName and result.implementation.function/handler to ensure the same checks remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/mock/src/msw/index.test.ts`:
- Around line 1500-1526: Move the specific it('derives responseMock and handler
names from operationName (splitByContentType)') test out of the nested
describe('strict mock types (`#3525`)') block into its own describe block (e.g.,
describe('splitByContentType handler naming (`#3342`)')) or into the top-level
describe('generateMSW') suite so it’s discoverable; locate the test that calls
generateMSW with operationId 'getPet' and operationName 'getPetWithFormData' and
wrap that it(...) in the new describe, preserving the assertions that reference
result.implementation.handlerName and result.implementation.function/handler to
ensure the same checks remain intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bce3002b-ae1a-4559-b57a-e45b39f84080
📒 Files selected for processing (10)
packages/mock/src/msw/index.test.tspackages/mock/src/msw/index.tstests/__snapshots__/mock/issue-3342/endpoints.tstests/__snapshots__/mock/issue-3342/model/avatar.tstests/__snapshots__/mock/issue-3342/model/avatarUpload.tstests/__snapshots__/mock/issue-3342/model/error.tstests/__snapshots__/mock/issue-3342/model/index.tstests/__snapshots__/mock/issue-3342/model/profile.tstests/__snapshots__/mock/issue-3342/model/updateProfileBody.tstests/configs/mock.config.ts
Summary
When an endpoint's
requestBodydeclares multiple content types (e.g.application/json+multipart/form-data) andsplitByContentType: trueis set, the client is correctly split into*WithJson/*WithFormDatafunctions (#3201), but the generated MSW mocks emit two handlers with identical names, so the file fails to compile:Closes #3342.
Root cause
splitByContentTypekeeps a singleoperationIdacross the sibling verb options but suffixes each one'soperationName(*WithJson/*WithFormData). The client side already names functions fromoperationName, butgenerateMSWderived the handler and responseMock name bases fromoperationId:So both variants collapsed to the same name. #3201 fixed the client side and missed the MSW side.
Fix
Derive the MSW handler/responseMock name bases from
operationNameso they match the client split. In the non-split caseoperationNamenormalizes to the same pascal-cased string asoperationId, so existing output is unchanged.generateDefinition's use ofoperationId(override-mock lookup key) is intentionally left untouched.Tests
packages/mock/src/msw/index.test.ts): a new case asserts that a suffixedoperationNameproducesget<Name>WithFormDataMockHandler/...ResponseMock. Existing fixtures gained the (previously omitted)operationNamefield, which equals theiroperationId, so their assertions are unchanged.tests/configs/mock.config.ts→issue3342): reuses the existingsplit-by-content-type.yamlspec withreact-query+ MSW mock +splitByContentType. The generated output is committed and goes throughtypecheck-generated, which compiles every generated client — so a regression of this collision would fail CI with TS2451 directly.Verification
@orval/mockand@orval/coreunit tests pass.typecheck-generated(all clients) and the full snapshot suite (samples + tests) pass.issue-3342fixtures are added).Note
Two handlers are now registered for the same route (one per content type). Their mocked responses are identical, so MSW uses the first match and the second is effectively redundant but harmless — this matches the reporter's expectation of distinct, split handler names. De-duplicating to a single shared handler is out of scope and could be a follow-up.
Summary by CodeRabbit
Bug Fixes
Tests