fix(orm): foreign key name mapping is not properly process in join #400
Conversation
WalkthroughThis PR extends join processing in the query name mapper with scope awareness for improved from-clause table references, adds automatic default Prisma client generator configuration with extension support, significantly expands test client options to support file operations and datasource extensions, introduces a comprehensive Rally polling application end-to-end test suite with full ZenStack model definitions covering users, spaces, polls, events, billing, and licensing, and updates code generation scripts to scan application-level schemas alongside existing test schemas. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 CompleteReviewing PR #400: Foreign Key Name Mapping Fix Review Tasks:
SummaryThis PR fixes a critical bug in the ORM's name mapping functionality where foreign key references in JOIN conditions weren't being properly resolved due to missing scope context. Code Quality & Architecture✅ Core Fix (name-mapper.ts:80-84)The fix addresses a scoping issue in // OLD: Join processing without FROM table scopes
const processedJoins = (node.joins ?? []).map((join) => this.processSelectTable(join.table));
// NEW: Join processing with FROM table scopes available
const processedJoins = this.withScopes([...processedFroms.map(({ scope }) => scope)], () =>
(node.joins ?? []).map((join) => this.processSelectTable(join.table)),
);Impact: When processing JOIN conditions, the ✅ Schema Generator Enhancement (prisma-schema-generator.ts:109-113)Automatically adds default Prisma client generator when missing: if (!this.zmodel.declarations.some(isGeneratorDecl)) {
this.generateDefaultGenerator(prisma);
}Benefits:
✅ Test Infrastructure Improvements (testtools/client.ts)Significant enhancements to test setup capabilities:
Test Coverage✅ Comprehensive Testing
✅ Test Quality
Potential Issues
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where foreign key name mapping was not properly handled in JOIN clauses. The issue was that when processing JOIN conditions, the scope from FROM clauses wasn't available, causing field name resolution to fail when field names were mapped using @map.
Key Changes
- Updated
QueryNameMapper.transformSelectQueryto make FROM table scopes available when processing JOIN clauses - Added
@mapattributes to primary key fields in the name-mapping test schema to test the fix - Added test cases to verify JOIN queries work correctly with mapped field names
- Enhanced test infrastructure with new Rally app test setup and improved test client options
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/orm/src/client/executor/name-mapper.ts | Core fix: Added FROM scopes when processing JOINs |
| tests/e2e/orm/client-api/name-mapping.test.ts | Added test cases for nested selects and joins with mapped IDs |
| tests/e2e/orm/schemas/name-mapping/schema.zmodel | Added @Map to id fields to test the fix |
| packages/testtools/src/client.ts | Enhanced with copyFiles, dataSourceExtensions, and better documentation |
| packages/sdk/src/prisma/prisma-schema-generator.ts | Auto-generate default generator with postgresqlExtensions preview feature |
| tests/e2e/apps/rally/* | New Rally app test infrastructure with complex schema |
💡 Add Copilot custom instructions for smarter, more guided reviews. 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: 0
🧹 Nitpick comments (5)
packages/testtools/src/client.ts (1)
168-184: Verify expect.getState() availability in all test contexts.The
copyFilesimplementation relies onexpect.getState()to determine the current test file path. While this works with Vitest's expect API, verify thattestPathis reliably available in all test execution modes (e.g., parallel execution, different test runners).Consider adding more context to the error message if testPath is unavailable:
if (!currentTestPath) { - throw new Error('Unable to determine current test file path'); + throw new Error('Unable to determine current test file path. The copyFiles option requires running within a Vitest test context.'); }tests/e2e/apps/rally/rally.test.ts (2)
10-24: Consider adding teardown to disconnect the test client
beforeEachcreates a newClientContractwith a Postgres dialect, but there's no correspondingafterEach/afterAllcallingdb.$disconnect(). Over time (or when you add more tests), this can leave open DB connections and keep Node’s event loop alive longer than necessary.I recommend mirroring the pattern used in other e2e tests and adding a teardown:
import { afterEach } from 'vitest'; afterEach(async () => { await db.$disconnect(); });
26-48: VerifytoResolveTruthymatcher or prefer standard.resolves.toBeTruthy()
await expect(db.spaceMember.findMany({...})).toResolveTruthy();is a bit unconventional for Vitest’s core API, which typically usesawait expect(promise).resolves.toBeTruthy()(or similar). If you’ve registered a custom matcher providingtoResolveTruthy, that’s fine—otherwise this will throw at runtime.If there’s no explicit custom matcher, I’d switch to:
await expect( db.spaceMember.findMany({ // ... }), ).resolves.toBeTruthy();Functionally it still asserts that the query resolves without error and returns a truthy value.
tests/e2e/apps/rally/zenstack/models/integrations.zmodel (2)
24-25: Consider avoiding duplicate constraint name across tablesBoth
Credential(Line 24) andCalendarConnection(Line 48) use@@unique([userId, provider, providerAccountId], name: "user_provider_account_unique"). Reusing the same constraint name across different tables can cause conflicts on databases where constraint names must be schema‑unique. Unless you’re intentionally mirroring an existing schema that already does this, consider either dropping the explicitname:(and letting the tooling generate one) or giving each table its own unique constraint name.Also applies to: 48-49
72-75: Clarify semantics oflastSyncedAtdefault
lastSyncedAtis non‑nullable with@default(now()). That makes a newly createdProviderCalendarlook as if it has already been synced once. If “never synced” is a meaningful state, you might preferDateTime?without a default and treatnullas “not yet synced”.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/orm/src/client/executor/name-mapper.ts(1 hunks)packages/sdk/src/prisma/prisma-schema-generator.ts(3 hunks)packages/testtools/src/client.ts(4 hunks)tests/e2e/apps/rally/rally.test.ts(1 hunks)tests/e2e/apps/rally/zenstack/input.ts(1 hunks)tests/e2e/apps/rally/zenstack/models.ts(1 hunks)tests/e2e/apps/rally/zenstack/models/billing.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/event.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/instance-settings.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/integrations.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/licensing.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/poll.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/space.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/models/user.zmodel(1 hunks)tests/e2e/apps/rally/zenstack/schema.zmodel(1 hunks)tests/e2e/orm/client-api/name-mapping.test.ts(2 hunks)tests/e2e/orm/schemas/name-mapping/schema.ts(2 hunks)tests/e2e/orm/schemas/name-mapping/schema.zmodel(1 hunks)tests/e2e/package.json(1 hunks)tests/e2e/scripts/generate.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
tests/e2e/apps/rally/zenstack/schema.zmodeltests/e2e/orm/schemas/name-mapping/schema.zmodeltests/e2e/apps/rally/zenstack/models/user.zmodeltests/e2e/orm/schemas/name-mapping/schema.tstests/e2e/apps/rally/zenstack/models/licensing.zmodeltests/e2e/apps/rally/zenstack/models.ts
🧬 Code graph analysis (4)
tests/e2e/apps/rally/rally.test.ts (3)
packages/orm/src/client/contract.ts (1)
ClientContract(52-172)tests/e2e/apps/rally/zenstack/schema.ts (2)
SchemaType(2523-2523)schema(9-2522)packages/testtools/src/client.ts (1)
createTestClient(95-244)
tests/e2e/orm/schemas/name-mapping/schema.ts (1)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)
packages/testtools/src/client.ts (2)
packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/orm/src/client/options.ts (1)
ClientOptions(45-96)
packages/sdk/src/prisma/prisma-schema-generator.ts (2)
packages/language/src/generated/ast.ts (2)
isGeneratorDecl(516-518)isDataSource(417-419)packages/sdk/src/prisma/prisma-builder.ts (1)
PrismaModel(11-52)
⏰ 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). (4)
- GitHub Check: Agent
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (24)
tests/e2e/scripts/generate.ts (2)
11-14: LGTM! Expanded schema scanning aligns with test suite enhancements.The addition of
apps/**/schema.zmodelto the generation sources enables scanning application-level schemas alongside existing ORM test schemas, which supports the new Rally e2e test suite introduced in this PR.
29-29: Plugin path change is correct; runtime verification needed.The review comment's technical assessment is accurate: the file was moved from
tests/e2e/orm/scripts/generate.tstotests/e2e/scripts/generate.ts, and the plugin path correctly changed from../../node_modules(two levels up) to../node_modules(one level up). Both resolve totests/e2e/node_modules/@zenstackhq/plugin-policy/plugin.zmodel.Since
@zenstackhq/plugin-policyis listed intests/e2e/package.jsonas a workspace dependency ("workspace:*"), pnpm should resolve it to node_modules at install time. However, the node_modules structure is not available in this environment, so runtime verification is needed to confirm the path resolves correctly when the script executes.tests/e2e/package.json (1)
8-8: LGTM! Script path update aligns with file relocation.The test:generate script now points to the correct location after the generate.ts file was moved from
orm/scripts/toscripts/.packages/orm/src/client/executor/name-mapper.ts (1)
80-84: Excellent fix for scope-aware join processing!This change correctly makes from-clause scopes available when processing join tables. Previously, join conditions couldn't properly resolve references to tables defined in the FROM clause, causing name mapping issues for foreign keys in joins. By wrapping join processing in
withScopes([...processedFroms.map(({ scope }) => scope)]), join conditions can now correctly reference and map names for from-clause tables.packages/sdk/src/prisma/prisma-schema-generator.ts (2)
110-112: Verify behavioral implications of auto-generating default generator.Automatically adding a default Prisma client generator when none is declared changes the behavior for schemas without explicit generators. While this is likely the desired behavior for most use cases, confirm that this won't break existing workflows that intentionally omit generator declarations.
177-184: PostgreSQL extensions preview feature name is correct.The preview feature name "postgresqlExtensions" is confirmed as correct per Prisma's official documentation. The code at lines 177-184 properly implements this feature name and requires no changes.
tests/e2e/orm/schemas/name-mapping/schema.zmodel (1)
14-14: LGTM! Test data enhancements for join name mapping.Adding
@mapattributes to ID fields creates the test scenario needed to verify that foreign key references in join conditions correctly resolve mapped identifier names. This directly tests the scope-aware join processing fix.Also applies to: 22-22
tests/e2e/apps/rally/zenstack/models/event.zmodel (2)
22-49: LGTM! Well-structured event model with comprehensive relationships.The ScheduledEvent model includes proper foreign key constraints, cascading deletes, appropriate indexes (Hash type for PostgreSQL optimization), and comprehensive field mappings. The relations to User, Space, and related entities are correctly defined.
65-83: LGTM! ScheduledEventInvite model follows best practices.The model correctly implements:
- Optional relation to User with SetNull on delete (line 77), which is appropriate for invite tracking even after user deletion
- Proper cascading delete for the scheduledEvent relation (line 76)
- Well-indexed foreign keys for query performance (lines 79-81)
packages/testtools/src/client.ts (2)
35-85: LGTM! Comprehensive expansion of test client options.The expanded options interface provides valuable capabilities:
dbFile: Enables reusing existing SQLite databasesdataSourceExtensions: Supports PostgreSQL extensions in testscopyFiles: Allows copying additional test fixturesThe option definitions are well-documented and type-safe.
140-140: LGTM! Extension support integrates with generator changes.The conditional insertion of
extensions = [...]into the datasource block correctly integrates with the default generator'spostgresqlExtensionspreview feature added inpackages/sdk/src/prisma/prisma-schema-generator.ts.tests/e2e/apps/rally/zenstack/schema.zmodel (1)
1-12: LGTM! Schema integrates Rally e2e test models.This schema file serves as the entry point for the comprehensive Rally application test suite, importing all necessary model definitions. The datasource is intentionally minimal since the URL is injected by the test tooling (as seen in
packages/testtools/src/client.ts).tests/e2e/orm/client-api/name-mapping.test.ts (2)
160-167: Nested include test correctly exercises FK mappingThis
include: { posts: { where: { title: 'Post1' } } }plus theauthorId: user.idassertion is a good, focused regression test for relation/foreign-key name mapping on nested includes. No issues from my side; this should catch regressions in how mapped FK names are resolved inside include filters and payloads.
211-221: Alternative join variant provides solid coverage for join callback pathThis
innerJoin('User', (join) => join.onRef('User.id', '=', 'Post.authorId'))mirrors the previous positional join test and properly assertsauthorIdandtitle. It exercises the join-callback path of the name mapper, which is exactly where the bug was. Looks good.tests/e2e/orm/schemas/name-mapping/schema.ts (1)
17-23: ID field@mapattributes align with underlying column namesAdding
@map("user_id")toUser.idand@map("post_id")toPost.idin the generated schema is consistent with your table naming (users.user_id,posts.post_id) and ensures the name-mapper sees the correct PK column names for joins. Looks coherent with the rest of the mapped fields (email, role, authorId).Also applies to: 55-61
tests/e2e/apps/rally/zenstack/models/instance-settings.zmodel (1)
1-10: InstanceSettings model looks consistent for a singleton settings tableThe
InstanceSettingsdefinition (fixed PKid @default(1), mapped snake_case columns,@updatedAtonupdatedAt) is coherent and matches the shape of a one-row instance-level settings table. No issues spotted.tests/e2e/apps/rally/zenstack/models/space.zmodel (1)
1-73: Space and membership models/enums are well-structured and align with Rally testsThe
Space,SpaceMember, andSpaceMemberInvitemodels plusSpaceMemberRole/SpaceTierenums are wired cleanly:
- FK fields (
ownerId,spaceId,userId,inviterId) and timestamps are correctly mapped to snake_case columns.- Relations specify
onDelete: Cascadeand unique/index constraints that match expected access patterns.SpaceMemberexposesuserId,lastSelectedAt, andspaceexactly as used inrally.test.ts.This schema should work well with the Postgres-backed Rally e2e tests and the name-mapping logic.
tests/e2e/apps/rally/zenstack/models/integrations.zmodel (1)
7-27: Integration models and relations look structurally soundThe
Credential,CalendarConnection, andProviderCalendarmodels line up well withUserand with each other: foreign keys use the mapped scalar fields, relation names are consistent (includingUserDefaultDestinationCalendar), and table/column mappings are coherent. This should give good coverage for join/name‑mapping behavior.Also applies to: 29-50, 52-87
tests/e2e/apps/rally/zenstack/models/billing.zmodel (1)
4-27: Billing schema is consistent and relationally well-formed
Subscription,SubscriptionStatus,SubscriptionInterval, andPaymentMethodare wired cleanly: FKs toUser/Spaceuse mapped IDs with cascade semantics, indices cover main lookup keys, and table/column mappings are coherent. Nothing blocking here from a schema/relations standpoint.Also applies to: 29-47, 49-60
tests/e2e/apps/rally/zenstack/models/user.zmodel (1)
65-92: User–calendar and session relations are coherently wiredThe
Usermodel’s relations toSubscription,Space,CalendarConnection,Credential, andProviderCalendar(Lines 65–87) all line up with the corresponding models: relation names match (UserToSubscription,UserSpaces,UserDefaultDestinationCalendar), and the mapped FK fields (default_destination_calendar_id, etc.) are used consistently. Likewise, the impersonation setup betweenSession.impersonatedByUserandUser.impersonatedSessionsvia"ImpersonatedSessions"is correctly paired. No changes needed here.Also applies to: 81-87, 103-119
tests/e2e/apps/rally/zenstack/models/poll.zmodel (1)
5-19: Poll domain schema is well-structured for relational queriesThe
Pollgraph (Poll,Watcher,Participant,Option,Vote,Comment,PollView) is modeled cleanly: FKs and@mapusages match the relations, delete behaviors (Cascade/SetNull) are sensible for lifecycle, and the indexes cover primary join and lookup paths (pollId,userId,spaceId, etc.). This should serve well for exercising complex joined queries in the e2e tests.Also applies to: 21-172
tests/e2e/apps/rally/zenstack/models/licensing.zmodel (1)
1-54: Licensing models and mappings look clean
License,LicenseValidation, andInstanceLicenseare straightforward: enums are used consistently, FK fromLicenseValidationtoLicenseis correctly defined with cascade delete, and table/column mappings are coherent. No changes needed from a schema perspective.tests/e2e/apps/rally/zenstack/models.ts (1)
1-64: Generated model and enum surface looks consistentThe imports, model
$ModelResult<$Schema, "...">aliases, and enum value/union exports all follow a consistent pattern and match what we’d expect from the generator. Nothing to fix here.tests/e2e/apps/rally/zenstack/input.ts (1)
1-550: Generated input/args typings are structurally soundThe arg/input aliases and
GetPayloaddefinitions are uniformly applied across all models, with consistent use of$Schema, generic arg helpers, and$SimplifiedModelResult. For a generated surface this looks correct and complete.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests