fix(delegate): column name mapping issue when delegates are involved #296
Conversation
|
Claude finished @ymc9's task —— View job Code Review CompleteI've thoroughly reviewed this PR that fixes delegate column name mapping issues. Here's my comprehensive feedback: Todo List:
SummaryThis PR addresses critical issues with column name mapping when working with delegate models in ZenStack's polymorphic inheritance system. The changes are well-targeted and fix legitimate bugs around field resolution and selection. Code Quality & Best Practices ✅Strengths:
Specific improvements:
Bug Analysis ✅Issues Fixed:
No new bugs identified - the changes are defensive and maintain backward compatibility. Performance Considerations ✅Positive impacts:
No performance regressions - the added logic only executes for delegate models and has minimal overhead. Security Assessment ✅No security concerns identified:
Test Coverage Assessment ✅Excellent test coverage:
Key test scenarios:
RecommendationsMinor Improvements:
Code Examples:The fix in // Before: Incorrect model context for delegate fields
return query.select(this.eb.ref(`${modelAlias}.${field}`).as(field));
// After: Proper recursive handling for delegate origin models
return this.buildSelectField(query, fieldDef.originModel, fieldDef.originModel, field);Verdict: APPROVED ✅This is a solid fix for legitimate bugs in delegate model handling. The changes are:
The delegate column name mapping issues were clearly problematic for users working with polymorphic inheritance, and this PR resolves them effectively. |
|
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. WalkthroughReplaced direct Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a column name mapping issue when delegate models are involved in queries by updating reference generation in the ZenStack runtime. The key problem was that the runtime wasn't properly handling field references when delegate inheritance was used, causing incorrect SQL generation.
Key Changes:
- Updated reference generation from
sql.ref()toeb.ref()throughout the codebase for better consistency - Fixed name mapping logic to properly handle delegated model fields
- Enhanced field resolution to work correctly with inheritance hierarchies
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/client/executor/name-mapper.ts | Fixed field iteration and reference transformation for delegate models |
| packages/runtime/src/client/crud/dialects/base-dialect.ts | Updated all reference calls to use eb.ref() instead of sql.ref() |
| packages/runtime/src/client/crud/dialects/postgresql.ts | Updated PostgreSQL-specific reference calls |
| packages/runtime/src/client/crud/operations/*.ts | Updated reference calls in count, aggregate, and base operations |
| tests/e2e/orm/client-api/computed-fields.test.ts | Updated test to use eb.ref() instead of sql.ref() |
| tests/regression/test/v2-migrated/*.ts | Added comprehensive regression tests for various delegate-related 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/runtime/src/client/crud/operations/count.ts (1)
1-1: Fix unusedsqlimport breaking build.TypeScript compilation fails with TS6133 because
sqlis no longer referenced after switching toeb.ref. Please drop the import.Apply this diff:
-import { sql } from 'kysely';packages/runtime/src/client/crud/operations/aggregate.ts (1)
1-1: Remove unusedsqlimport to unblock build.After replacing
sql.refwitheb.ref,sqlis unused and triggers TS6133 in CI. Please delete the import.Apply this diff:
-import { sql } from 'kysely';
🧹 Nitpick comments (9)
tests/regression/test/v2-migrated/issue-1551.test.ts (1)
4-26: Verify that the test adequately covers the column name mapping fix.The test loads a schema with delegates and mapped columns (
@map('profile_id')), which aligns with the PR objective. However, the test only verifies that the schema can be loaded without errors—it doesn't perform any queries or assertions to confirm that the column name mapping works correctly at runtime with delegates.Consider whether runtime verification is needed:
- If issue #1551 was purely a schema generation/compilation error, then loading the schema is sufficient.
- If the issue involved incorrect SQL generation or runtime query behavior, add assertions that exercise the delegate relationship with mapped columns (e.g., create a User with an IndividualProfile and verify the profile_id mapping works correctly).
Additionally, the test name "verifies issue 1551" is generic. Consider renaming it to describe what's being verified, such as
'delegate with mapped foreign key column'.tests/regression/test/v2-migrated/issue-1518.test.ts (1)
23-29: Add explicit assertion to verify the record creation.The test currently relies on implicit assertion (the create operation throws if the bug exists). While this pattern is common for regression tests, adding an explicit verification would make the test more robust and self-documenting.
Consider adding a verification step:
- await db.taskActivity.create({ + const created = await db.taskActivity.create({ data: { id: '00000000-0000-0000-0000-111111111111', title: 'Test Activity', description: 'Description of task', }, }); + + expect(created).toMatchObject({ + id: '00000000-0000-0000-0000-111111111111', + title: 'Test Activity', + description: 'Description of task', + });Additionally, import
expectfrom vitest:-import { it } from 'vitest'; +import { expect, it } from 'vitest';tests/regression/test/v2-migrated/issue-1563.test.ts (1)
1-26: LGTM! Well-structured regression test.The test correctly verifies the fix for issue 1563 related to column name mapping when delegates are involved. The schema setup with a relation and validation constraint, followed by the update operation, appropriately exercises the code path that was problematic.
Optional enhancement:
Consider adding a brief comment above the test explaining what issue 1563 was about (e.g., "updates failing due to column name mapping with delegates" or similar). This would help future maintainers understand the specific scenario being tested.
tests/regression/test/v2-migrated/issue-1507.test.ts (1)
4-24: Consider expanding test coverage for edge cases.The test correctly verifies the positive case where
auth().agematches a profile's age. Consider adding:
- A negative test case where
auth().agedoesn't match any profile (e.g.,age: 25) to verify empty results- Verification of the returned profile's content (e.g., checking that the returned profile has
age: 18)Example additions:
// Negative case: no match await expect(db.$setAuth({ id: 1, age: 25 }).profile.findMany()).resolves.toHaveLength(0); await expect(db.$setAuth({ id: 1, age: 25 }).profile.count()).resolves.toBe(0); // Verify returned profile content const profiles = await db.$setAuth({ id: 1, age: 18 }).profile.findMany(); expect(profiles[0].age).toBe(18);tests/regression/test/v2-migrated/issue-1520.test.ts (1)
4-37: Consider verifying thetypediscriminator field.The schema uses
@@delegate (type)for polymorphic behavior, but the test doesn't explicitly verify that thetypefield is correctly set to identify the subtype. While the test focuses oncourseIdmapping (aligning with the PR's column name mapping fix), confirming the discriminator value would strengthen the regression coverage.Consider adding a
typefield check in the assertion:expect(r.addedToNotifications).toEqual( expect.arrayContaining([ expect.objectContaining({ id: 1, courseId: 1, receiverId: 1, senderId: 2, + type: 'AddedToCourseNotification', }), ]), );tests/regression/test/v2-migrated/issue-1562.test.ts (1)
4-22: Well-structured regression test for validation with type composition.The test correctly sets up a scenario with type inheritance (
model User with Base) and validates that the regex constraint is enforced. The schema definition is clear and appropriate for testing the validation behavior mentioned in issue 1562.Consider adding a brief comment explaining what issue 1562 was about for future maintainers:
+// Issue 1562: Validation should work correctly when fields are defined +// in models that use type composition (delegates) it('verifies issue 1562', async () => {tests/regression/test/v2-migrated/issue-1506.test.ts (1)
4-4: Document what issue 1506 was and what this test verifies.Add a comment explaining the original issue and what behavior this test validates. This helps maintainers understand the test's purpose and prevents regressions.
For example:
+// Issue 1506: Policy evaluation failed when accessing related fields through delegates +// due to incorrect column name mapping in the generated SQL queries it('verifies issue 1506', async () => {tests/regression/test/v2-migrated/issue-1576.test.ts (1)
35-61: Test logic is correct and validates the delegation fix.The test properly verifies that
createManyAndReturnworks with delegated models and that thetypefield is correctly populated. The assertions confirm that both items are created with the expected properties.Optional: Consider adding explicit count verification.
For additional robustness, you could verify the exact count of returned items:
+ const result = await db.goldItem.createManyAndReturn({ - await expect( - db.goldItem.createManyAndReturn({ data: [ { profileId: profile.id, inventory: true, }, { profileId: profile.id, inventory: true, }, ], - }), - ).resolves.toEqual( + }); + + expect(result).toHaveLength(2); + expect(result).toEqual( expect.arrayContaining([ expect.objectContaining({ profileId: profile.id, type: 'GoldItem', inventory: true }), expect.objectContaining({ profileId: profile.id, type: 'GoldItem', inventory: true }), ]), );This makes it explicit that exactly 2 items should be returned, though the current assertion is sufficient for this regression test.
tests/regression/test/v2-migrated/issue-1627.test.ts (1)
48-48: LGTM!The assertion correctly validates that without an auth context, the access control policy blocks read access and returns zero results. This effectively tests the column name mapping fix for delegates in policy evaluation.
Optionally, consider adding a complementary positive test case that verifies the access control allows reads when proper auth context is provided:
// Add after line 48 await expect( db.$use({ user: { id: '1' } }).gymUser.findMany() ).resolves.toHaveLength(1);This would provide more comprehensive coverage of the access control behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
packages/runtime/src/client/crud/dialects/base-dialect.ts(9 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(1 hunks)packages/runtime/src/client/crud/operations/aggregate.ts(2 hunks)packages/runtime/src/client/crud/operations/base.ts(1 hunks)packages/runtime/src/client/crud/operations/count.ts(1 hunks)packages/runtime/src/client/executor/name-mapper.ts(4 hunks)tests/e2e/orm/client-api/computed-fields.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1235.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1506.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1507.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1518.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1520.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1522.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1530.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1533.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1551.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1562.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1563.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1574.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1575.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1576.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1585.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1627.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1642.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1644.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1645.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-1562.test.tstests/e2e/orm/client-api/computed-fields.test.tstests/regression/test/v2-migrated/issue-1518.test.tspackages/runtime/src/client/executor/name-mapper.tspackages/runtime/src/client/crud/dialects/postgresql.tspackages/runtime/src/client/crud/operations/count.tspackages/runtime/src/client/crud/dialects/base-dialect.tstests/regression/test/v2-migrated/issue-1551.test.tspackages/runtime/src/client/crud/operations/aggregate.tstests/regression/test/v2-migrated/issue-1576.test.tstests/regression/test/v2-migrated/issue-1522.test.tstests/regression/test/v2-migrated/issue-1563.test.tspackages/runtime/src/client/crud/operations/base.tstests/regression/test/v2-migrated/issue-1530.test.tstests/regression/test/v2-migrated/issue-1575.test.tstests/regression/test/v2-migrated/issue-1507.test.tstests/regression/test/v2-migrated/issue-1574.test.tstests/regression/test/v2-migrated/issue-1506.test.tstests/regression/test/v2-migrated/issue-1642.test.tstests/regression/test/v2-migrated/issue-1520.test.tstests/regression/test/v2-migrated/issue-1645.test.tstests/regression/test/v2-migrated/issue-1644.test.tstests/regression/test/v2-migrated/issue-1235.test.tstests/regression/test/v2-migrated/issue-1533.test.tstests/regression/test/v2-migrated/issue-1627.test.tstests/regression/test/v2-migrated/issue-1585.test.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/client-api/computed-fields.test.ts
🧬 Code graph analysis (16)
tests/regression/test/v2-migrated/issue-1562.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1518.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1576.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1522.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1563.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1530.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1507.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1574.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1506.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1642.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1520.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1645.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(176-187)
tests/regression/test/v2-migrated/issue-1644.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1533.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1627.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
tests/regression/test/v2-migrated/issue-1585.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(51-166)
🪛 GitHub Actions: Build and Test
packages/runtime/src/client/crud/operations/aggregate.ts
[error] 1-1: TS6133: 'sql' is declared but its value is never read.
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (26)
tests/regression/test/v2-migrated/issue-1551.test.ts (1)
1-2: LGTM!The imports are correct for a ZenStack regression test using Vitest.
tests/regression/test/v2-migrated/issue-1235.test.ts (2)
1-1: LGTM!The removal of
testLoggerimport is a reasonable cleanup that simplifies the test setup. This aligns with potential API changes in the@zenstackhq/testtoolsalpha version.
6-14: LGTM!The removal of the
testLoggerparameter fromcreatePolicyTestClientcalls is consistent with the import change and appears to be an API simplification. The test functionality is fully preserved - both regression tests still correctly validate that updating ID fields throws the expected error.Also applies to: 23-31
tests/regression/test/v2-migrated/issue-1507.test.ts (2)
1-2: LGTM!Imports are appropriate for a policy-based regression test.
7-10: Verify if the User model is necessary.The User model is defined but never instantiated or referenced in the test. The
$setAuth({ id: 1, age: 18 })call works without creating an actual User record. Consider removing the User model definition if it's not required for this test case, as it may cause confusion about the test's requirements.packages/runtime/src/client/crud/dialects/postgresql.ts (1)
234-234: LGTM: Consistent switch to instance-bound expression builder.The change from
sql.ref(left)tothis.eb.ref(left)aligns with the PR's objective to fix column name mapping issues for delegate models. Using the instance-bound expression builder (this.eb) instead of the globalsqlallows proper column resolution and transformations when dealing with polymorphic inheritance hierarchies.packages/runtime/src/client/crud/operations/base.ts (1)
1543-1545: LGTM: Proper use of callback-scoped expression builder.The change correctly uses
eb.ref(...)whereebis the callback parameter from.where((eb) => ...). This follows Kysely's pattern of using the callback-scoped expression builder for references within query callbacks, ensuring proper column name resolution for delegate models.packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
105-105: LGTM: Comprehensive and consistent refactor to instance-bound references.The systematic replacement of
sql.ref(...)withthis.eb.ref(...)across all query-building paths (distinctOn, filters, joins, orderBy, field selection) is well-executed. This enables proper column name mapping for delegate models by using the instance-bound expression builder, which can apply necessary transformations based on the schema and model context.The changes are consistent and cover all relevant code paths without altering the query logic.
Also applies to: 251-251, 271-273, 336-336, 356-364, 375-389, 846-862, 951-951
tests/regression/test/v2-migrated/issue-1520.test.ts (2)
1-3: LGTM!Standard test imports are correctly configured.
39-55: Add assertions confirming inferredcourseIdand subtype for nested createMany
After creating the course, assert thatr.addedToNotifications[0].courseId === r.idand that the notification instance is of typeAddedToCourseNotification.tests/regression/test/v2-migrated/issue-1533.test.ts (2)
4-4: Clarify the TODO comment status.The TODO mentions "JSON null support", but the test asserts specific behavior for JSON null handling. Please clarify:
- Is JSON null support now complete and this TODO should be removed?
- Should this test be marked as skipped or expected to fail until support is complete?
- Does the TODO refer to additional functionality beyond what this test covers?
5-53: Test structure and logic look good.The test effectively verifies JSON path queries with both null and string values. The setup creates appropriate test data, and the assertions correctly validate the expected behavior.
Note: This approval is contingent on fixing the missing
Prismaimport flagged in the previous comment.tests/regression/test/v2-migrated/issue-1562.test.ts (2)
24-24: Correct validation test logic.The test correctly validates that a name with spaces ("1 2 3 4") is rejected by the regex pattern
^[a-zA-Z0-9_]{3,30}$, which only allows alphanumeric characters and underscores.
1-2: Custom matcher availability verified—no import needed.
toBeRejectedByValidation is implemented and registered in @zenstackhq/testtools (vitest-ext.ts).tests/regression/test/v2-migrated/issue-1644.test.ts (1)
1-24: Approve regression test
Field-level policy syntax@allow('read', auth() == this)is valid and consistent with other v2-migrated tests. Test logic and skip annotation are appropriate.tests/regression/test/v2-migrated/issue-1576.test.ts (1)
1-33: LGTM! Well-structured regression test.The test setup is clear and appropriate:
- Schema correctly models delegation using
@@delegate(type)for both Profile/GoldProfile and Item/GoldItem hierarchies- Relation between Item and Profile via
profileIdis properly defined- Test client creation follows standard patterns
This effectively reproduces the scenario for testing column name mapping with delegates.
tests/regression/test/v2-migrated/issue-1574.test.ts (5)
1-5: LGTM! Appropriate test structure and skip marker.The imports are correct, and using
it.skipwith a clear TODO comment is the right approach for tests awaiting feature implementation.
6-52: LGTM! Well-designed schema for policy testing.The schema effectively tests the interaction between field-level policies (line 22:
namefield with read-all override) and model-level policies (line 28: owner-only access). The use of both optional (ModelB) and required (ModelC) references to ModelA provides good coverage of different access patterns.
54-67: LGTM! Proper test setup pattern.The setup correctly uses
$unuseAll()to bypass policies during data creation and$setAuth()to create authenticated client instances for each user. This follows the established pattern for policy testing.
69-83: Verify null-safety of nested property access.Lines 71, 75, and 79 access
ref.namedirectly without null checks. If the query returnsnullor therefobject is missing, this will throw a TypeError rather than fail the assertion gracefully.Consider adding optional chaining or explicit null checks:
const t1 = await db1.modelB.findFirst({ select: { ref: true } }); -expect(t1.ref.name).toBeTruthy(); +expect(t1?.ref?.name).toBeTruthy();Or assert the parent object first:
const t1 = await db1.modelB.findFirst({ select: { ref: true } }); +expect(t1.ref).toBeTruthy(); expect(t1.ref.name).toBeTruthy();
89-95: Verify null-safety of nested property access.Similar to lines 71, 75, 79, lines 91 and 95 access
ref.namedirectly without null checks, which could throw ifrefis missing or null.Apply the same null-safety pattern as suggested for the earlier assertions.
tests/regression/test/v2-migrated/issue-1627.test.ts (3)
1-2: LGTM!Standard test imports are correct and appropriate for a regression test using ZenStack testtools and Vitest.
33-46: LGTM!Test data setup correctly uses
$unuseAll()to bypass policies during data creation. The nested create properly establishes the Gym↔GymUser relation, with the foreign key automatically set through the ORM.
4-31: Approve regression test schema
Access rule usesuser == auth()syntax consistent with existing tests (issue-689); no change needed.tests/regression/test/v2-migrated/issue-1522.test.ts (2)
47-85: LGTM!The test setup and query logic are well-structured for a regression test. The nested create, followed by a separate create for
TaskActivity, and then a retrieval with nested includes and consistent ordering (byidx) appropriately exercises the delegate column name mapping scenario described in the PR objectives.
16-43: Explicit @@Allow policies aren’t required for Section, Activity, UrlActivity, or TaskActivity—defaults permit access and this matches other v2-migrated tests.
|
Claude finished @ymc9's task —— View job Code Review CompleteI've thoroughly reviewed this PR that fixes delegate column name mapping issues. Here's my comprehensive feedback: Todo List:
SummaryThis PR addresses critical issues with column name mapping when working with delegate models in ZenStack's polymorphic inheritance system. The changes are well-targeted and fix legitimate bugs around field resolution and selection. Code Quality & Best Practices ✅Strengths:
Specific improvements:
Bug Analysis ✅Issues Fixed:
No new bugs identified - the changes are defensive and maintain backward compatibility. Performance Considerations ✅Positive impacts:
No performance regressions - the added logic only executes for delegate models and has minimal overhead. Security Assessment ✅No security concerns identified:
Test Coverage Assessment ✅Excellent test coverage:
Key test scenarios:
RecommendationsMinor Improvements:
Code Examples:The fix in // Before: Incorrect model context for delegate fields
return query.select(this.eb.ref(`${modelAlias}.${field}`).as(field));
// After: Proper recursive handling for delegate origin models
return this.buildSelectField(query, fieldDef.originModel, fieldDef.originModel, field);Verdict: APPROVED ✅This is a solid fix for legitimate bugs in delegate model handling. The changes are:
The delegate column name mapping issues were clearly problematic for users working with polymorphic inheritance, and this PR resolves them effectively. |
|
Claude finished @ymc9's task —— View job Code Review CompleteI've thoroughly reviewed this PR that fixes delegate column name mapping issues. Here's my comprehensive feedback: Todo List:
SummaryThis PR addresses critical issues with column name mapping when working with delegate models in ZenStack's polymorphic inheritance system. The changes are well-targeted and fix legitimate bugs around field resolution and selection. Code Quality & Best Practices ✅Strengths:
Specific improvements:
Bug Analysis ✅Issues Fixed:
No new bugs identified - the changes are defensive and maintain backward compatibility. Performance Considerations ✅Positive impacts:
No performance regressions - the added logic only executes for delegate models and has minimal overhead. Security Assessment ✅No security concerns identified:
Test Coverage Assessment ✅Excellent test coverage:
Key test scenarios:
RecommendationsMinor Improvements:
Code Examples:The fix in // Before: Incorrect model context for delegate fields
return query.select(this.eb.ref(`${modelAlias}.${field}`).as(field));
// After: Proper recursive handling for delegate origin models
return this.buildSelectField(query, fieldDef.originModel, fieldDef.originModel, field);Verdict: APPROVED ✅This is a solid fix for legitimate bugs in delegate model handling. The changes are:
The delegate column name mapping issues were clearly problematic for users working with polymorphic inheritance, and this PR resolves them effectively. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/regression/test/v2-migrated/issue-1533.test.ts (1)
1-3: MissingPrismaimport remains unresolved.The previously flagged issue persists: line 34 references
Prisma.DbNull, but thePrismanamespace is not imported. This will cause aReferenceErrorwhen the test is un-skipped.Apply the fix from the previous review:
import { createTestClient } from '@zenstackhq/testtools'; +import { Prisma } from '@prisma/client'; import { expect, it } from 'vitest';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/regression/test/v2-migrated/issue-1533.test.ts(1 hunks)tests/regression/test/v2-migrated/issue-1627.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/regression/test/v2-migrated/issue-1627.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
tests/regression/test/v2-migrated/issue-1533.test.ts
🧬 Code graph analysis (1)
tests/regression/test/v2-migrated/issue-1533.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). (2)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
Summary by CodeRabbit
Refactor
Tests
Bug Fixes