fix: delegate count relation issue, default boolean value issue #302
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Typed JSON default validation special-case, standardizes relation count query construction via a model-select builder, switches default/generated value handling to use field definitions, avoids creating unique constraints that reference inherited fields, and adds several regression tests covering related behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
|
Claude finished @ymc9's task —— View job PR Review Complete ✅This PR fixes delegate count relation issues and default boolean value issues. The changes are well-structured and address specific bugs with comprehensive test coverage. Todo List:
🔍 Code Review SummaryQuality Assessment: Good ⭐⭐⭐⭐✅ Strengths
🔧 Key Changes Analysis1. Language Validator Fix (
2. Delegate Count Relations (
3. Default Value Generation (
4. Schema DB Constraints (
🧪 Test Coverage AnalysisAll 6 regression tests are well-written and cover:
|
There was a problem hiding this comment.
Pull Request Overview
Adds regression tests and runtime/language fixes for delegate relation counts and default value handling.
- Skip generating unique constraints for inherited fields in delegated models.
- Apply dialect transformation to generated defaults; fix relation _count handling to respect nested filters.
- Enforce that @default on typed JSON fields uses string literal defaults in the validator; add multiple regression tests.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/regression/test/v2-migrated/issue-2247.test.ts | Adds regression test to ensure delegated relations with nested policy checks don’t crash on read. |
| tests/regression/test/v2-migrated/issue-2246.test.ts | Adds tests verifying _count with where filters on relations under delegation. |
| tests/regression/test/v2-migrated/issue-2106.test.ts | Adds test for BigInt create default handling. |
| tests/regression/test/v2-migrated/issue-2039.test.ts | Adds test for typed JSON defaults parsing. |
| tests/regression/test/v2-migrated/issue-2038.test.ts | Adds test for boolean default from auth() context. |
| tests/regression/test/v2-migrated/issue-2028.test.ts | Adds test for querying compound unique inputs with delegation. |
| packages/runtime/src/client/helpers/schema-db-pusher.ts | Skips adding unique constraints for inherited fields to avoid duplicates in delegated inheritance. |
| packages/runtime/src/client/crud/operations/base.ts | Ensures generated defaults are dialect-transformed; cleans up default/updatedAt handling to use fieldDef. |
| packages/runtime/src/client/crud/dialects/base-dialect.ts | Applies nested where via buildModelSelect for _count queries and types fieldModel strongly. |
| packages/language/src/validators/attribute-application-validator.ts | Validates that @default on typed JSON fields uses string arguments. |
Comments suppressed due to low confidence (1)
packages/runtime/src/client/crud/dialects/base-dialect.ts:1
- This path changes _count logic for many-to-many relations by switching to buildModelSelect (which applies filters/policies). There is no test here exercising _count with where filters on M2M relations; please add a regression test that covers M2M relation counting with a where clause to ensure filtering is correctly applied post-change.
import { invariant, isPlainObject } from '@zenstackhq/common-helpers';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/language/src/validators/attribute-application-validator.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/regression/test/v2-migrated/issue-2106.test.ts (2)
4-13: Consider querying back the created record.The test creates a User with a BigInt field but doesn't verify that the value was persisted and can be retrieved correctly. Consider adding a query to fetch the created record and assert that the
agefield contains the expected BigInt value.Example:
const db = await createTestClient( ` model User { id Int @id age BigInt @@allow('all', true) } `, ); - await expect(db.user.create({ data: { id: 1, age: 1n } })).toResolveTruthy(); + const created = await db.user.create({ data: { id: 1, age: 1n } }); + expect(created).toBeTruthy(); + + const retrieved = await db.user.findUnique({ where: { id: 1 } }); + expect(retrieved?.age).toBe(1n);
15-15: Optional: Add a descriptive comment about the issue.Consider adding a brief comment explaining what bug issue 2106 addresses. This helps future maintainers understand the purpose of the test.
Example:
+// Issue 2106: Verify that BigInt fields can be created without errors await expect(db.user.create({ data: { id: 1, age: 1n } })).toResolveTruthy();tests/regression/test/v2-migrated/issue-2038.test.ts (2)
7-11: Consider adding a default value toUser.flag.While not required for this specific test, adding a default value to the
flagfield would make the schema more robust and prevent potential issues if users are created elsewhere in the test suite.model User { id Int @id @default(autoincrement()) - flag Boolean + flag Boolean @default(false) @@allow('all', true) }
4-24: Consider adding edge case coverage.The current test verifies the happy path where
auth().flagistrue. To improve robustness, consider adding test cases for:
auth().flagisfalse- No auth context is set
- Auth context exists but
flagisundefinedThis would provide more comprehensive coverage of the auth-driven default behavior.
tests/regression/test/v2-migrated/issue-2246.test.ts (1)
48-56: Assert the included relation contentRight now the test only checks that
findManyreturns one director. If the regression we’re guarding against caused themoviesinclude to ignore itswherefilter (or return an empty array), this assertion would still pass because there’s only one director in the fixture. Please assert the filtered include result so the test actually fails when the relation isn’t filtered as expected.- await expect( - db.director.findMany({ - include: { - movies: { - where: { title: 'Inception' }, - }, - }, - }), - ).resolves.toHaveLength(1); + const directors = await db.director.findMany({ + include: { + movies: { + where: { title: 'Inception' }, + }, + }, + }); + expect(directors).toHaveLength(1); + expect(directors[0]?.movies).toHaveLength(1); + expect(directors[0]?.movies[0]?.title).toBe('Inception');tests/regression/test/v2-migrated/issue-2247.test.ts (2)
4-4: Consider adding a descriptive comment explaining what's being tested.The test name "verifies issue 2247" is generic. Since this is a regression test for a specific fix related to delegation and relation count queries, consider adding a comment that explains what behavior from issue 2247 is being verified (e.g., "verifies that relation count queries with @@DeleGate don't crash" or similar).
Example:
+// Regression test for issue 2247: verifies that relation count queries +// with @@delegate(placeType) and nested relation filters resolve successfully it('verifies issue 2247', async () => {
59-60: Consider whether test data should be added to exercise access control rules.The test verifies that the query resolves successfully (which is appropriate for a regression test if the original issue was a crash or error). However, without creating any test data:
- The access control rules on lines 50-54 aren't meaningfully exercised
thing.findMany()will return an empty array, which still passestoResolveTruthy()since[]is truthyIf the intent is solely to verify that delegation + relation count queries compile and execute without errors, the current test is sufficient. However, if the access control filtering logic should also be verified, consider adding test data setup.
Example with data setup:
const authDb = db.$setAuth({ id: '1', employerId: '1' }); // Create test data to exercise access control rules await db.country.create({ data: { name: 'Country1', members: { create: { id: '1' } }, things: { create: { id: 'thing1' } } } }); const things = await authDb.thing.findMany(); expect(things).toHaveLength(1);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/language/src/validators/attribute-application-validator.ts(2 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(2 hunks)packages/runtime/src/client/crud/operations/base.ts(2 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(1 hunks)tests/regression/test/v2-migrated/issue-2028.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2038.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2039.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2106.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2246.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-2247.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/src/client/helpers/schema-db-pusher.tstests/regression/test/v2-migrated/issue-2246.test.tspackages/runtime/src/client/crud/operations/base.tstests/regression/test/v2-migrated/issue-2247.test.tstests/regression/test/v2-migrated/issue-2039.test.tstests/regression/test/v2-migrated/issue-2028.test.tspackages/language/src/validators/attribute-application-validator.tstests/regression/test/v2-migrated/issue-2038.test.tstests/regression/test/v2-migrated/issue-2106.test.ts
🧬 Code graph analysis (9)
packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
packages/sdk/src/schema/schema.ts (1)
GetModels(109-109)packages/runtime/src/client/query-utils.ts (1)
getManyToManyRelation(248-291)
tests/regression/test/v2-migrated/issue-2246.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
packages/runtime/src/client/crud/operations/base.ts (2)
packages/sdk/src/schema/schema.ts (1)
BuiltinType(85-95)packages/language/src/generated/ast.ts (1)
BuiltinType(101-101)
tests/regression/test/v2-migrated/issue-2247.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-2039.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-2028.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
packages/language/src/validators/attribute-application-validator.ts (2)
packages/language/src/utils.ts (1)
hasAttribute(51-53)packages/sdk/src/model-utils.ts (1)
hasAttribute(59-64)
tests/regression/test/v2-migrated/issue-2038.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-2106.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (10)
tests/regression/test/v2-migrated/issue-2028.test.ts (1)
55-88: Solid regression coverageCreating both delegated subtypes and asserting
findUniquewith the compound key for each case exercises the exact regression surface and should keep the delegate count behavior honest going forward.tests/regression/test/v2-migrated/issue-2106.test.ts (1)
1-2: LGTM!The imports are correct and appropriate for the test.
tests/regression/test/v2-migrated/issue-2038.test.ts (2)
1-2: LGTM!Standard test imports are appropriate for this regression test.
21-24: LGTM!The test logic correctly verifies that the
@default(auth().flag)attribute applies the authenticated user's flag value to the Post's published field.tests/regression/test/v2-migrated/issue-2247.test.ts (1)
5-57: LGTM! Schema appropriately tests delegation with relation count filters.The schema effectively demonstrates the scenario for issue 2247:
- Line 24: Uses
@@delegate(placeType)for polymorphic delegation- Lines 28-42: Inheritance hierarchy (Country → Region → City)
- Lines 50-54: Complex nested relation count filters with
?[]syntax that likely triggered the original issueThe schema structure is appropriate for verifying that delegation and relation count queries work together without errors.
packages/language/src/validators/attribute-application-validator.ts (1)
368-372: LGTM! JSON default validation is correct.The special case handling for JSON-typed fields correctly enforces that default values must be strings, which will later be parsed as JSON. This aligns with the test case in
issue-2039.test.tsand the broader changes for JSON default handling.packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
165-179: LGTM! Inheritance-aware constraint handling is correct.The logic properly skips unique constraints for fields inherited from base models, preventing duplicate constraint definitions. Both single-field and multi-field constraints are handled correctly by checking if
originModeldiffers from the current model.tests/regression/test/v2-migrated/issue-2039.test.ts (1)
4-27: LGTM! Comprehensive regression test for JSON defaults.The test effectively verifies that JSON-typed fields with string defaults are correctly parsed and persisted. The test coverage complements the validation changes in
attribute-application-validator.tsand ensures the end-to-end behavior works as expected.packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
994-1022: LGTM! Count query refactoring improves consistency.The changes standardize relation count queries to use
buildModelSelectinstead of directselectFromcalls. The type casts toGetModels<Schema>are appropriate for type safety, and the refactoring ensures consistent model selection behavior across count operations.packages/runtime/src/client/crud/operations/base.ts (1)
825-852: LGTM! Refactoring improves code clarity.The changes consistently use the
fieldDefvariable (already destructured from the loop) instead of repeatedly accessingfields[field]. This makes the code cleaner and more efficient while maintaining identical behavior. The transformation logic for generated values, updatedAt fields, and default values remains correct.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Summary by CodeRabbit
Bug Fixes
Tests