fix(mock): use one-sided bounds when only minLength/maxLength or minItems/maxItems is specified#3120
fix(mock): use one-sided bounds when only minLength/maxLength or minItems/maxItems is specified#3120youngcm2 wants to merge 7 commits intoorval-labs:masterfrom
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:
📝 WalkthroughWalkthroughAdjust mock scalar generation so global min/max defaults are only applied when neither bound is specified; added tests, an OpenAPI spec, generated model and endpoint snapshots, and a test config covering one-sided string/array constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![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/faker/getters/scalar.test.ts (1)
447-472: Good test coverage for string one-sided constraints.The tests correctly verify that only the specified bound is included in the output. Consider adding similar unit tests for array one-sided constraints (
minItemsonly andmaxItemsonly) to complement the integration snapshot tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mock/src/faker/getters/scalar.test.ts` around lines 447 - 472, Add two unit tests alongside the existing string-bound tests that call getMockScalar with baseArg and an item of type 'array' (use the same baseArg and item.name 'test-item'): one where only maxItems is set (e.g., maxItems: 5) and assert result.value contains the corresponding faker.array.* call with only max present, and another where only minItems is set (e.g., minItems: 3) and assert result.value contains only min; target the getMockScalar function and the item's minItems/maxItems fields so the tests mirror the string one-sided constraint cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/mock/src/faker/getters/scalar.test.ts`:
- Around line 447-472: Add two unit tests alongside the existing string-bound
tests that call getMockScalar with baseArg and an item of type 'array' (use the
same baseArg and item.name 'test-item'): one where only maxItems is set (e.g.,
maxItems: 5) and assert result.value contains the corresponding faker.array.*
call with only max present, and another where only minItems is set (e.g.,
minItems: 3) and assert result.value contains only min; target the getMockScalar
function and the item's minItems/maxItems fields so the tests mirror the string
one-sided constraint cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b1b4280-19cb-4a88-9cc1-0586cbdbe791
📒 Files selected for processing (7)
packages/mock/src/faker/getters/scalar.test.tspackages/mock/src/faker/getters/scalar.tstests/__snapshots__/mock/mock-constraints/endpoints.tstests/__snapshots__/mock/mock-constraints/model/constrainedItem.tstests/__snapshots__/mock/mock-constraints/model/index.tstests/configs/mock.config.tstests/specifications/mock-constraints.yaml
|
Running build now. |
|
Looks like linter issues |
|
Looks like Linter is still failing build |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/mock/src/faker/getters/scalar.ts`:
- Around line 270-277: The code forces a lower bound of 1 when only a max
constraint is provided; update the min-default logic so a missing min stays
undefined if a max is present. Concretely, change arrMin calculation (using
item.minItems, item.maxItems, safeMockOptions.arrayMin/arrayMax) so it uses
undefined instead of 1 when maxItems is specified but minItems is not, and apply
the same change to the analogous min/max handling for lengths (the
minLength/maxLength variables and their use of safeMockOptions). This preserves
one-sided max-only constraints and prevents invalid ranges like min:1 / max:0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d498401e-8abd-409b-be74-89e924643ebc
📒 Files selected for processing (1)
packages/mock/src/faker/getters/scalar.ts
youngcm2
force-pushed
the
fix/incorrect-array-values-msw
branch
from
0363caa to
c1bc38f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/mock/src/faker/getters/scalar.ts (1)
154-159: Consider applying the same cross-suppression pattern to number constraints.The number handling still uses simple nullish coalescing without the cross-suppression logic applied to arrays and strings. This could produce invalid faker ranges if a schema specifies
minimum: 100without a maximum, and the globalnumberMaxis set to a value less than 100.This is out of scope for the current PR but worth considering for consistency in a follow-up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mock/src/faker/getters/scalar.ts` around lines 154 - 159, The number bound computation (numMin/numMax in the scalar getter) should mirror the cross-suppression pattern used for arrays/strings: compute initial min/max from (exclusiveMinimum ?? minimum ?? safeMockOptions.numberMin) and (exclusiveMaximum ?? maximum ?? safeMockOptions.numberMax), then sanitize so the two bounds cannot produce an invalid range — if the schema provides a min that is greater than the chosen max, clamp/adjust the max to the schema min (or vice versa if schema max is less than the chosen min); update the logic around numMin and numMax to use these rules so global defaults from safeMockOptions are suppressed/clamped by explicit schema constraints and the final range is always valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/mock/src/faker/getters/scalar.ts`:
- Around line 154-159: The number bound computation (numMin/numMax in the scalar
getter) should mirror the cross-suppression pattern used for arrays/strings:
compute initial min/max from (exclusiveMinimum ?? minimum ??
safeMockOptions.numberMin) and (exclusiveMaximum ?? maximum ??
safeMockOptions.numberMax), then sanitize so the two bounds cannot produce an
invalid range — if the schema provides a min that is greater than the chosen
max, clamp/adjust the max to the schema min (or vice versa if schema max is less
than the chosen min); update the logic around numMin and numMax to use these
rules so global defaults from safeMockOptions are suppressed/clamped by explicit
schema constraints and the final range is always valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47be989d-fd08-4d30-9416-45ae4c988254
📒 Files selected for processing (2)
packages/mock/src/faker/getters/scalar.test.tspackages/mock/src/faker/getters/scalar.ts
✅ Files skipped from review due to trivial changes (1)
- packages/mock/src/faker/getters/scalar.test.ts
melloware
force-pushed
the
fix/incorrect-array-values-msw
branch
from
c1bc38f to
4070f72
Compare
|
Looks like Snapshots are failing now |
youngcm2
force-pushed
the
fix/incorrect-array-values-msw
branch
from
4070f72 to
7eb3d4b
Compare
melloware
force-pushed
the
fix/incorrect-array-values-msw
branch
3 times, most recently
from
28ff472 to
878651c
Compare
youngcm2
force-pushed
the
fix/incorrect-array-values-msw
branch
from
878651c to
e718086
Compare
|
@snebjorn can you chime in here the SNAPSHOTs seem to be frustrating users? |
|
Snapshots are working as expected. They're saying that - { length: faker.number.int({ min: 1, max: 100 }) }
+ { length: faker.number.int({ max: 100 }) }If this is intended, the snapshot should be updated. |
…tems/maxItems is specified
Updated generated comment to reflect orval version 8.6.2.
melloware
force-pushed
the
fix/incorrect-array-values-msw
branch
from
91b3420 to
2f6d2c9
Compare
3 participants
Closes #3121
Summary
When a schema specifies only
maxLength/maxItemswithout a corresponding min, or onlyminLength/minItemswithout a corresponding max, the mock scalar generator was incorrectly falling back to global defaults (stringMin: 10,arrayMin: 1, etc.) for the unspecified bound. This could produce invalid faker calls likefaker.string.alpha({length: {min: 10, max: 5}})where min > max.Changes
strMin/strMaxfallback logic: global defaults now only apply when neither bound is schema-specifiedarrMin/arrMaxfallback logic with the same rulemaxLength-only andminLength-only string schema casesmock-constraints.yaml) and snapshot for one-sided constraint scenariosSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chore