fix: handle interleave constraints as foreign key to visualize relations for Cloud Spanner#4076
fix: handle interleave constraints as foreign key to visualize relations for Cloud Spanner#4076KaoruMuta wants to merge 2 commits intoliam-hq:mainfrom
Conversation
…ons for Cloud Spanner
KaoruMuta
requested review from
NoritakaIkeda,
Copilot and
sasamuku
and removed request for
a team
|
Finished running flow.
|
||||||||||||
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded support for parsing Cloud Spanner Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@KaoruMuta is attempting to deploy a commit to the Liam Team on Vercel. A member of the Team first needs to authorize it. |
Check changeset necessityStatus: REQUIRED Reason:
Changeset (copy & paste):---
"@liam-hq/schema": patch
---
- 🐛 Treat Cloud Spanner INTERLEAVE constraints (from tbls) as foreign keys to correctly visualize parent–child relations
- Normalize INTERLEAVE to a FOREIGN KEY-shaped constraint in the tbls parser, preserving delete/update actions for relation generation |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes missing ERD relations for Cloud Spanner tbls inputs by treating INTERLEAVE constraints as a foreign-key-shaped constraint so the existing relationship generation can visualize parent/child table relations.
Changes:
- Add
INTERLEAVEconstraint handling in thetblsparser by normalizing it into aFOREIGN KEYconstraint shape. - Add a unit test covering
INTERLEAVEconstraint parsing and action extraction (ON DELETE CASCADE).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
frontend/packages/schema/src/parser/tbls/parser.ts |
Normalizes INTERLEAVE constraints into FOREIGN KEY-shaped constraints so relationships can be derived. |
frontend/packages/schema/src/parser/tbls/index.test.ts |
Adds coverage asserting how INTERLEAVE constraints are converted into internal constraint objects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| constraint.type === 'INTERLEAVE' && | ||
| constraint.columns && | ||
| constraint.columns.length > 0 && | ||
| constraint.referenced_columns && | ||
| constraint.referenced_columns.length > 0 && | ||
| constraint.referenced_table | ||
| ) { | ||
| const { updateConstraint, deleteConstraint } = extractForeignKeyActions( | ||
| constraint.def, | ||
| ) | ||
|
|
||
| return [ | ||
| constraint.name, | ||
| { | ||
| type: 'FOREIGN KEY', | ||
| name: constraint.name, | ||
| columnNames: constraint.columns, | ||
| targetTableName: constraint.referenced_table, | ||
| targetColumnNames: constraint.referenced_columns, | ||
| updateConstraint, | ||
| deleteConstraint, | ||
| }, |
There was a problem hiding this comment.
INTERLEAVE constraints can list the child table’s full primary key columns (e.g. [parent_id, child_id]) while referenced_columns only contains the parent key columns. Normalizing this directly into a FOREIGN KEY with mismatched columnNames/targetColumnNames can (1) produce invalid FK definitions for any downstream consumer (e.g. deparsers) and (2) skew relationship cardinality calculation because determineCardinalityForForeignKey uses columnNames as-is. Consider mapping only the parent-key portion, e.g. columnNames: constraint.columns.slice(0, constraint.referenced_columns.length) (and guard that the slice length is > 0).
| const expected = { | ||
| albums_interleave: { | ||
| type: 'FOREIGN KEY', | ||
| name: 'albums_interleave', | ||
| columnNames: ['singer_id', 'album_id'], | ||
| targetTableName: 'singers', | ||
| targetColumnNames: ['singer_id'], | ||
| updateConstraint: 'NO_ACTION', | ||
| deleteConstraint: 'CASCADE', | ||
| }, |
There was a problem hiding this comment.
The expected normalized FOREIGN KEY currently asserts columnNames: ['singer_id', 'album_id'] while targetColumnNames only has ['singer_id']. If processInterleaveConstraint is updated to slice the local columns to the referenced column count (to avoid mismatches / cardinality issues), update this expectation accordingly (likely columnNames: ['singer_id']).
| constraint.type === 'INTERLEAVE' && | ||
| constraint.columns && | ||
| constraint.columns.length > 0 && | ||
| constraint.referenced_columns && | ||
| constraint.referenced_columns.length > 0 && | ||
| constraint.referenced_table | ||
| ) { | ||
| const { updateConstraint, deleteConstraint } = extractForeignKeyActions( | ||
| constraint.def, | ||
| ) | ||
|
|
||
| return [ | ||
| constraint.name, | ||
| { | ||
| type: 'FOREIGN KEY', | ||
| name: constraint.name, | ||
| columnNames: constraint.columns, | ||
| targetTableName: constraint.referenced_table, | ||
| targetColumnNames: constraint.referenced_columns, | ||
| updateConstraint, | ||
| deleteConstraint, | ||
| }, | ||
| ] | ||
| } | ||
|
|
||
| return null |
There was a problem hiding this comment.
processInterleaveConstraint duplicates most of processForeignKeyConstraint. To reduce maintenance risk (e.g. future changes to FK normalization not being applied here), consider reusing processForeignKeyConstraint by transforming the input (type: 'FOREIGN KEY', possibly slicing columns) and delegating to the existing handler.
| constraint.type === 'INTERLEAVE' && | |
| constraint.columns && | |
| constraint.columns.length > 0 && | |
| constraint.referenced_columns && | |
| constraint.referenced_columns.length > 0 && | |
| constraint.referenced_table | |
| ) { | |
| const { updateConstraint, deleteConstraint } = extractForeignKeyActions( | |
| constraint.def, | |
| ) | |
| return [ | |
| constraint.name, | |
| { | |
| type: 'FOREIGN KEY', | |
| name: constraint.name, | |
| columnNames: constraint.columns, | |
| targetTableName: constraint.referenced_table, | |
| targetColumnNames: constraint.referenced_columns, | |
| updateConstraint, | |
| deleteConstraint, | |
| }, | |
| ] | |
| } | |
| return null | |
| constraint.type !== 'INTERLEAVE' || | |
| !constraint.columns || | |
| constraint.columns.length === 0 || | |
| !constraint.referenced_columns || | |
| constraint.referenced_columns.length === 0 || | |
| !constraint.referenced_table | |
| ) { | |
| return null | |
| } | |
| return processForeignKeyConstraint({ | |
| ...constraint, | |
| type: 'FOREIGN KEY', | |
| }) |
Issue
Why is this change needed?
I created #2548 to fix this issue, but this change affects common schema not only tbls but prisma..etc.
I strongly agree with solution no.3 (#2411 (comment)) while we don't fully support cloud spanner in the perspective of maintaining liam.
This PR implements solution no.3 by handling tbls interleave constraint as foreign key.
Evidence
I put schema.json by tbls which reflects cloud spanner schema information and check the behavior after rendering.
Summary by CodeRabbit
New Features
Tests