feat: customId function
#2377
feat: customId function
#2377
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:
📝 WalkthroughWalkthroughAdds a new default-value generator Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/e2e/orm/client-api/custom-id.test.ts`:
- Around line 140-160: Remove the unnecessary `@ts-expect-error` comments that
precede the assertion lines invoking client.user.create in the test (these are
the three occurrences after each createTestClient call where you do await
expect(client.user.create({ data: {} })).rejects.toThrowError(...)); leave the
valid `@ts-expect-error` comments that suppress the type errors on the customId
function arguments intact, and run typecheck to confirm no other TS errors are
introduced.
🧹 Nitpick comments (3)
packages/language/src/validators/function-invocation-validator.ts (1)
240-252: Duplicated logic with_checkNanoid.
_checkCustomIdis identical to_checkNanoid(lines 226–238). Consider extracting a shared helper like_checkPositiveFirstArgto avoid the duplication.♻️ Proposed refactor
+ private _checkPositiveFirstArg(expr: InvocationExpr, accept: ValidationAcceptor) { + const lengthArg = expr.args[0]?.value; + if (lengthArg) { + const length = getLiteral<number>(lengthArg); + if (length !== undefined && length <= 0) { + accept('error', 'first argument must be a positive number', { + node: expr.args[0]!, + }); + } + } + } + `@func`('nanoid') private _checkNanoid(expr: InvocationExpr, accept: ValidationAcceptor) { - // first argument must be positive if provided - const lengthArg = expr.args[0]?.value; - if (lengthArg) { - const length = getLiteral<number>(lengthArg); - if (length !== undefined && length <= 0) { - accept('error', 'first argument must be a positive number', { - node: expr.args[0]!, - }); - } - } + this._checkPositiveFirstArg(expr, accept); } `@func`('customId') private _checkCustomId(expr: InvocationExpr, accept: ValidationAcceptor) { - // first argument must be positive if provided - const lengthArg = expr.args[0]?.value; - if (lengthArg) { - const length = getLiteral<number>(lengthArg); - if (length !== undefined && length <= 0) { - accept('error', 'first argument must be a positive number', { - node: expr.args[0]!, - }); - } - } + this._checkPositiveFirstArg(expr, accept); }packages/language/test/function-invocation.test.ts (1)
418-434: Test coverage is incomplete compared to thenanoid()suite.The
nanoid()length validation tests (lines 362–416) cover both positive acceptance and rejection of0and-1. This suite only checks-1. Consider adding a positive-length acceptance test and acustomId(0)rejection test for parity.tests/e2e/orm/client-api/custom-id.test.ts (1)
24-24: Unnecessaryasyncondescribecallback.The
describeblock doesn't use top-levelawait, so theasyncmodifier is unnecessary and could be removed.
|
@ymc9 only non-empty strings are allowed at the moment. Should the restrictions be relaxed, or maybe tightened? Not sure if, for example, a string full of only whitespaces could cause issues. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/orm/src/client/crud/operations/base.ts`:
- Around line 1098-1109: The current use of invariant in the customId generation
path (the with('customId', ...) block that calls this.client.$options.customId)
loses error messages in production; replace those invariant calls with a
preserved-message error constructor (e.g., call createInvalidInputError or
createInternalError) so users see meaningful messages if customId is missing or
returns a non-string. Specifically, where the code currently does
invariant(this.client.$options.customId, ...) and invariant(generated && typeof
generated === 'string', ...), throw a new preserved-message error (using
createInvalidInputError/createInternalError) with the same descriptive text and
include context (model, field, length) as appropriate to aid debugging.
- Line 1107: The validation for generated custom IDs in base.ts currently only
checks `generated && typeof generated === 'string'`, which permits
whitespace-only strings; update the check used around the invariant (the
`generated` validation for "customId") to ensure the string is not all
whitespace by using a trimmed-length test (e.g., verify `typeof generated ===
'string' && generated.trim().length > 0`) before calling invariant, and if
whitespace-only IDs are intentionally allowed, add a clear comment documenting
that exception near the `customId` handling.
🧹 Nitpick comments (2)
packages/orm/src/client/options.ts (2)
43-65:modelcould useGetModels<Schema>for stronger typing.
ZModelFunctionContexton line 24 typesmodelasGetModels<Schema>, butCustomIdFunctionContextuses a plainstring. UsingGetModels<Schema>would give consumers autocomplete and type checking on the model name.Also, minor grammar nit: lines 50 and 55 say "for which the ID should be generated for" — the trailing "for" is redundant.
Proposed fix
export type CustomIdFunctionContext<Schema extends SchemaDef> = { /** * ZenStack client instance. */ client: ClientContract<Schema>; /** - * The model for which the ID should be generated for. + * The model for which the ID should be generated. */ - model: string; + model: GetModels<Schema>; /** - * The field for which the ID should be generated for. + * The field for which the ID should be generated. */ field: string; /** * The length of the ID as requested by the schema. */ length?: number; };
65-65:CustomIdFunctionis synchronous — async ID generation is unsupported.Returning
string(notstring | Promise<string>) prevents users from generating IDs via async sources (e.g., a remote ID service, DB sequence lookup). SincefillGeneratedAndDefaultValuesandevalGeneratorinbase.tsare both synchronous, this would need broader changes to support, but it's worth flagging as a potential limitation consumers may hit.If you don't plan to support async generation now, consider documenting this limitation in the JSDoc. If you do want to support it,
evalGeneratorandfillGeneratedAndDefaultValueswould need to become async.
1 participant
Allows developers to specify a custom implementation of an ID generation function.
Sample Usage
Closes #2348
Prisma issues: prisma/prisma#6719
Summary by CodeRabbit
New Features
Tests