feat: build a directed graph out of schema#2
Conversation
📝 WalkthroughWalkthroughAdds a DatabaseName field to SchemaGraph and implements Build(db *database.Database) that populates Nodes from db.Tables, constructs Edges from foreign-key constraints (only when referenced tables exist), and returns a populated SchemaGraph. (<=50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)graph/graph.go (1)
🔇 Additional comments (5)
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: 1
🤖 Fix all issues with AI agents
In @graph/graph.go:
- Line 22: The edges slice is declared as a nil slice with "var edges
[]ForeignKeyEdge" which fails DeepEqual-based tests; replace its initialization
with a non-nil empty slice (e.g., use "edges := []ForeignKeyEdge{}" or "edges :=
make([]ForeignKeyEdge, 0)") so the variable edges (of type []ForeignKeyEdge) is
an empty but non-nil slice matching test expectations.
🧹 Nitpick comments (3)
graph/graph.go (2)
20-20: Add nil check for db parameter.The function doesn't validate that
dbis non-nil, which could cause a panic at line 24.🛡️ Proposed fix
func Build(db *database.Database) *SchemaGraph { + if db == nil { + return &SchemaGraph{ + Nodes: make(map[TableName]*database.Table), + Edges: []ForeignKeyEdge{}, + } + } nodes := make(map[TableName]*database.Table) var edges []ForeignKeyEdge
28-38: Consider adding validation for foreign key references for defensive programming.The function creates edges without verifying that
constraint.ReferenceTableexists in the nodes map. While the current data flow (PostgreSQL InspectSchema) guarantees all ReferenceTable values point to existing tables due to database-enforced referential integrity, adding explicit validation would make this assumption clear and protect against issues if Build is called with incomplete or manually constructed Database objects.Consider adding a check to skip or log dangling references:
if _, exists := nodes[TableName(constraint.ReferenceTable)]; !exists { // Handle dangling reference: skip, log warning, or error continue }This would also make the code more resilient if the graph package is used in other contexts beyond its current unused state.
graph/graph_test.go (1)
10-161: Good test coverage for core scenarios.The tests comprehensively validate the Build function with three key scenarios: empty database, single table, and foreign key relationships. The assertions properly check DatabaseName, Nodes, and Edges.
Consider adding test cases for edge scenarios:
- Multiple foreign keys from the same table
- Self-referencing foreign key (table referencing itself)
- Composite foreign keys (multi-column)
- Foreign key referencing non-existent table (if this should be handled)
📋 Example test case for multiple foreign keys
{ name: "table with multiple foreign keys", db: &database.Database{ Name: "test_db", Tables: []database.Table{ { Name: "users", Columns: []database.Column{ {Name: "id", Type: "integer", IsNullable: false}, }, Constraints: []database.Constraint{ {Kind: database.PrimaryKey, Columns: []string{"id"}}, }, }, { Name: "addresses", Columns: []database.Column{ {Name: "id", Type: "integer", IsNullable: false}, }, Constraints: []database.Constraint{ {Kind: database.PrimaryKey, Columns: []string{"id"}}, }, }, { Name: "orders", Columns: []database.Column{ {Name: "id", Type: "integer", IsNullable: false}, {Name: "user_id", Type: "integer", IsNullable: false}, {Name: "shipping_address_id", Type: "integer", IsNullable: false}, }, Constraints: []database.Constraint{ {Kind: database.PrimaryKey, Columns: []string{"id"}}, { Kind: database.ForeignKey, Columns: []string{"user_id"}, ReferenceTable: "users", ReferenceColumns: []string{"id"}, }, { Kind: database.ForeignKey, Columns: []string{"shipping_address_id"}, ReferenceTable: "addresses", ReferenceColumns: []string{"id"}, }, }, }, }, }, expected: &SchemaGraph{ DatabaseName: "test_db", Nodes: map[TableName]*database.Table{ // ... nodes mapping }, Edges: []ForeignKeyEdge{ { FromTable: "orders", ToTable: "users", Columns: []string{"user_id"}, ReferenceColumns: []string{"id"}, }, { FromTable: "orders", ToTable: "addresses", Columns: []string{"shipping_address_id"}, ReferenceColumns: []string{"id"}, }, }, }, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
graph/graph.gograph/graph_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
graph/graph.go (1)
database/database.go (3)
Table(43-47)Database(50-53)ForeignKey(19-19)
🔇 Additional comments (1)
graph/graph.go (1)
24-26: LGTM!Taking the address of the slice element correctly avoids copying table data while ensuring the pointer remains valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @graph/graph.go:
- Around line 28-38: The foreign-key edge creation loop (iterating
table.Constraints and building ForeignKeyEdge using constraint.ReferenceTable)
can produce dangling edges when referenced tables are in other schemas; before
appending the edge to edges, verify the referenced table exists in your
nodes/tables map (the same map populated by getTables())—e.g., resolve
TableName(constraint.ReferenceTable) against the nodes map or db.Tables and skip
the constraint if the target is missing; alternatively, update
getTables()/getAllForeignKeys() to include cross-schema references consistently,
but do not append edges for referenceTable values that are not present in the
nodes map.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
graph/graph.go
🧰 Additional context used
🧬 Code graph analysis (1)
graph/graph.go (1)
database/database.go (3)
Table(43-47)Database(50-53)ForeignKey(19-19)
🔇 Additional comments (2)
graph/graph.go (2)
15-17: Well-designed schema graph structure.The SchemaGraph struct provides a clean representation with efficient lookup via the Nodes map and clear foreign key relationships in Edges. The DatabaseName field adds useful context for tracking the source database.
24-26: LGTM! Correct pointer handling.Using
&db.Tables[i]to obtain a pointer to the slice element is the correct approach to avoid copying the Table struct while ensuring a stable reference.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @graph/graph.go:
- Around line 20-49: Build currently adds edges only if the referenced table was
already seen, causing order-dependent missing edges and it also dereferences db
without a nil check; update Build to first check for nil db, then perform a
two-pass approach: first iterate db.Tables to populate the nodes map (use
TableName(table.Name) as before), then in a second loop iterate all tables and
their Constraints and for each database.ForeignKey create a ForeignKeyEdge
(FromTable, ToTable using TableName(constraint.ReferenceTable), Columns,
ReferenceColumns) and append it to edges — this ensures edges are created
regardless of table order and uses the fully populated nodes map to optionally
verify referenced tables exist.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
graph/graph.go
🧰 Additional context used
🧬 Code graph analysis (1)
graph/graph.go (2)
database/database.go (5)
Table(43-47)Database(50-53)ForeignKey(19-19)InspectSchema(56-58)InspectSchema(84-99)database/database_test.go (1)
TestInspectSchema(14-119)
🔇 Additional comments (1)
graph/graph.go (1)
14-18: LGTM! Well-structured schema graph representation.The field additions are appropriate:
DatabaseNametracks the database contextNodesas a map enables efficient table lookupsEdgesas a slice provides a straightforward representation of foreign key relationships
1 participant
Description
Buildfunction to construct schema graph from database metadataTesting:
go test ./graph -v