fix: relation include orderBy validation issue, more test migrations #297
Conversation
WalkthroughUpdates language base-resolution to include mixins and optional baseModel, broadening declaration matching. Adjusts runtime relation validator to accept single or array orderBy in to-many selects/includes. Fixes testtools extraSourceFiles extension handling. Adds multiple regression tests under tests/regression/test/v2-migrated covering schemas, policies, delegation, ordering, and validation scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 Code Review CompletedPR Summary: This PR fixes a validation issue with relation include orderBy and adds 18 new regression tests migrated from v2. 🔧 Core Changes Analysis1. Runtime Validator Fix (
2. Language Utils Enhancement (
3. Test Tools Fix (
📋 Test Coverage AssessmentAdded 18 comprehensive regression tests covering:
🎯 Code QualityStrengths:
Security Considerations:
Performance Impact:
🏆 Overall AssessmentThis is a high-quality fix that:
Recommendation: ✅ APPROVE The validation fix is essential for users who need to sort included relations using array syntax, and the extensive test suite greatly improves confidence in the stability of delegate/polymorphic features. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a relation include orderBy validation issue and adds multiple test migrations for various bug regression tests. The fix ensures that orderBy validation properly handles arrays in relation includes, while the test migrations verify that previously reported issues remain resolved.
- Fixed orderBy validation in relation includes to properly handle array inputs
- Added comprehensive regression test coverage for issues #1647-#1894
- Updated testtools to handle TypeScript file extensions more flexibly
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/runtime/src/client/crud/validator/index.ts | Fixed orderBy validation to use orArray wrapper for relation includes |
| packages/testtools/src/schema.ts | Added support for .ts file extension handling in extra source files |
| packages/language/src/utils.ts | Enhanced recursive base type resolution to include base models alongside mixins |
| tests/regression/test/v2-migrated/*.test.ts | Added 17 regression test files covering various schema validation and functionality issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
tests/regression/test/v2-migrated/issue-1758.test.ts (1)
4-4: Consider a more descriptive test name.The test name "verifies issue 1758" is functional but doesn't convey what the test validates. Consider a more descriptive name that explains the scenario being tested.
Apply this diff for a more descriptive name:
-it('verifies issue 1758', async () => { +it('validates error when using inherited fields from polymorphic base in @@unique', async () => {Optionally, add a comment referencing the issue number for traceability:
+// Regression test for issue #1758 it('validates error when using inherited fields from polymorphic base in @@unique', async () => {tests/regression/test/v2-migrated/issue-1849.test.ts (1)
1-27: LGTM! Consider adding a comment describing issue 1849.The test correctly exercises schema loading with an external enum module import and default enum value usage. The
loadSchemacall properly maps the additional schema key'enum'toenum.zmodel, which resolves the import statementimport './enum'as expected.The implicit assertion via
loadSchema's internal validation is sufficient for a regression test verifying that this schema pattern loads without errors.Optionally, add a brief comment explaining what issue 1849 was to improve maintainability:
+// Issue 1849: [brief description of the original issue] it('verifies issue 1849', async () => {tests/regression/test/v2-migrated/issue-1786.test.ts (1)
1-46: LGTM! Well-structured regression test.The test correctly follows the v2-migrated pattern: loading an inline schema with mixins (
with BaseContent), inheritance (extends Content), delegation (@@delegate), and access control rules. The implicit assertion inloadSchema(which validates successful schema loading) is sufficient for verifying that issue 1786's scenario compiles without errors.Consider adding a brief comment above the test explaining what specific aspect of issue 1786 this verifies (e.g., "Tests delegation with mixins and inheritance" or similar context).
tests/regression/test/v2-migrated/issue-1698.test.ts (2)
41-59: Consider removing debug console.log statements.The data creation logic is correct and properly connects the polymorphic entities. The console.log statements (lines 44, 49, 54, 59) are useful during development but could be removed for cleaner test output.
Apply this diff to remove the console.log statements:
const door1 = await db.ironDoor.create({ data: { strength: 100, color: 'blue' }, }); - console.log(door1); const door2 = await db.woodenDoor.create({ data: { texture: 'pine', color: 'red' }, }); - console.log(door2); const house1 = await db.privateHouse.create({ data: { size: 5000, door: { connect: { id: door1.id } } }, }); - console.log(house1); const house2 = await db.skyscraper.create({ data: { height: 3000, door: { connect: { id: door2.id } } }, }); - console.log(house2);
61-71: Assertions correctly verify polymorphic relation includes.The test properly validates that including delegated relations returns the correct polymorphic type with all fields (IronDoor's
strengthand WoodenDoor'stexture). The expected values match the created data.Similar to the data creation block, consider removing the console.log statements (lines 62, 68) for cleaner test output.
Apply this diff to remove the console.log statements:
const r1 = await db.privateHouse.findFirst({ include: { door: true } }); - console.log(r1); expect(r1).toMatchObject({ door: { color: 'blue', strength: 100 }, }); const r2 = (await db.skyscraper.findMany({ include: { door: true } }))[0]; - console.log(r2); expect(r2).toMatchObject({ door: { color: 'red', texture: 'pine' }, });tests/regression/test/v2-migrated/issue-1857.test.ts (1)
25-25: Replace type assertion with proper options.Using
{} as anybypasses type checking and could hide configuration issues.- const db = new ZenStackClient(schema, {} as any); + const db = new ZenStackClient(schema, { dialect: options.dialect });Note: You may need to pass the dialect or other required options from the outer scope.
tests/regression/test/v2-migrated/issue-1734.test.ts (1)
4-5: Skipped test is appropriate for incomplete feature.The test is correctly skipped with a clear TODO comment indicating that field-level policy support is pending. This is good practice for work-in-progress features.
If there isn't already a tracking issue for field-level policy support, would you like me to help draft one to ensure this test gets re-enabled once the feature is complete?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/language/src/utils.ts(1 hunks)packages/runtime/src/client/crud/validator/index.ts(1 hunks)packages/testtools/src/schema.ts(1 hunks)tests/regression/test/v2-migrated/issue-1647.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1648.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1674.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1681.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1693.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1695.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1698.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1734.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1745.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1755.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1758.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1763.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1786.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1835.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1849.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1857.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1870.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1894.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
tests/regression/test/v2-migrated/issue-1755.test.tstests/regression/test/v2-migrated/issue-1647.test.tstests/regression/test/v2-migrated/issue-1693.test.tstests/regression/test/v2-migrated/issue-1648.test.tspackages/testtools/src/schema.tspackages/runtime/src/client/crud/validator/index.tstests/regression/test/v2-migrated/issue-1681.test.tstests/regression/test/v2-migrated/issue-1870.test.tstests/regression/test/v2-migrated/issue-1734.test.tstests/regression/test/v2-migrated/issue-1698.test.tstests/regression/test/v2-migrated/issue-1786.test.tstests/regression/test/v2-migrated/issue-1894.test.tstests/regression/test/v2-migrated/issue-1835.test.tstests/regression/test/v2-migrated/issue-1695.test.tstests/regression/test/v2-migrated/issue-1857.test.tstests/regression/test/v2-migrated/issue-1745.test.tspackages/language/src/utils.tstests/regression/test/v2-migrated/issue-1674.test.tstests/regression/test/v2-migrated/issue-1763.test.tstests/regression/test/v2-migrated/issue-1849.test.tstests/regression/test/v2-migrated/issue-1758.test.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/testtools/src/schema.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.ts : The generated TypeScript schema should be named `schema.ts`
Applied to files:
packages/testtools/src/schema.ts
🧬 Code graph analysis (19)
tests/regression/test/v2-migrated/issue-1755.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(51-166)packages/runtime/src/client/functions.ts (1)
now(106-106)
tests/regression/test/v2-migrated/issue-1647.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1693.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1648.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1681.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1870.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1734.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1698.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1786.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1894.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1835.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1695.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1857.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1745.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
packages/language/src/utils.ts (2)
packages/language/src/generated/ast.ts (6)
isDataModel(391-393)TypeDef(725-733)TypeDef(735-735)DataModel(377-387)DataModel(389-389)isTypeDef(737-739)packages/language/src/ast.ts (1)
DataModel(49-54)
tests/regression/test/v2-migrated/issue-1674.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1763.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1849.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)
tests/regression/test/v2-migrated/issue-1758.test.ts (1)
packages/testtools/src/schema.ts (1)
loadSchemaWithError(126-148)
⏰ 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, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (25)
tests/regression/test/v2-migrated/issue-1758.test.ts (1)
4-27: LGTM! Well-structured regression test.The test correctly validates that ZenStack prevents using inherited fields from a polymorphic base model (
Contentwith@@delegate) in a@@uniqueconstraint on the derived model (Store). The schema setup properly demonstrates the error condition, and the expected error message is specific and clear.tests/regression/test/v2-migrated/issue-1698.test.ts (2)
1-2: LGTM!Imports are correct and appropriate for the test.
4-39: LGTM!The schema correctly implements delegation-based polymorphism for both House and Door hierarchies, with proper relations between them. The test setup effectively validates that includes work correctly with delegated models.
tests/regression/test/v2-migrated/issue-1857.test.ts (1)
37-38: Verify if zod schema validation should be implemented now.The TODO comment suggests pending work for zod schema support. Should this be completed as part of this test migration, or is it acceptable to defer?
If this validation is important for verifying issue 1857, consider implementing it now or creating a separate issue to track it.
packages/testtools/src/schema.ts (1)
55-61: LGTM! Good defensive coding.The refactored logic correctly prevents double
.tsextensions whenextraSourceFileskeys already include the extension. This aligns with the test suite's usage patterns where supplemental TypeScript sources may be passed with or without the extension.packages/runtime/src/client/crud/validator/index.ts (1)
680-682: LGTM! Expands orderBy flexibility for to-many relations.The change correctly allows
orderByto accept both a single object and an array of objects in to-many relation includes/selects. This aligns with Prisma's behavior and is validated by the test coverage intests/regression/test/v2-migrated/issue-1755.test.ts.tests/regression/test/v2-migrated/issue-1695.test.ts (1)
1-21: LGTM! Well-structured regression test.The test correctly verifies schema loading with a SoftDelete mixin and access control rules. The inline schema follows ZenStack conventions and properly demonstrates the issue scenario.
packages/language/src/utils.ts (1)
169-174: LGTM! Correctly aggregates bases from mixins and baseModel.The refactored logic properly includes both mixins and the optional
baseModelwhendeclis aDataModel, broadening the base resolution to handle inheritance scenarios. The updated type predicate correctly matches bothTypeDefandDataModeldeclarations.tests/regression/test/v2-migrated/issue-1755.test.ts (1)
1-58: LGTM! Comprehensive orderBy validation test.The test thoroughly verifies both scalar and array
orderByforms at top-level and nested relation levels. This properly validates the runtime validator changes that expandedorderByacceptance for to-many relations.tests/regression/test/v2-migrated/issue-1745.test.ts (1)
1-94: LGTM! Comprehensive schema validation test.The test verifies a complex schema with delegation, enums, type inheritance, and access control rules. The inline schema is well-structured and properly demonstrates the scenario from issue 1745.
tests/regression/test/v2-migrated/issue-1693.test.ts (1)
1-18: LGTM! Clean delegation test.The test correctly verifies schema loading with a simple delegation pattern where
DogextendsAnimal. The inline schema is minimal and focused on the specific scenario from issue 1693.tests/regression/test/v2-migrated/issue-1870.test.ts (1)
1-14: LGTM! Validates Unsupported type handling.The test correctly verifies schema loading with PostgreSQL's
Unsupportedtype for spatial data and a Gist index. The inline schema is minimal and properly demonstrates the scenario from issue 1870.tests/regression/test/v2-migrated/issue-1835.test.ts (1)
1-23: LGTM!The test correctly verifies that a schema with an enum and a model marked with
@@ignorecan be loaded successfully. The test structure is appropriate for a regression test.tests/regression/test/v2-migrated/issue-1648.test.ts (1)
4-42: Test logic is correct.The test properly validates that the deny rule based on a related model's field works as expected. The policy enforcement is correctly tested for both allowed and denied cases.
tests/regression/test/v2-migrated/issue-1647.test.ts (1)
4-51: LGTM! Skipped test with clear TODO.The test suite is appropriately skipped with a clear TODO comment indicating it's pending multi-schema support. The test structure is correct and ready to be enabled once the feature is available.
tests/regression/test/v2-migrated/issue-1681.test.ts (1)
4-29: LGTM!The test correctly validates that authentication context propagates to nested operations and that both
createManyandcreateManyAndReturnwork withauth().iddefaults. The test logic is sound.tests/regression/test/v2-migrated/issue-1674.test.ts (1)
4-80: LGTM!The test correctly validates policy enforcement on delegated models. The expectation that
postA.createis rejected is correct since thePostmodel (and its derived models) lacks an explicit@@allowrule for create operations.tests/regression/test/v2-migrated/issue-1763.test.ts (1)
4-42: LGTM!The test validates
extraSourceFileshandling with delegated models. The test structure is appropriate for verifying that the generated TypeScript code compiles and executes correctly with the schema.tests/regression/test/v2-migrated/issue-1734.test.ts (3)
81-103: LGTM! Test logic correctly validates field-level policies.The test assertions properly verify the expected behavior:
- Queries the polymorphic
profiletable for both user and organization- Verifies that without authentication, the
publishedfield is not readable (lines 94-95)- Verifies that with authentication (user who has access), the
publishedfield becomes readable (line 103)The explicit check for
publishedbeing undefined (line 95) is a good defensive assertion on top of thetoMatchObjectcheck.
44-44:@default(auth().id)is supported; no further action needed.
52-79: Tests already verifytypeauto-population via delegation.The inline schema declares
@@delegate(type)onProfile, and the existing assertions onfoundUserandfoundOrgconfirm thetypefield is populated as expected. No further action required.tests/regression/test/v2-migrated/issue-1894.test.ts (4)
1-2: LGTM!The imports are appropriate for a Vitest-based ZenStack regression test.
7-23: LGTM!The schema correctly demonstrates ZenStack's delegation and inheritance features. The relation setup and @@DeleGate directive are properly configured.
44-45: LGTM!The test assertions correctly verify that model delegation works with relation connections. The use of explicit
id: 0and theconnectsyntax properly exercises the feature being tested.
26-40: Missing await on async function call.Line 37 calls
main()without awaiting it. Sincemain()is declared as async and performs asynchronous database operations, this will cause the script to exit before those operations complete, potentially leading to incomplete execution or race conditions.Apply this diff to fix the async handling:
- main(); + await main();Additionally, note that line 32 uses
{} as anyfor the client options. While acceptable in test code, consider passing a more specific type or at least an empty object without the type assertion if no options are needed.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Bug Fixes
Tests