fix(core): allow 'all' as isUUID validator argument#18200
Conversation
The isUUID field validator accepts both number (1, 2, 3, 4, 5, 7)
and the string 'all' as valid arguments, but the TypeScript type
definition only allowed number.
This fix updates the type to include 'all' as a valid option for
both the simple form (isUUID: 'all') and the object form
(isUUID: { msg: string; args: number | 'all' }).
Fixes sequelize#18171
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughColumn Changes
Sequence Diagram(s)(Skipped — changes are type updates and test additions without multi-component runtime control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
| * only allow uuids | ||
| */ | ||
| isUUID?: number | { msg: string; args: number }; | ||
| isUUID?: number | 'all' | { msg: string; args: number | 'all' }; |
There was a problem hiding this comment.
Shouldn't we import from @types/validator?
ephys
changed the title
The isUUID field validator delegates to validator.js, whose UUIDVersion type covers numeric UUID versions as well as string versions such as 'all'. Reuse the type from @types/validator for both the direct isUUID form and the custom message args form, and keep typings coverage for the newly supported 'all' usage. Fixes sequelize#18171
|
Pushed a follow-up commit addressing the review feedback about reusing validator's own type. Summary:
Local verification:
Note: I avoided force-updating the branch to latest upstream because GitHub rejected the rebased push with the current OAuth token lacking |
![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/core/src/model.d.ts (1)
9-9: Prefer the top-level'validator'import over the deep internal path.
UUIDVersionis exported at the top level of@types/[email protected], so the idiomatic import isfrom 'validator'. TypeScript resolves this to the@types/validatordeclarations automatically; you don't import from@types/validatordirectly. The current deep importfrom 'validator/lib/isUUID'couples the public.d.tssurface to DefinitelyTyped's internal file structure and is unnecessarily fragile.Dependencies are already correct: both
validatorand@types/validatorare declared independenciesof@sequelize/core, so the new public type will resolve properly for downstream consumers.♻️ Suggested change
-import type { UUIDVersion } from 'validator/lib/isUUID'; +import type { UUIDVersion } from 'validator';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/model.d.ts` at line 9, The current type import uses a deep path: replace the import of UUIDVersion from 'validator/lib/isUUID' with the top-level module export from 'validator' so the public .d.ts surface doesn't depend on DefinitelyTyped internals; update the import statement that references UUIDVersion to import from 'validator' and ensure any other references to that symbol (UUIDVersion) remain unchanged.
🤖 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/core/src/model.d.ts`:
- Line 9: The current type import uses a deep path: replace the import of
UUIDVersion from 'validator/lib/isUUID' with the top-level module export from
'validator' so the public .d.ts surface doesn't depend on DefinitelyTyped
internals; update the import statement that references UUIDVersion to import
from 'validator' and ensure any other references to that symbol (UUIDVersion)
remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30103197-235c-4399-b99d-06776a34b613
📒 Files selected for processing (1)
packages/core/src/model.d.ts
| StrictRequiredBy, | ||
| } from '@sequelize/utils'; | ||
| import type { SetRequired } from 'type-fest'; | ||
| import type { UUIDVersion } from 'validator/lib/isUUID'; |
There was a problem hiding this comment.
| import type { UUIDVersion } from 'validator/lib/isUUID'; | |
| import type { UUIDVersion } from 'validator'; |
I haven't tested it, but I think this is cleaner and coderabbit also had a nitpick comment that this is nicer. Could you see if this suggestion works?
|
Thanks, @WikiRik. I tested the suggestion and updated Verification (all passed):
I kept verification scoped to the changed file and @sequelize/core typings; I did not run a full monorepo build. |
2 participants
Description of Changes
The isUUID field validator accepts both number (1, 2, 3, 4, 5, 7) and the string 'all' as valid arguments, but the TypeScript type definition only allowed number.
This PR updates the type to include 'all' as a valid option for both:
isUUID: 'all'isUUID: { msg: string; args: number | 'all' }Issue Reference
Fixes #18171
Type of Change
Testing
Added TypeScript type tests in
test/types/validators.tsto verify that bothisUUID: 4andisUUID: 'all'are accepted by the type checker.Checklist
Summary by CodeRabbit
New Features
Tests