fix(cli): add missing opposite relation fields during db pull when multiple FKs target the same model#2652
fix(cli): add missing opposite relation fields during db pull when multiple FKs target the same model#2652svetch wants to merge 2 commits intozenstackhq:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes improve database-pull field reconciliation by introducing a helper function to extract relation names from ChangesRelation-name reconciliation for DB pull
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
Fixes db pull merge behavior so that named opposite-side relation fields (back-references) are no longer dropped when multiple foreign keys point to the same target model, ensuring schemas are fully restored/preserved in these multi-FK scenarios.
Changes:
- Added extraction of
@relationpositional relation name (getRelationName) to enable reliable matching of back-reference fields. - Updated the field matching priority during
db pullmerge to include relation-name-based matching and removed the blanket skip for unmatched back-references. - Added regression tests covering restore/preserve behavior when multiple models reference the same target model via multiple FKs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/cli/test/db/pull.test.ts | Adds regression tests to ensure opposite relation fields are restored/preserved when multiple FKs target the same model. |
| packages/cli/src/actions/pull/utils.ts | Introduces getRelationName helper to read the first positional argument of @relation. |
| packages/cli/src/actions/db.ts | Extends merge matching logic to use relation-name matching and allows adding previously skipped named back-references. |
💡 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.
🧹 Nitpick comments (2)
packages/cli/test/db/pull.test.ts (1)
195-230: 💤 Low valueTest placement: "preserve" test sits under "Pull from zero" describe.
This test does not zero out the schema between push and pull, so semantically it belongs to the "Pull with existing schema - preserve schema features" describe block (Line 330) rather than "Pull from zero - restore complete schema from database" (Line 11). The first new test (Lines 155-193) is correctly placed under "Pull from zero". Reorganizing improves discoverability and aligns with the existing pattern (e.g., the analogous "preserve" test for self-referencing models at Lines 132-153 has the same minor placement issue, so this is consistent — but worth fixing for both).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/db/pull.test.ts` around lines 195 - 230, Move the "should preserve opposite relation fields when multiple models have FKs to the same target" test out of the "Pull from zero" describe and into the "Pull with existing schema - preserve schema features" describe block (i.e., relocate the entire it(...) block so it runs alongside the other "preserve" tests); ensure you also relocate the analogous self-referencing "preserve" test if present so both preservation tests live under the same describe, keeping the test name and behavior unchanged (look for the it(...) with that exact description and the matching test that checks self-referencing models).packages/cli/src/actions/pull/utils.ts (1)
131-137: 💤 Low valueMinor: redundant
as StringLiteralcast.The check on Line 135 already narrows
firstPositionalArg.valuetoStringLiteral, so the cast on Line 136 is unnecessary. Not a functional issue.Optional cleanup
export function getRelationName(decl: DataField): string | undefined { const relationAttr = decl?.attributes?.find((a) => a.decl?.ref?.name === '@relation'); if (!relationAttr) return undefined; const firstPositionalArg = relationAttr.args.find((a) => !a.name); if (!firstPositionalArg || firstPositionalArg.value?.$type !== 'StringLiteral') return undefined; - return (firstPositionalArg.value as StringLiteral).value; + return firstPositionalArg.value.value; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/actions/pull/utils.ts` around lines 131 - 137, In getRelationName, remove the redundant type cast "as StringLiteral" on the return line: after you already checked firstPositionalArg.value?.$type === 'StringLiteral', directly access and return firstPositionalArg.value.value (or assign firstPositionalArg.value to a const typed variable for clarity) so the explicit cast is unnecessary; update the return accordingly in the getRelationName function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/actions/pull/utils.ts`:
- Around line 131-137: In getRelationName, remove the redundant type cast "as
StringLiteral" on the return line: after you already checked
firstPositionalArg.value?.$type === 'StringLiteral', directly access and return
firstPositionalArg.value.value (or assign firstPositionalArg.value to a const
typed variable for clarity) so the explicit cast is unnecessary; update the
return accordingly in the getRelationName function.
In `@packages/cli/test/db/pull.test.ts`:
- Around line 195-230: Move the "should preserve opposite relation fields when
multiple models have FKs to the same target" test out of the "Pull from zero"
describe and into the "Pull with existing schema - preserve schema features"
describe block (i.e., relocate the entire it(...) block so it runs alongside the
other "preserve" tests); ensure you also relocate the analogous self-referencing
"preserve" test if present so both preservation tests live under the same
describe, keeping the test name and behavior unchanged (look for the it(...)
with that exact description and the matching test that checks self-referencing
models).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbfb51ad-0e00-4e56-b68a-78e5be718f3b
📒 Files selected for processing (3)
packages/cli/src/actions/db.tspackages/cli/src/actions/pull/utils.tspackages/cli/test/db/pull.test.ts
|
Hi @svetch , the build failure was likely caused by an old issue on dev branch. Could you merge from upstream and push? Thanks! |
svetch
force-pushed
the
dev
branch
2 times, most recently
from
4e971db to
3dc0c97
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/actions/db.ts (1)
380-389: ⚡ Quick winFilter relation-name matches by owning-vs-back-reference kind.
getRelationNamereturns the same string for both sides of a relation, and in self-relations both sides live in the same model. If the new field is a back-reference and steps 1–3 don't match (e.g. the user renamed a self-relation field), step 4 will collect both the owning (fields:present) and back-reference original fields, hitting the multi-match branch at line 413 which silently skips back-reference fields and prevents step 6 (findOriginalBackReferenceFields) from running becauseoriginalFields.lengthis no longer 0. Net effect: existing back-reference attributes silently fail to merge in this edge case.Adding a kind filter restricts the relation-name candidates to fields of the same side as
fand avoids the cross-side collision. The same correction should be applied to the deletion check at lines 610–617 so an original back-reference isn't preserved by accidentally matching the new owning field of the same self-relation.♻️ Proposed filter for both match sites
if (originalFields.length === 0) { // Try matching by relation name (the first positional arg in `@relation`) // This is essential for back-reference fields that only have a relation name const newRelName = getRelationName(f as any); if (newRelName) { originalFields = originalDataModel.fields.filter( - (d) => d.$type === 'DataField' && getRelationName(d as any) === newRelName, + (d) => + d.$type === 'DataField' && + getRelationName(d as any) === newRelName && + !!getRelationFieldsKey(d as any) === !!getRelationFieldsKey(f as any), ); } }// Try matching by relation name (for named back-reference fields) const originalRelName = getRelationName(f as any); if (originalRelName) { const matchByRelName = newDataModel.fields.find( - (d) => d.$type === 'DataField' && getRelationName(d as any) === originalRelName, + (d) => + d.$type === 'DataField' && + getRelationName(d as any) === originalRelName && + !!getRelationFieldsKey(d as any) === !!getRelationFieldsKey(f as any), ); if (matchByRelName) return false; }Also applies to: 610-617
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/actions/db.ts` around lines 380 - 389, The relation-name match is including both owning and back-reference sides because getRelationName returns the same string for both; update the filter that sets originalFields (the block that uses originalDataModel.fields.filter and getRelationName for f) to also check the field kind so only fields with the same side as the new field f are considered (e.g., require the candidate d.$type/kind to match f's kind or otherwise use the same owning-versus-back-reference predicate you use elsewhere), and apply the identical kind-filter change to the deletion check that scans originalDataModel.fields (the 610–617 match site) so original back-reference fields cannot be accidentally matched by the new owning field; this ensures findOriginalBackReferenceFields still runs when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/actions/db.ts`:
- Around line 38-40: Remove the locally defined isDataField type guard (the
function named isDataField) and instead import the exported isDataField from
`@zenstackhq/language` alongside the already-imported isEnum; update the import
statement that currently brings in isEnum to also include isDataField, and
delete the local function definition (the three-line function isDataField in
db.ts) so the file uses the shared type guard from the language package.
---
Nitpick comments:
In `@packages/cli/src/actions/db.ts`:
- Around line 380-389: The relation-name match is including both owning and
back-reference sides because getRelationName returns the same string for both;
update the filter that sets originalFields (the block that uses
originalDataModel.fields.filter and getRelationName for f) to also check the
field kind so only fields with the same side as the new field f are considered
(e.g., require the candidate d.$type/kind to match f's kind or otherwise use the
same owning-versus-back-reference predicate you use elsewhere), and apply the
identical kind-filter change to the deletion check that scans
originalDataModel.fields (the 610–617 match site) so original back-reference
fields cannot be accidentally matched by the new owning field; this ensures
findOriginalBackReferenceFields still runs when appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6fae5c2-9588-468a-adfc-d2d6c057fd27
📒 Files selected for processing (4)
packages/cli/src/actions/db.tspackages/cli/src/actions/pull/index.tspackages/cli/src/actions/pull/provider/sqlite.tspackages/cli/src/actions/pull/utils.ts
…ltiple FKs target the same model Back-reference relation fields (the opposite side of a relation, with @relation but no `fields` arg) were silently skipped during the merge phase of `db pull` when no matching field existed in the original schema. This caused models like `Users` that are referenced by many tables (e.g., via `user_created`/`user_updated` FKs) to be missing their back-reference fields after pulling. The fix adds relation-name-based matching as a new step in the field matching algorithm, and removes the blanket early-skip that discarded all unmatched back-references. Named back-references that don't match any existing field are now correctly added as new fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/actions/db.ts`:
- Around line 310-319: The current relation-name fallback in the block that
builds originalFields (using getRelationName(f)) is too permissive and can
mis-pair self-relations; update the matching logic so that when falling back to
relation-name matching you only select candidates from originalDataModel.fields
whose relation has the same presence/absence of a fields argument (i.e., both
have `@relation`(fields: ...) or both lack it) and whose referenced target model
is identical to the current field f's referenced model; apply the same
constrained matching change to the other analogous block around the
getRelationName fallback (the second occurrence noted in the review) so both
places check both "fields-argument presence" and "referenced target model" in
addition to getRelationName before assigning originalFields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9320c43-29b4-4312-bd46-96cd417610d7
📒 Files selected for processing (3)
packages/cli/src/actions/db.tspackages/cli/src/actions/pull/utils.tspackages/cli/test/db/pull.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/test/db/pull.test.ts
- Match relation fields by relation name, referenced model, and relation-args shape - Reduce false matches when multiple relations share the same name
Back-reference relation fields (the opposite side of a relation, with @relation but no
fieldsarg) were silently skipped during the merge phase ofdb pullwhen no matching field existed in the original schema. This caused models likeUsersthat are referenced by many tables (e.g., viauser_created/user_updatedFKs) to be missing their back-reference fields after pulling.The fix adds relation-name-based matching as a new step in the field matching algorithm, and removes the blanket early-skip that discarded all unmatched back-references. Named back-references that don't match any existing field are now correctly added as new fields.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests