Conversation
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
…elation filters (#2455) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Fields inherited via a mixin type on a delegate base model were not getting their `originModel` set in the generated schema, causing the ORM to include them in the wrong table's INSERT statement. Introduces `getOwnedFields` and `getDelegateOriginModel` helpers in `model-utils.ts` and uses them in both `ts-schema-generator` and `prisma-schema-generator`, replacing the previous logic that only checked `field.$container` directly. Fixes #2351 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
The test was failing on MySQL because the dataType enum field was required but not provided when creating DataText records. MySQL is stricter about enum validation than SQLite, causing 'Data truncated for column' errors. Co-authored-by: Yiming Cao <ymc9@users.noreply.github.com>
…names - Change DataType enum from TEXT/NUMBER to DataText/DataNumber - Remove explicit dataType field from test as it's auto-set by delegate discriminator - Fixes MySQL test failure: delegate discriminators must use model names Co-authored-by: Yiming Cao <ymc9@users.noreply.github.com>
Contributor
|
Claude finished @ymc9's task —— View job Code Review for v3.4.5 Release PR ✅This is a solid release that includes important bug fixes and performance improvements. Here's my comprehensive review: Todo List:
✅ Code Quality & Best PracticesExcellent: The code follows consistent patterns, proper error handling, and maintains good separation of concerns. Key observations:
🔒 Security ConsiderationsGood: No security vulnerabilities identified. The changes maintain proper access controls:
⚡ Performance ImprovementsExcellent: Several performance optimizations included:
🐛 Bug Fixes ReviewedAll regression tests are well-written and address real user issues: Issue #2351 - Delegate Model Inheritance:
Issue #2424 - Deep Nested Includes:
Issue #2440 - Relation Filtering:
Issue #2460 - createManyAndReturn:
📦 Version ManagementPerfect: All 38 packages consistently updated to v3.4.5 with no version drift. 🧪 Test CoverageExcellent: The regression tests are comprehensive and test real-world scenarios:
|
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPatch release 3.4.5: widespread package version bumps; ORM relation filters moved to EXISTS with MySQL override; temp-alias transformer adds byte-length based compacting; SDK adds delegate-origin utilities and updates generators; policy handler accepts default insert values; test client env tweak; four new regression tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/regression/test/issue-2460.test.ts (1)
28-35: Assert the returned payload, not just the row count.
toHaveLength(2)proves the insert no longer throws, but it won't catch the asymmetric-column bug if values get shifted between rows. Please verify the returnedkey/notepairs too.💡 Suggested test tightening
const result = await db.$setAuth(user).item.createManyAndReturn({ data: [ { key: 'a', note: 'hello' }, { key: 'b' }, ], }); expect(result).toHaveLength(2); + expect(result[0]).toMatchObject({ key: 'a', note: 'hello' }); + expect(result[1].key).toBe('b'); + expect(result[1].note ?? null).toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2460.test.ts` around lines 28 - 35, The test currently only checks length via expect(result).toHaveLength(2) which won't catch column-shift bugs; update the assertion after calling db.$setAuth(user).item.createManyAndReturn to assert the returned array contents exactly match the expected rows (e.g. verify each object's key and note fields correspond to { key: 'a', note: 'hello' } and { key: 'b', note: null/undefined } as appropriate). Locate the call site db.$setAuth(user).item.createManyAndReturn and replace/augment the length assertion with strict checks on result[*].key and result[*].note (or deep equality against the expected array) so any asymmetric-column shifts fail the test.tests/regression/test/issue-2440.test.ts (1)
22-118: Add one mutation case for the newEXISTSpath.These tests only exercise
findMany, but the MySQL-specific change in this PR is there to make relation filters safe insideUPDATE/DELETEstatements. A smallupdateManyordeleteManyassertion here would protect the path that changed most.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/regression/test/issue-2440.test.ts` around lines 22 - 118, Add a mutation assertion that exercises the new EXISTS path by calling updateMany or deleteMany with a relation filter (e.g., posts: { some: { value: { gt: 2 } } }) inside one of the tests (for example after the first 'some' test): perform db.user.updateMany({ where: { posts: { some: { value: { gt: 2 } } } }, data: { name: 'Updated' } }) (or deleteMany) and assert the returned count/affectedRows and that the correct user (userA) was updated/deleted; refer to symbols db.user.updateMany or db.user.deleteMany and the existing relation filters (posts.some.value.gt) to locate where to add the mutation case.
🤖 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/sdk/src/model-utils.ts`:
- Around line 93-101: The function getDelegateOriginModel uses identity
comparison via getOwnedFields(base).includes(field), which fails for inherited
fields that are different AST instances; update the check to compare fields by a
stable identifier (e.g., field.name or another unique property) instead of
object identity. Specifically, inside getDelegateOriginModel replace the
includes(field) test with a predicate like getOwnedFields(base).some(f => f.name
=== field.name) (or use the appropriate unique key for DataField), so inherited
fields are correctly recognized as delegate-owned; keep the rest of the loop
logic unchanged.
---
Nitpick comments:
In `@tests/regression/test/issue-2440.test.ts`:
- Around line 22-118: Add a mutation assertion that exercises the new EXISTS
path by calling updateMany or deleteMany with a relation filter (e.g., posts: {
some: { value: { gt: 2 } } }) inside one of the tests (for example after the
first 'some' test): perform db.user.updateMany({ where: { posts: { some: {
value: { gt: 2 } } } }, data: { name: 'Updated' } }) (or deleteMany) and assert
the returned count/affectedRows and that the correct user (userA) was
updated/deleted; refer to symbols db.user.updateMany or db.user.deleteMany and
the existing relation filters (posts.some.value.gt) to locate where to add the
mutation case.
In `@tests/regression/test/issue-2460.test.ts`:
- Around line 28-35: The test currently only checks length via
expect(result).toHaveLength(2) which won't catch column-shift bugs; update the
assertion after calling db.$setAuth(user).item.createManyAndReturn to assert the
returned array contents exactly match the expected rows (e.g. verify each
object's key and note fields correspond to { key: 'a', note: 'hello' } and {
key: 'b', note: null/undefined } as appropriate). Locate the call site
db.$setAuth(user).item.createManyAndReturn and replace/augment the length
assertion with strict checks on result[*].key and result[*].note (or deep
equality against the expected array) so any asymmetric-column shifts fail the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cee9fd64-9150-4e21-9bd2-dd8c0bfea5a1
📒 Files selected for processing (38)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/clients/client-helpers/package.jsonpackages/clients/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/orm/package.jsonpackages/orm/src/client/crud/dialects/base-dialect.tspackages/orm/src/client/crud/dialects/mysql.tspackages/orm/src/client/executor/temp-alias-transformer.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/options.tspackages/plugins/policy/package.jsonpackages/plugins/policy/src/policy-handler.tspackages/schema/package.jsonpackages/sdk/package.jsonpackages/sdk/src/model-utils.tspackages/sdk/src/prisma/prisma-schema-generator.tspackages/sdk/src/ts-schema-generator.tspackages/server/package.jsonpackages/testtools/package.jsonpackages/testtools/src/client.tspackages/zod/package.jsonsamples/orm/package.jsontests/e2e/package.jsontests/regression/package.jsontests/regression/test/issue-2351.test.tstests/regression/test/issue-2424.test.tstests/regression/test/issue-2440.test.tstests/regression/test/issue-2460.test.tstests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
Contributor
|
Claude finished @ymc9's task —— View job Code Review for v3.4.5 Release PR ✅This is a solid release that includes important bug fixes and performance improvements. Here's my comprehensive review: Todo List:
✅ Code Quality & Best PracticesExcellent: The code follows consistent patterns, proper error handling, and maintains good separation of concerns. Key observations:
🔒 Security ConsiderationsGood: No security vulnerabilities identified. The changes maintain proper access controls:
⚡ Performance ImprovementsExcellent: Several performance optimizations included:
🐛 Bug Fixes ReviewedAll regression tests are well-written and address real user issues: Issue #2351 - Delegate Model Inheritance:
Issue #2424 - Deep Nested Includes:
Issue #2440 - Relation Filtering:
Issue #2460 - createManyAndReturn:
📦 Version ManagementPerfect: All 38 packages consistently updated to v3.4.5 with no version drift. 🧪 Test CoverageExcellent: The regression tests are comprehensive and test real-world scenarios:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores