fix(mock): strict faker schema mock types for enums, nested spreads, and binary - #3607
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughFixes three TypeScript errors in strict faker schema mock generation (issue ChangesStrict mock schema kind tracking, import dedup, and enum/binary type fixes (issue
Sequence Diagram(s)sequenceDiagram
participant generateFakerForSchemas
participant getMockScalar
participant classifyStrictMockSchemaType
participant appendImportsDelta
participant mergeStrictMockSchemaKinds
generateFakerForSchemas->>getMockScalar: resolve schema (with seeded allOf refs)
getMockScalar-->>generateFakerForSchemas: implementation + imports
generateFakerForSchemas->>appendImportsDelta: append new imports since last snapshot
generateFakerForSchemas->>classifyStrictMockSchemaType: classify schema as object/alias/binary
classifyStrictMockSchemaType-->>generateFakerForSchemas: StrictMockSchemaKind
generateFakerForSchemas->>generateFakerForSchemas: accumulate strictMockSchemaKinds[typeName]
generateFakerForSchemas->>mergeStrictMockSchemaKinds: merge accumulated kinds
mergeStrictMockSchemaKinds-->>generateFakerForSchemas: merged Record<string, StrictMockSchemaKind>
generateFakerForSchemas-->>writeFakerSchemaMocks: implementation + strictMockSchemaKinds
sequenceDiagram
participant WriteModeWriter as writeSplitMode / writeSplitTagsMode / writeTagsMode
participant getFinalizeMockImplementationOptions
participant collectRecoveredSchemaFactoryImports
participant mergeGeneratorImports
participant filterLocalStrictMockTypeImports
participant generateImportsForBuilder
WriteModeWriter->>getFinalizeMockImplementationOptions: mockOutput[]
getFinalizeMockImplementationOptions-->>WriteModeWriter: finalizeMockOptions (strictSchemaTypeNames, strictMockSchemaKinds)
WriteModeWriter->>collectRecoveredSchemaFactoryImports: finalizedImplementation + componentSchemaNames
collectRecoveredSchemaFactoryImports-->>WriteModeWriter: recoveredSchemaFactoryImports[]
WriteModeWriter->>mergeGeneratorImports: mockOutput.imports + recoveredSchemaFactoryImports
mergeGeneratorImports-->>WriteModeWriter: merged imports[]
WriteModeWriter->>filterLocalStrictMockTypeImports: merged imports + strictSchemaTypeNames
filterLocalStrictMockTypeImports-->>WriteModeWriter: filtered imports[]
WriteModeWriter->>generateImportsForBuilder: filtered imports
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 |
…and binary
Classify strict {Schema}Mock aliases by schema shape.
Cast nested factory spreads to base mock types.
Add petstore regression spec for #3590.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Import {Schema}Mock from index.faker when operation fakers spread schema
factories with strict casts. Skip imports that duplicate local response mocks.
Co-authored-by: Cursor <cursoragent@cursor.com>
…ports (#3590) Use appendImportsDelta to avoid stack overflows with schemas: true. Recover missing get*Mock imports in tags-split faker files. Guard MSW generation when response.imports is undefined. Add pet-themed regression specs and tests for each failure mode. Co-authored-by: Cursor <cursoragent@cursor.com>
Use OpenApiSchemaObject and createTestContextSpec override options. Cast undefined response imports via unknown for the runtime guard test. Co-authored-by: Cursor <cursoragent@cursor.com>
Merge getMockScalar result imports in generateFakerForSchemas. Collect nested oneOf split helper types and filter local strict mock imports in single mode. Co-authored-by: Cursor <cursoragent@cursor.com>
PetDetailSettingsItem is referenced by strict mock factories and must be imported in index.faker.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
09a69d0 to
f1dbbe2
Compare
|
I'm going for a full AI approach here... I'll let it make a local build of Orval and check again against all our yaml files. I'm not sure how we'll do the review process here... @wadakatu what do you think? |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/mock/src/faker/imports.test.ts (1)
67-79: ⚡ Quick winAdd a strict-signature fixture for
collectSplitMockTypeImports.Current coverage validates only the non-strict signature form. Please add one strict generic factory case (
<O extends Partial<T>...>(overrideResponse?: O): MockWithNullableOverrides<...>) to lock in strict-mode import recovery behavior.🤖 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/faker/imports.test.ts` around lines 67 - 79, The test suite for collectSplitMockTypeImports currently only validates the non-strict signature form. Add an additional test case (another it() block within the describe('collectSplitMockTypeImports') block) that covers the strict generic factory signature pattern. This new test should create an implementation string using the strict-signature form with a generic type parameter constraint (like <O extends Partial<T>), an optional overrideResponse parameter, and a return type using MockWithNullableOverrides, then verify that collectSplitMockTypeImports correctly extracts all necessary type imports from this pattern.
🤖 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.
Inline comments:
In `@packages/core/src/writers/split-mode.ts`:
- Around line 17-22: The merged imports are being emitted directly after
recovery without filtering for local strict mock types, which allows split mock
files to import Schema Mock types declared locally, causing duplicate identifier
errors. Find where the merged imports from mergeGeneratorImports are emitted
around line 239-246 and apply the filterLocalStrictMockTypeImports function to
filter out locally declared strict mock types before emitting the split mock
imports, ensuring that only safe imports are included in the output.
In `@packages/core/src/writers/tags-mode.ts`:
- Around line 336-348: The usesSchemaFactories check on line 336 is evaluating
all generators in output.mock.generators globally, causing schema-factory
recovery to execute for unrelated mock entries in the same loop when names
collide. Scope this check to only the current mock entry being processed by
restricting the generator check to generators that are specific to the current
output or mock entry context, rather than checking all generators in the output
collection. This ensures that collectRecoveredSchemaFactoryImports is only
called when the specific mock entry being processed actually uses faker with
schemas enabled, preventing spurious imports in unrelated mock files.
In `@packages/mock/src/faker/imports.ts`:
- Around line 42-44: The regex pattern in the matchAll call only matches factory
signatures that use Partial types for the overrideResponse parameter, but strict
factories use generic signatures and are being skipped, resulting in missing
type imports. Update the regex pattern to handle both non-strict Partial type
signatures and strict generic signatures. The pattern should be made more
flexible to capture factory signatures regardless of whether they use Partial or
generic type constraints on the override parameter.
In `@packages/mock/src/mock-types.ts`:
- Around line 53-60: The condition that checks for schema.oneOf, schema.anyOf,
and schema.allOf in the type classification logic unconditionally treats all
composed schemas as object-kind. For scalar unions that compose scalar types
rather than objects, this misclassification causes them to be routed
incorrectly. Instead of unconditionally including these composition keywords in
the object-kind check, add logic to inspect the schemas within the composition
arrays (the items in oneOf, anyOf, or allOf) to determine whether they actually
compose objects or just scalars, and only classify them as object-kind when they
genuinely compose object types.
In `@packages/mock/src/msw/mocks.test.ts`:
- Line 8: The regression test for aggregating imports when response.imports is
undefined has a tautological assertion at line 31 that always passes
(toBeGreaterThanOrEqual(0)). Replace this weak assertion with an explicit check
that validates the intended behavior of aggregating imports—verify that the
actual aggregated imports match the expected aggregated imports from the test
case, ensuring the test will properly catch regressions in the undefined-import
handling logic.
---
Nitpick comments:
In `@packages/mock/src/faker/imports.test.ts`:
- Around line 67-79: The test suite for collectSplitMockTypeImports currently
only validates the non-strict signature form. Add an additional test case
(another it() block within the describe('collectSplitMockTypeImports') block)
that covers the strict generic factory signature pattern. This new test should
create an implementation string using the strict-signature form with a generic
type parameter constraint (like <O extends Partial<T>), an optional
overrideResponse parameter, and a return type using MockWithNullableOverrides,
then verify that collectSplitMockTypeImports correctly extracts all necessary
type imports from this pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af6ce46e-662c-4c0a-a850-f1dfb9b587f4
⛔ Files ignored due to path filters (66)
tests/__snapshots__/mock/issue-3525-multi/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3525/model/index.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/pets/pets.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3574-strict-mock-tags-split-fetch/pets/pets/pets.msw.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3574-strict-mock-tags-split-multi-fetch/store/store/store.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3574-strict-mock-tags-split-multi-fetch/store/store/store.msw.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/pets/pets.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3574-strict-mock-tags-split/pets/pets/pets.msw.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-binary-response-imports/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-binary-response-imports/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/pets/pets/pets.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/pets/pets/pets.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/healthStatus.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/index.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/pet.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/petDetailResponse.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/petOwner.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-tags-split-schema-imports/schemas/responseBase.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child0.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child1.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child10.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child11.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child12.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child13.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child14.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child15.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child16.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child17.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child18.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child19.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child2.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child20.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child21.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child22.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child23.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child24.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child25.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child26.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child27.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child28.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child29.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child3.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child4.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child5.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child6.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child7.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child8.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/child9.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/index.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590-wide-schema-imports/model/wideParent.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/index.faker.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/pet.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/petCategory.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/petDetail.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/petDetailSettingsItem.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/petList.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/petProfile.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/petSetting.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/photoUpload.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/score.tsis excluded by!**/__snapshots__/**tests/__snapshots__/mock/issue-3590/model/status.tsis excluded by!**/__snapshots__/**
📒 Files selected for processing (32)
packages/core/src/types.tspackages/core/src/writers/finalize-mock-implementation.tspackages/core/src/writers/mock-imports.test.tspackages/core/src/writers/mock-imports.tspackages/core/src/writers/single-mode.tspackages/core/src/writers/split-mode.tspackages/core/src/writers/split-tags-mode.tspackages/core/src/writers/tags-mode.tspackages/core/src/writers/target-tags.tspackages/core/src/writers/target.tspackages/mock/src/faker/getters/array-item-factory.tspackages/mock/src/faker/getters/combine.tspackages/mock/src/faker/getters/object.test.tspackages/mock/src/faker/getters/object.tspackages/mock/src/faker/imports.test.tspackages/mock/src/faker/imports.tspackages/mock/src/faker/index.test.tspackages/mock/src/faker/index.tspackages/mock/src/faker/resolvers/value.tspackages/mock/src/mock-types.test.tspackages/mock/src/mock-types.tspackages/mock/src/msw/index.tspackages/mock/src/msw/mocks.test.tspackages/mock/src/msw/mocks.tspackages/orval/src/client.tspackages/orval/src/write-specs.tstests/api-generation.spec.tstests/configs/mock.config.tstests/specifications/issue-3590-binary-response-imports.yamltests/specifications/issue-3590-tags-split-schema-imports.yamltests/specifications/issue-3590-wide-schema-imports.yamltests/specifications/issue-3590.yaml
…#3590) Filter local strict mock type imports in split mode. Scope schema-factory recovery to the current mock generator. Recognize strict factory signatures when collecting split imports. Classify scalar composed schemas as aliases. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/mock/src/mock-types.ts`:
- Around line 71-72: The `branch` parameter in the callback function passed to
`branches.some()` is missing an explicit type annotation, which causes
TypeScript to infer it as `any` and triggers a strict type checking error. Add
an explicit type annotation to the `branch` parameter in the callback to
indicate it should be typed as `SchemaObject | ReferenceObject` (the union type
of items that can appear in `oneOf`, `anyOf`, and `allOf` arrays according to
the OpenAPI specification). This will satisfy the strict TypeScript checks and
eliminate the implicit `any` type warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdca5b8d-1403-48c7-91b1-ff86d976eda3
📒 Files selected for processing (8)
packages/core/src/writers/split-mode.tspackages/core/src/writers/split-tags-mode.tspackages/core/src/writers/tags-mode.tspackages/mock/src/faker/imports.test.tspackages/mock/src/faker/imports.tspackages/mock/src/mock-types.test.tspackages/mock/src/mock-types.tspackages/mock/src/msw/mocks.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/mock/src/msw/mocks.test.ts
- packages/mock/src/faker/imports.test.ts
- packages/core/src/writers/split-mode.ts
- packages/core/src/writers/tags-mode.ts
- packages/mock/src/mock-types.test.ts
- packages/mock/src/faker/imports.ts
- packages/core/src/writers/split-tags-mode.ts
Cast oneOf/anyOf/allOf branches so the branch callback is not implicit any. Co-authored-by: Cursor <cursoragent@cursor.com>
|
I've fixed the CodeRabbit suggestions. |
|
i will let @wadakatu review |
|
Reviewed on @melloware's cc. Built locally — all gates green (build / typecheck / lint / format, unit 0-fail, snapshots 5056, I also stress-tested against LGTM. Non-blocking notes in a follow-up comment below. |
|
Non-blocking notes (none should hold up merge): Pre-existing gap (not this PR). A schema that is itself a nullable object ( Minor. |
…er-schema-mock-types Co-authored-by: Cursor <cursoragent@cursor.com>
) Push resolved imports from resolveMockValue into combineImports instead of scraping the shared imports array, which #3606 no longer mutates. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@melloware I've fixed the merge conflicts. |
|
Sorry just created another merge conflict :) |
…er-schema-mock-types Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
{Schema}Mockaliases: mapped types for objects, type aliases for enums/scalars/arrays, andArrayBufferfor binary faker schema mocksget*Mock()spreads to base mock types soMockWithNullableOverridesparent casts typecheckschemas/index.faker.tstests/specifications/issue-3590.yaml(followsissue-3525.yamlstyle)Fixes #3590
Supersedes #3591 (closed when the head fork was deleted and recreated).
Test plan
packages/mockunit tests (247 pass)tests/generated/mocktypecheck passes (node tests/scripts/typecheck-generated.mjs)mock issue-3590 strict faker schema mocks typecheck and emit correct aliasesMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes