ERD Tool: Insert table with relations via drag-and-drop #9581
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Christian P. <pirnichristian@gmail.com>
Co-authored-by: Christian P. <pirnichristian@gmail.com>
WalkthroughThis PR enhances the ERD tool to properly display foreign key relationships when tables are individually added to diagrams. It introduces a user preference to optionally include related tables, refactors node linking logic into dedicated methods, and improves foreign key reference tracking using object identifiers (OIDs) to handle incomplete schema references. Changes
Sequence DiagramsequenceDiagram
participant User
participant ERDTool as ERDTool.jsx
participant API
participant ERDCore
participant DB as Database
User->>ERDTool: Drag table from tree to diagram
ERDTool->>API: Fetch table schema data
API->>DB: Query table metadata
DB-->>API: Return table/FK data
API-->>ERDTool: Provide table data
alt insert_table_with_relations = true
ERDTool->>ERDCore: addNodeWithLinks(data, position, metadata)
ERDCore->>ERDCore: Create node from data
ERDCore->>ERDCore: Build oid→uid map
ERDCore->>ERDCore: addLinksBetweenNodes(oidUidMap)
ERDCore->>ERDCore: Resolve FK references & create links
ERDCore-->>ERDTool: Node + Related nodes with FK links
else insert_table_with_relations = false
ERDTool->>ERDCore: addNode(cloned data, position, metadata)
ERDCore->>ERDCore: Create single node only
ERDCore-->>ERDTool: Single node without related tables
end
ERDTool->>User: Display diagram with table(s) and FK relationships
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx`:
- Around line 599-618: The catch block for the API call in the apiObj.get(...)
chain currently only logs and rethrows, causing silent failures; replace the
console.error/throw in that catch with a call to the component's existing error
handler (e.g. this.handleAxiosCatch(err) or the notifier used elsewhere) to
surface the failure to users and remove the rethrow so the promise is handled;
update the catch for apiObj.get(...) where TableSchema.getErdSupportedData and
addNode/addNodeWithLinks are invoked to call handleAxiosCatch(err) instead of
console.error/throw.
In `@web/pgadmin/tools/erd/static/js/erd_tool/nodes/TableNode.jsx`:
- Around line 161-165: serializeData() is mutating the node's live data by
filtering data.foreign_key in-place which can permanently remove FKs; fix by
cloning the data (or at least the foreign_key array) returned from getData()
before applying the filter so the node's persisted state remains unchanged —
e.g., obtain const data = this.getData(); create a shallow copy (or copy of
data.foreign_key) and run the .filter on that copy, then return the
cloned/modified copy while leaving the original getData() object intact.
🧹 Nitpick comments (1)
web/pgadmin/tools/erd/static/js/erd_tool/ERDCore.js (1)
686-724: Guard against missing FK column metadata during link creation.
addLinksBetweenNodesassumesfk.columns[0]and column lookups always resolve. A malformed FK or a column removed in the ERD could throw here and break drag‑and‑drop. A small guard keeps this path resilient.🛡️ Suggested defensive checks
- const theFkColumn = theFk.columns[0]; + const theFkColumn = theFk.columns?.[0]; + if (!theFkColumn) return; let referencesUid = oidUidMap[theFkColumn.references_oid]; @@ - const newData = { - local_table_uid: uid, - local_column_attnum: _.find( - tableNodesDict[uid].getColumns(), - (col) => col.name == theFkColumn.local_column - ).attnum, - referenced_table_uid: referencesUid, - referenced_column_attnum: _.find( - tableNodesDict[referencesUid].getColumns(), - (col) => col.name == theFkColumn.referenced - ).attnum, - }; + const localCol = _.find( + tableNodesDict[uid].getColumns(), + (col) => col.name == theFkColumn.local_column + ); + const refCol = _.find( + tableNodesDict[referencesUid].getColumns(), + (col) => col.name == theFkColumn.referenced + ); + if (!localCol || !refCol) return; + const newData = { + local_table_uid: uid, + local_column_attnum: localCol.attnum, + referenced_table_uid: referencesUid, + referenced_column_attnum: refCol.attnum, + };
Comment on lines
+599
to
+618
| this.apiObj.get(nodeDropData.objUrl) | ||
| .then((res)=>{ | ||
| const data = TableSchema.getErdSupportedData(res.data); | ||
| const {x, y} = this.diagram.getEngine().getRelativeMousePoint(e); | ||
| const position = [x,y]; | ||
| const metadata = { | ||
| fillColor: this.state.fill_color, | ||
| textColor: this.state.text_color, | ||
| }; | ||
|
|
||
| const newNode = this.state.preferences.insert_table_with_relations | ||
| ? this.diagram.addNodeWithLinks(data, position, metadata) | ||
| : this.diagram.addNode(this.diagram.cloneTableData(data), position, metadata); | ||
|
|
||
| newNode.setSelected(true); | ||
| }) | ||
| .catch((err)=>{ | ||
| console.error(err); | ||
| throw (err instanceof Error ? err : Error(gettext('Something went wrong'))); | ||
| }); |
There was a problem hiding this comment.
Surface drag‑and‑drop API failures to users.
The current catch logs and rethrows, but nothing handles the rejection, so drops can fail silently. Consider using the existing handleAxiosCatch (or notifier) and avoid rethrowing.
💡 Suggested fix
.catch((err)=>{
console.error(err);
- throw (err instanceof Error ? err : Error(gettext('Something went wrong')));
+ this.handleAxiosCatch(err);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.apiObj.get(nodeDropData.objUrl) | |
| .then((res)=>{ | |
| const data = TableSchema.getErdSupportedData(res.data); | |
| const {x, y} = this.diagram.getEngine().getRelativeMousePoint(e); | |
| const position = [x,y]; | |
| const metadata = { | |
| fillColor: this.state.fill_color, | |
| textColor: this.state.text_color, | |
| }; | |
| const newNode = this.state.preferences.insert_table_with_relations | |
| ? this.diagram.addNodeWithLinks(data, position, metadata) | |
| : this.diagram.addNode(this.diagram.cloneTableData(data), position, metadata); | |
| newNode.setSelected(true); | |
| }) | |
| .catch((err)=>{ | |
| console.error(err); | |
| throw (err instanceof Error ? err : Error(gettext('Something went wrong'))); | |
| }); | |
| this.apiObj.get(nodeDropData.objUrl) | |
| .then((res)=>{ | |
| const data = TableSchema.getErdSupportedData(res.data); | |
| const {x, y} = this.diagram.getEngine().getRelativeMousePoint(e); | |
| const position = [x,y]; | |
| const metadata = { | |
| fillColor: this.state.fill_color, | |
| textColor: this.state.text_color, | |
| }; | |
| const newNode = this.state.preferences.insert_table_with_relations | |
| ? this.diagram.addNodeWithLinks(data, position, metadata) | |
| : this.diagram.addNode(this.diagram.cloneTableData(data), position, metadata); | |
| newNode.setSelected(true); | |
| }) | |
| .catch((err)=>{ | |
| console.error(err); | |
| this.handleAxiosCatch(err); | |
| }); |
🤖 Prompt for AI Agents
In `@web/pgadmin/tools/erd/static/js/erd_tool/components/ERDTool.jsx` around lines
599 - 618, The catch block for the API call in the apiObj.get(...) chain
currently only logs and rethrows, causing silent failures; replace the
console.error/throw in that catch with a call to the component's existing error
handler (e.g. this.handleAxiosCatch(err) or the notifier used elsewhere) to
surface the failure to users and remove the rethrow so the promise is handled;
update the catch for apiObj.get(...) where TableSchema.getErdSupportedData and
addNode/addNodeWithLinks are invoked to call handleAxiosCatch(err) instead of
console.error/throw.
There was a problem hiding this comment.
@Lancear, understood. I've noted your decision to keep the error handling as-is.
(´･ω･`)
Author
|
We were unsure how to update the translations, thus skipped that for now |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature: Drag and Drop Tables with Relations in ERD
This feature allows tables to be dragged into the ERD tool along with their foreign key relationships automatically.
Closes #5578
Closes #8198
Key Decisions
Keep original
oidandforeign_keyin nodesPreserves table identity and allows checking future references. Only the first node with each
oidretains theoid.Added
references_oidNew field to track which tables are referenced by foreign keys. Foreign keys without the original
referencesfield point to missing tables.First instance priority
Only the first insert of a table is checked for incoming references from other nodes, preventing duplicate foreign key links.
SQL generation cleanup
Foreign keys pointing to missing tables are deleted from the ER diagram during SQL generation to keep output valid.
File save preserves all foreign keys
Missing table references are kept when saving, allowing users to add the referenced tables later.
User preference added
New preference "Drag and Drop table with relations" to control this behavior, default acts like before.
Co-authored-by: Christian P. pirnichristian@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.