-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(drizzle): skip foreign key for blocks that are shared across collections #14395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where foreign keys were incorrectly created for blocks that are shared across collections. The fix identifies blocks with custom database names and skips creating the parent ID foreign key for them.
- Adds logic to detect blocks with custom database names that indicate shared usage across collections
- Conditionally creates foreign keys only for non-shared blocks
- Prevents database constraint errors for shared block configurations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const baseForeignKeys: Record<string, RawForeignKey> = { | ||
| _parentIdFk: { | ||
| // Skip creating a parent_id foreign key for blocks that are shared across collections (have a custom dbName) | ||
| const hasCustomDbName = block.dbName !== undefined && block.dbName !== null |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more concise nullish coalescing check. The condition can be simplified to block.dbName != null which checks for both undefined and null in a single comparison.
| const hasCustomDbName = block.dbName !== undefined && block.dbName !== null | |
| const hasCustomDbName = block.dbName != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if you have tried to use Block References for this case and if that feature doesn't work here we should fix it.
I don't think that this PR is a good solution since just checking for block.dbName does not really tell us that this block is shared across collections. Additionally, it'd cause a schema regression for every block that has dbName with removing a foreign key.
If we really want to check if a block is "shared", we'd have to traverse fields of every collection and see if we can find at least 2 the same JS references to that block.
If I remember correctly I tried it with Block referneces and received the same problem. In addition to that block references had the problem that when you have a custom field with imports these haven't been added to the import map. |
|
Yes - just confirmed it. Same issue when using blockReferences |
Recursively traverse collection and global fields to detect blocks used in multiple locations and skip foreign key creation to avoid constraint conflicts
|
@r1tsuu feel free to improve and test it. |
|
@technicaldirector I wrote a unit test and noticed that your logic didn't work correctly. The reason is because blocks in payload/packages/payload/src/utilities/flattenAllFields.ts Lines 12 to 17 in d4d9622
originalRef and changed your checks to use block.originalRef instead. In the adapter we only use flattenedFields so we don't need it to be compatible with normal fields.
|
DanRibbens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution is not going to work because when we ignore the foreign key issue you still have the problem that when using a shared table (same db name) across multiple collections that use the same block, you need have to be able to determine which blocks belong to which collection as the parent_id could be the same for multiple collection tables.
We may need to close this PR unless there is some other solution to this.
Fixes #14350