docs: add JSDoc #22
docs: add JSDoc #22
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ModelHandler
participant UpdateOperationHandler
participant Database
Client->>ModelHandler: updateManyAndReturn(args)
ModelHandler->>UpdateOperationHandler: handle('updateManyAndReturn', args)
UpdateOperationHandler->>Database: Begin transaction
UpdateOperationHandler->>Database: Update records (with where/data)
Database-->>UpdateOperationHandler: Updated row IDs
UpdateOperationHandler->>Database: Select updated rows by IDs
Database-->>UpdateOperationHandler: Updated entities
UpdateOperationHandler->>ModelHandler: Return updated entities
ModelHandler->>Client: Return updated entities
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (4)
packages/runtime/src/client/crud/operations/delete.ts (1)
58-64: 🛠️ Refactor suggestionMissing validation:
limitshould be a positive integer
runDeleteManyforwardsargs?.limitunchecked. Add a guard in the input validator (or here) to ensurelimitis a positive integer; negative or zero silently produces confusing results.- const result = await this.delete( + if (args?.limit !== undefined && args.limit <= 0) { + throw new Error('`limit` must be a positive integer'); + } + const result = await this.delete( this.kysely, this.model, args?.where, args?.limit, false );packages/runtime/src/client/crud/validator.ts (1)
1100-1119:⚠️ Potential issueSchema needs
.optional()for consistency
makeUpdateManyAndReturnSchemareturns a required object, but
validateUpdateManyAndReturnArgsallowsundefined.
CallingupdateManyAndReturn()with no args will currently throw.- const result = base.merge( + const result = base.merge( z.object({ select: this.makeSelectSchema(model).optional(), omit: this.makeOmitSchema(model).optional(), }) ); - return this.refineForSelectOmitMutuallyExclusive(result); + return this.refineForSelectOmitMutuallyExclusive(result).optional();packages/runtime/src/client/crud/operations/base.ts (2)
1471-1505: 🛠️ Refactor suggestionAdd basic validation for
limit
limitis forwarded to SQL without checks. Negative or zero values will compile but behave unpredictably.if (limit !== undefined && limit <= 0) { throw new InternalError('limit must be a positive integer'); }A tiny guard avoids accidental “no-op” / undefined behaviour on some dialects.
1458-1467: 🛠️ Refactor suggestionSilently dropping relation fields hides user errors
Relation attributes in
dataare ignored (continuepath).
Fail fast to save debugging time:if (isRelationField(...) ) { throw new QueryError('updateMany does not support relation updates'); }Prefer an explicit error to surprising partial updates.
🧹 Nitpick comments (6)
TODO.md (1)
57-59: Indentation violates MD007The new checklist item is indented with 8 spaces, while the rest of the list uses 4. This triggers markdown-lint (see static-analysis hint).
- - [x] JSDoc for CRUD methods + - [x] JSDoc for CRUD methods🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
57-57: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
59-59: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
packages/runtime/test/client-api/create-many-and-return.test.ts (1)
83-85: Prefer runtime-only assertions over@ts-expect-errorin tests
@ts-expect-errorsuppresses the compiler error but still emits JS that touchesname, so we need theundefinedassertions anyway.
Using Jest/Vitest you could avoid emitting the extra JS by asserting on shape instead:-// @ts-expect-error -expect(r[0]!.name).toBeUndefined(); +expect(r[0]).not.toHaveProperty('name');Same for the second block. This keeps the type-safety (compiler will still complain if
nameappears) and removes dead code.Also applies to: 91-93
packages/runtime/test/typing/verify-typing.ts (1)
77-79: Prefer compile-time assertions over runtimeconsole.login typing testsThe added
console.logstatements validate access to nested_countfields only at runtime. Consider usingts-expect-error/expectAssignablestyle assertions (e.g.expectTypeOf(user3._count.posts).toEqualTypeOf<number>()) so that the test continues to guard the type surface without producing noisy runtime output.packages/runtime/src/client/crud/operations/delete.ts (1)
40-46: Explicitundefinedarg forlimitis redundant and obscures the call-site
runDeletealways deletes at most one record (unique filter), so passing an extra positionalundefinedforlimitprovides no value and makes the signature harder to follow. Omitting the param entirely keeps the call in-sync with the logical need while still relying on the defaultundefined.- const result = await this.delete( - this.kysely, - this.model, - args.where, - undefined, - false - ); + const result = await this.delete( + this.kysely, + this.model, + args.where, + /* limit */ undefined, + /* returnData */ false + );Keeping the named comment is optional but clarifies intent without the extra parameter.
packages/runtime/test/client-api/update-many.test.ts (1)
101-118: Consider adding a “no-match” assertionNice happy-path test. You may also want a case where
updateManyAndReturnfinds zero rows and returns an empty array to lock in that behaviour.packages/runtime/src/client/crud/operations/update.ts (1)
94-121: Minor optimisation opportunityYou already have the updated rows in
updateResult; doing an extra
readround-trip just to applyselect/omitincurs an extra query and
builds a potentially hugeORclause.
Consider post-processingupdateResultin memory instead.(Not blocking.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
TODO.md(1 hunks)packages/runtime/src/client/client-impl.ts(2 hunks)packages/runtime/src/client/contract.ts(1 hunks)packages/runtime/src/client/crud-types.ts(5 hunks)packages/runtime/src/client/crud/dialects/base.ts(1 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(1 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(1 hunks)packages/runtime/src/client/crud/operations/base.ts(7 hunks)packages/runtime/src/client/crud/operations/delete.ts(2 hunks)packages/runtime/src/client/crud/operations/update.ts(4 hunks)packages/runtime/src/client/crud/validator.ts(4 hunks)packages/runtime/src/client/options.ts(2 hunks)packages/runtime/test/client-api/create-many-and-return.test.ts(1 hunks)packages/runtime/test/client-api/delete-many.test.ts(1 hunks)packages/runtime/test/client-api/find.test.ts(1 hunks)packages/runtime/test/client-api/update-many.test.ts(1 hunks)packages/runtime/test/typing/verify-typing.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/runtime/src/client/options.ts (1)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(11-18)
packages/runtime/src/client/crud/validator.ts (2)
packages/sdk/src/schema/schema.ts (1)
GetModels(105-108)packages/runtime/src/client/crud-types.ts (1)
UpdateManyAndReturnArgs(812-818)
packages/runtime/src/client/crud/operations/update.ts (3)
packages/runtime/src/client/crud-types.ts (1)
UpdateManyAndReturnArgs(812-818)packages/sdk/src/schema/schema.ts (1)
GetModels(105-108)packages/runtime/src/client/query-utils.ts (1)
getIdValues(183-196)
packages/runtime/src/client/crud/operations/base.ts (5)
packages/runtime/src/client/index.ts (1)
ToKysely(7-7)packages/runtime/src/client/query-builder.ts (1)
ToKysely(18-18)packages/sdk/src/schema/schema.ts (1)
GetModels(105-108)packages/runtime/src/client/errors.ts (1)
InternalError(7-11)packages/runtime/src/client/query-utils.ts (1)
getIdFields(40-46)
packages/runtime/src/client/crud-types.ts (1)
packages/sdk/src/schema/schema.ts (2)
SchemaDef(11-18)GetModels(105-108)
🪛 markdownlint-cli2 (0.17.2)
TODO.md
58-58: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
🪛 Biome (1.9.4)
packages/runtime/src/client/options.ts
[error] 73-73: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (10)
packages/runtime/src/client/contract.ts (1)
66-69: JSDoc visibility correctly clarifiedThe
@privatetag is the right way to hide$pushSchema()from public docs while keeping it available for tests. No further action needed.packages/runtime/src/client/crud/dialects/sqlite.ts (1)
325-327: Redundant override?
supportsDeleteWithLimitis hard-coded tofalse, which is correct for SQLite. If the base class already defaults tofalse, this override is unnecessary noise. Otherwise, keep it.packages/runtime/src/client/crud/dialects/postgresql.ts (1)
393-395: Same question about default implementationAs with the SQLite dialect, verify whether the base class default is already
false. If so, consider removing this duplicate override to reduce maintenance overhead.packages/runtime/src/client/client-impl.ts (1)
427-434:updateManyAndReturnwired correctly – ensure typings & docs are in lock-stepThe new handler mirrors
createManyAndReturn, usingpostProcess=true, which is appropriate because returned rows need transformation. 👍Please double-check that:
ModelOperationsincludesupdateManyAndReturn(looks covered incrud-types.ts).InputValidatorenforces mutually exclusiveselect/include/omitthe same way as other batch-returning methods.- Docs (JSDoc & README) mention the new capability.
Otherwise the addition LGTM.
packages/runtime/test/client-api/delete-many.test.ts (1)
58-80: Great coverage forlimit, consider dialect skip logicThe test exercises both zero-match and partial-delete paths—nice!
One caveat: for dialects that don’t supportDELETE … LIMIT, the client may emulate the behaviour in memory or fall back silently. Ensure the test matrix skips or adapts for those dialects, otherwise CI will fail once support for such dialects is re-enabled.packages/runtime/test/client-api/find.test.ts (1)
829-850: Tests look good – covers both simple and filtered counts.packages/runtime/src/client/crud/validator.ts (1)
767-774: *Good parity with other AndReturn schemasRefactor removes
includeand adds mutual-exclusion refinement – looks correct.packages/runtime/src/client/crud/dialects/base.ts (1)
1177-1211: Appreciate the added JSDoc – clarifies the dialect contract.packages/runtime/src/client/crud/operations/base.ts (1)
1483-1503: DuplicatebuildIdFieldRefscomputation
buildIdFieldRefsis invoked twice inside the same expression – once with ats-expect-error.
Store the result in a local variable to avoid repetition and drop the suppress-comment.[ suggest_optional_refactor ]
packages/runtime/src/client/crud-types.ts (1)
1696-1699: UseSelectSubsetforupdateManyAndReturnto keep API consistency
createManyAndReturnalready leveragesSelectSubsetto ban illegalselect+omitcombos.
Switching here keeps the safeguard:- updateManyAndReturn<T extends UpdateManyAndReturnArgs<...>>( - args: Subset<T, UpdateManyAndReturnArgs<...>> + updateManyAndReturn<T extends UpdateManyAndReturnArgs<...>>( + args: SelectSubset<T, UpdateManyAndReturnArgs<...>>[ suggest_essential_refactor ]
Summary by CodeRabbit
New Features
updateManyAndReturnmethod.Bug Fixes
limitoption in bulk delete operations to ensure correct behavior when deleting multiple records.Documentation
Tests
updateManyAndReturnfeature.deleteManywith thelimitoption.