-
Notifications
You must be signed in to change notification settings - Fork 0
fix(erd): preserve PostgreSQL relation identity in inference #990
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?
Changes from all commits
4a144a5
654dc49
fe4a457
39240fe
6e45c03
83f93ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # PostgreSQL identifier fidelity in relationship inference | ||
|
|
||
| ## Status | ||
|
|
||
| Implemented in `frontend/src/erd/autoInfer.ts` and guarded by `frontend/src/erd/__tests__/autoInfer.postgresIdentifiers.test.ts`. | ||
|
|
||
| ## Defect | ||
|
|
||
| Relationship inference used a display title as an identity source, split it on every period, and then passed the discovered name through an ASCII-only replacement function. Those transformations are not SQL quoting boundaries. They can silently change PostgreSQL relation identity for quoted identifiers containing spaces, non-Latin letters, mixed case, or periods, and can alias `Order.Items` to an unrelated `Items` table. | ||
|
|
||
| ## Decision | ||
|
|
||
| Snapshot conversion carries PostgreSQL's exact `relation_name` as structured node data. Relationship inference uses that value as its identity key. A title-only fallback exists only for manually constructed legacy nodes and removes at most the first schema separator; it does not collapse subsequent periods or create trailing-segment aliases. | ||
|
|
||
| ```text | ||
| snapshot relation_name | ||
| → exact relation identity key | ||
| → `_id` naming candidate | ||
| → exact map lookup | ||
| → inferred edge | ||
| ``` | ||
|
|
||
| The browser helper does not construct or execute SQL. Identifier rendering and SQL generation remain responsible for context-aware quoting at their own boundaries. Character deletion or last-segment aliasing is not SQL-injection prevention and can change which object is referenced. | ||
|
|
||
| ## Invariants | ||
|
|
||
| - Snapshot relation names are propagated as structured data rather than re-parsed from presentation text. | ||
| - Unicode, mixed-case, space-containing, and period-containing quoted relation names retain exact identity. | ||
| - `Order.Items` is not registered as an alias for `Items`; ambiguity resolves only by exact relation name. | ||
| - The inference heuristic still requires a matching existing table key; arbitrary column text cannot create a new target object. | ||
| - Self-relations remain excluded by exact relation-name comparison. | ||
| - SQL execution authority is unchanged and remains outside the browser inference helper. | ||
|
|
||
| ## Test-first evidence | ||
|
|
||
| The regression suite was added before the production repair and covers Korean, space-containing mixed-case, period-containing quoted identifiers, and the ambiguous pair `Order.Items` versus `Items`. On the protected-base implementation the test does not compile because structured `relation_name` is absent; after that representation is introduced, the ambiguity assertion also prevents reintroducing trailing-segment aliasing. | ||
|
|
||
| Exact-head repository CI, type checking, frontend coverage, security workflows, and independent review remain authoritative; commit order records TDD intent but does not replace those gates. | ||
|
|
||
| ## Monitoring and rollback | ||
|
|
||
| Monitor inferred candidates and accepted inferred edges by coarse identifier class without logging full customer identifiers. A post-release drop limited to non-ASCII or quoted identifiers indicates a regression. Do not roll back to ASCII rewriting or terminal-segment aliasing. Roll back only to an implementation that preserves exact model identity and applies quoting exclusively when SQL is rendered. | ||
|
|
||
| ## References | ||
|
|
||
| PostgreSQL Global Development Group. (2026). *4.1. Lexical structure*. In *PostgreSQL 18 documentation*. https://www.postgresql.org/docs/18/sql-syntax-lexical.html | ||
|
|
||
| PostgreSQL Global Development Group. (2026). *9.4. String functions and operators*. In *PostgreSQL 18 documentation*. https://www.postgresql.org/docs/18/functions-string.html | ||
|
|
||
| Rahm, E., & Bernstein, P. A. (2001). A survey of approaches to automatic schema matching. *The VLDB Journal, 10*(4), 334–350. https://doi.org/10.1007/s007780100057 | ||
|
|
||
| Rahm and Bernstein distinguish name/language evidence from structural and constraint evidence in schema matching. That distinction supports keeping the identifier token intact at the name-matching boundary instead of destructively normalizing it into a different database object identity. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| import type { Node } from "@xyflow/react"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| import { inferRelationships } from "../autoInfer"; | ||
| import type { TableNodeData } from "../convert"; | ||
|
|
||
| function tableNode( | ||
| id: string, | ||
| title: string, | ||
| relationName: string, | ||
| columns: TableNodeData["columns"], | ||
| ): Node<TableNodeData> { | ||
| return { | ||
| id, | ||
| position: { x: 0, y: 0 }, | ||
| data: { | ||
| title, | ||
| relation_name: relationName, | ||
| columns, | ||
| badges: { | ||
| pk: columns.some((column) => column.is_pk), | ||
| fk: columns.some((column) => column.column_name.endsWith("_id")), | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| describe("inferRelationships PostgreSQL identifier fidelity", () => { | ||
| it("preserves Unicode, spaces, mixed case, and periods in relation identity", () => { | ||
| const nodes: Node<TableNodeData>[] = [ | ||
| tableNode("unicode_target", "public.사용자", "사용자", [ | ||
| { column_name: "id", data_type: "bigint", is_not_null: true, is_pk: true }, | ||
| ]), | ||
| tableNode("unicode_source", "public.활동", "활동", [ | ||
| { column_name: "사용자_id", data_type: "bigint", is_not_null: true, is_pk: false }, | ||
| ]), | ||
| tableNode("quoted_target", "public.Order Items", "Order Items", [ | ||
| { column_name: "id", data_type: "bigint", is_not_null: true, is_pk: true }, | ||
| ]), | ||
| tableNode("quoted_source", "public.Order Audit", "Order Audit", [ | ||
| { column_name: "Order Items_id", data_type: "bigint", is_not_null: true, is_pk: false }, | ||
| ]), | ||
| tableNode("dotted_target", "public.Order.Items", "Order.Items", [ | ||
| { column_name: "id", data_type: "bigint", is_not_null: true, is_pk: true }, | ||
| ]), | ||
| tableNode("dotted_source", "public.Order.Audit", "Order.Audit", [ | ||
| { column_name: "Order.Items_id", data_type: "bigint", is_not_null: true, is_pk: false }, | ||
| ]), | ||
| ]; | ||
|
|
||
| const edges = inferRelationships(nodes); | ||
|
|
||
| expect(edges).toHaveLength(3); | ||
| expect(edges.find((edge) => edge.source === "unicode_source")).toMatchObject({ | ||
| target: "unicode_target", | ||
| data: { sourceColumns: ["사용자_id"], targetColumns: ["id"] }, | ||
| }); | ||
| expect(edges.find((edge) => edge.source === "quoted_source")).toMatchObject({ | ||
| target: "quoted_target", | ||
| data: { sourceColumns: ["Order Items_id"], targetColumns: ["id"] }, | ||
| }); | ||
| expect(edges.find((edge) => edge.source === "dotted_source")).toMatchObject({ | ||
| target: "dotted_target", | ||
| data: { sourceColumns: ["Order.Items_id"], targetColumns: ["id"] }, | ||
| }); | ||
| }); | ||
|
|
||
| it("does not alias a dotted relation to its trailing identifier segment", () => { | ||
| const nodes: Node<TableNodeData>[] = [ | ||
| tableNode("dotted_target", "public.Order.Items", "Order.Items", [ | ||
| { column_name: "id", data_type: "bigint", is_not_null: true, is_pk: true }, | ||
| ]), | ||
| tableNode("plain_target", "public.Items", "Items", [ | ||
| { column_name: "id", data_type: "bigint", is_not_null: true, is_pk: true }, | ||
| ]), | ||
| tableNode("source", "public.Audit", "Audit", [ | ||
| { column_name: "Items_id", data_type: "bigint", is_not_null: true, is_pk: false }, | ||
| ]), | ||
| ]; | ||
|
|
||
| expect(inferRelationships(nodes)).toContainEqual( | ||
| expect.objectContaining({ source: "source", target: "plain_target" }), | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,17 @@ | ||
| import type { Edge, Node } from "@xyflow/react"; | ||
| import type { TableNodeData } from "./convert"; | ||
| import { sourceColumnHandleId, targetColumnHandleId } from "./handleUtils"; | ||
| import { sanitizeTableName } from "./securityUtils"; | ||
|
|
||
| function relationName(node: Node<TableNodeData>): string { | ||
| if (node.data.relation_name) { | ||
| return node.data.relation_name; | ||
| } | ||
|
|
||
| const firstSeparator = node.data.title.indexOf("."); | ||
| return firstSeparator >= 0 | ||
| ? node.data.title.slice(firstSeparator + 1) | ||
| : node.data.title; | ||
|
Comment on lines
+10
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Info: Fallback identity differs for multi-dot titles For nodes without Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| } | ||
|
|
||
| /** | ||
| * 인자로 받은 노드 목록을 바탕으로 관계(Edge)를 추론하여 반환합니다. | ||
|
|
@@ -12,32 +22,26 @@ export function inferRelationships( | |
| ): Edge[] { | ||
| const newEdges: Edge[] = []; | ||
|
|
||
| // ⚡ Bolt: Use Map for O(1) table name lookups instead of Set + Array.find(), | ||
| // reducing complexity from O(N^2) to O(N). | ||
| // Use exact PostgreSQL relation identity for O(1) lookups. Do not register | ||
| // trailing segments such as "Items" for a distinct "Order.Items" relation. | ||
| const nodesByTableName = new Map<string, Node<TableNodeData>>(); | ||
| for (const n of nodes) { | ||
| const parts = n.data.title.split("."); | ||
| const tableName = parts[parts.length - 1]; | ||
| // Preserve Original .find behavior by only setting the first occurrence | ||
| if (!nodesByTableName.has(tableName)) { | ||
| nodesByTableName.set(tableName, n); | ||
| for (const node of nodes) { | ||
| const exactRelationName = relationName(node); | ||
| if (!nodesByTableName.has(exactRelationName)) { | ||
| nodesByTableName.set(exactRelationName, node); | ||
|
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 동일한
PostgreSQL은 서로 다른 schema에 같은 객체 이름을 허용합니다. (postgresql.org) schema-qualified identity를 전달하여 조회하거나, 같은 🤖 Prompt for AI Agents |
||
| } | ||
| } | ||
|
|
||
| for (const sourceNode of nodes) { | ||
| const srcParts = sourceNode.data.title.split("."); | ||
| const srcTableName = srcParts[srcParts.length - 1]; | ||
| const sourceRelationName = relationName(sourceNode); | ||
|
|
||
| for (const column of sourceNode.data.columns) { | ||
| const colName = column.column_name; | ||
|
|
||
| // xxxx_id 형태인지 확인 | ||
| if (colName.endsWith("_id")) { | ||
| const targetEntity = colName.slice(0, -3); // "_id" 제거 | ||
| const targetEntity = colName.slice(0, -3); | ||
|
|
||
| let targetTableName = ""; | ||
|
|
||
| // 대상 테이블 이름 추측 (단수형/복수형 등 간단히) | ||
| if (nodesByTableName.has(targetEntity)) { | ||
| targetTableName = targetEntity; | ||
| } else if (nodesByTableName.has(targetEntity + "s")) { | ||
|
|
@@ -46,56 +50,49 @@ export function inferRelationships( | |
| targetTableName = targetEntity + "es"; | ||
| } | ||
|
|
||
| // 자기 참조는 일단 제외 | ||
| if (targetTableName && targetTableName !== srcTableName) { | ||
| // ⚡ Bolt: O(1) lookup instead of O(N) string splitting array scan | ||
| const safeTargetTableName = sanitizeTableName(targetTableName); | ||
| const targetNode = nodesByTableName.get(safeTargetTableName); | ||
| if (targetTableName && targetTableName !== sourceRelationName) { | ||
| const targetNode = nodesByTableName.get(targetTableName); | ||
| if (!targetNode) { | ||
| continue; | ||
| } | ||
|
|
||
| if (targetNode) { | ||
| // 대상 테이블에 'id' 필드가 있는지, 혹은 PK 컬럼이 하나인지 확인 | ||
| // 여기서는 단순하게 'id' 컬럼이 있거나, 첫 번째 PK 컬럼으로 연결 | ||
| let targetColName = ""; | ||
| let idCol = undefined; | ||
| let pkCol = undefined; | ||
| let targetColName = ""; | ||
| let idCol = undefined; | ||
| let pkCol = undefined; | ||
|
|
||
| // ⚡ Bolt: Single pass O(C) search instead of two O(C) array scans with intermediate functions | ||
| for (const c of targetNode.data.columns) { | ||
| if (c.column_name === "id") { | ||
| idCol = c; | ||
| break; // id found, early exit | ||
| } | ||
| if (c.is_pk && !pkCol) { | ||
| pkCol = c; | ||
| } | ||
| for (const candidate of targetNode.data.columns) { | ||
| if (candidate.column_name === "id") { | ||
| idCol = candidate; | ||
| break; | ||
| } | ||
|
|
||
| if (idCol) { | ||
| targetColName = "id"; | ||
| } else { | ||
| if (pkCol) { | ||
| targetColName = pkCol.column_name; | ||
| } else if (targetNode.data.columns.length > 0) { | ||
| targetColName = targetNode.data.columns[0].column_name; | ||
| } | ||
| if (candidate.is_pk && !pkCol) { | ||
| pkCol = candidate; | ||
| } | ||
| } | ||
|
|
||
| if (targetColName) { | ||
| newEdges.push({ | ||
| id: `inferred_${sourceNode.id}_${colName}_${targetNode.id}_${targetColName}`, | ||
| source: sourceNode.id, | ||
| target: targetNode.id, | ||
| sourceHandle: sourceColumnHandleId(colName), | ||
| targetHandle: targetColumnHandleId(targetColName), | ||
| type: "smoothstep", | ||
| animated: true, | ||
| label: "inferred_fk", | ||
| data: { | ||
| sourceColumns: [colName], | ||
| targetColumns: [targetColName], | ||
| }, | ||
| }); | ||
| } | ||
| if (idCol) { | ||
| targetColName = "id"; | ||
| } else if (pkCol) { | ||
| targetColName = pkCol.column_name; | ||
| } else if (targetNode.data.columns.length > 0) { | ||
| targetColName = targetNode.data.columns[0].column_name; | ||
| } | ||
|
|
||
| if (targetColName) { | ||
| newEdges.push({ | ||
| id: `inferred_${sourceNode.id}_${colName}_${targetNode.id}_${targetColName}`, | ||
| source: sourceNode.id, | ||
| target: targetNode.id, | ||
| sourceHandle: sourceColumnHandleId(colName), | ||
| targetHandle: targetColumnHandleId(targetColName), | ||
| type: "smoothstep", | ||
| animated: true, | ||
| label: "inferred_fk", | ||
| data: { | ||
| sourceColumns: [colName], | ||
| targetColumns: [targetColName], | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import type { SnapshotJson } from '../types' | |
|
|
||
| export type TableNodeData = { | ||
| title: string | ||
| relation_name?: string | ||
| comment?: string | null | ||
| columns: Array<{ column_name: string; data_type: string; is_not_null: boolean; is_pk: boolean; column_comment?: string | null; example_value?: string | number | boolean | null }> | ||
| indexes?: IndexRecommendation[] | ||
|
|
@@ -134,6 +135,7 @@ export function snapshotToGraph(snapshot: SnapshotJson): { nodes: Array<Node<Tab | |
| position: { x: (i % GRID_COLUMNS) * GRID_X_GAP, y: Math.floor(i / GRID_COLUMNS) * GRID_Y_GAP }, | ||
| data: { | ||
| title: `${t.schema_name}.${t.relation_name}`, | ||
| relation_name: t.relation_name, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
새 회귀 테스트는
As per coding guidelines, 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| comment: t.relation_comment, | ||
| columns: cols, | ||
| badges: { | ||
|
|
||
This file was deleted.
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.
🟡 Frontend changelog not updated for user-visible change
CLAUDE.md requires user-visible frontend changes to be recorded in both CHANGELOG.md and
frontend/CHANGELOG.md. This PR adds the[FE]inference-fidelity entry only to the root file, leavingfrontend/CHANGELOG.mduntouched.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.