From e8ac0e8c35341dd0fe73ca9fdb295d30a46529d4 Mon Sep 17 00:00:00 2001 From: SG Command Date: Mon, 31 Aug 2026 02:05:32 +0200 Subject: [PATCH] fix diff round-trip comparison --- pkg/diff/diff.go | 188 +++++++++++++++++++++++++----- pkg/diff/diff_test.go | 16 +++ pkg/readers/dbml/reader.go | 22 ++++ pkg/readers/pgsql/queries.go | 10 ++ pkg/readers/pgsql/reader.go | 2 + pkg/readers/pgsql/reader_test.go | 9 ++ tests/postgres/issue21/schema.sql | 30 +++++ tests/postgres/issue21/spec.dbml | 28 +++++ 8 files changed, 275 insertions(+), 30 deletions(-) create mode 100644 tests/postgres/issue21/schema.sql create mode 100644 tests/postgres/issue21/spec.dbml diff --git a/pkg/diff/diff.go b/pkg/diff/diff.go index 0f5a763..7bb7b4d 100644 --- a/pkg/diff/diff.go +++ b/pkg/diff/diff.go @@ -4,6 +4,8 @@ import ( "fmt" "reflect" "sort" + "strconv" + "strings" "git.warky.dev/wdevs/relspecgo/pkg/models" ) @@ -229,11 +231,13 @@ func compareColumns(source, target map[string]*models.Column) *ColumnDiff { func compareColumnDetails(source, target *models.Column) map[string]any { changes := make(map[string]any) + sourceType, sourceLength, sourceDefault := comparableColumn(source) + targetType, targetLength, targetDefault := comparableColumn(target) - if source.Type != target.Type { + if sourceType != targetType { changes["type"] = map[string]string{"source": source.Type, "target": target.Type} } - if source.Length != target.Length { + if sourceLength != targetLength { changes["length"] = map[string]int{"source": source.Length, "target": target.Length} } if source.Precision != target.Precision { @@ -245,8 +249,8 @@ func compareColumnDetails(source, target *models.Column) map[string]any { if source.NotNull != target.NotNull { changes["not_null"] = map[string]bool{"source": source.NotNull, "target": target.NotNull} } - if !reflect.DeepEqual(source.Default, target.Default) { - changes["default"] = map[string]any{"source": source.Default, "target": target.Default} + if !reflect.DeepEqual(sourceDefault, targetDefault) { + changes["default"] = map[string]any{"source": sourceDefault, "target": targetDefault} } if source.AutoIncrement != target.AutoIncrement { changes["auto_increment"] = map[string]bool{"source": source.AutoIncrement, "target": target.AutoIncrement} @@ -258,6 +262,28 @@ func compareColumnDetails(source, target *models.Column) map[string]any { return changes } +// comparableColumn accepts DBML's compact type/default spelling as well as +// PostgreSQL's normalized fields (for example varchar(255) vs varchar + 255). +func comparableColumn(column *models.Column) (string, int, any) { + typeName := strings.TrimSpace(column.Type) + defaultValue := column.Default + lower := strings.ToLower(typeName) + if i := strings.Index(lower, " default "); i >= 0 { + if defaultValue == nil { + defaultValue = strings.TrimSpace(typeName[i+len(" default "):]) + } + typeName = strings.TrimSpace(typeName[:i]) + } + length := column.Length + if open := strings.LastIndex(typeName, "("); open >= 0 && strings.HasSuffix(typeName, ")") { + if parsed, err := strconv.Atoi(strings.TrimSpace(typeName[open+1 : len(typeName)-1])); err == nil && length == 0 { + length = parsed + } + typeName = strings.TrimSpace(typeName[:open]) + } + return strings.ToLower(typeName), length, defaultValue +} + func compareIndexes(source, target map[string]*models.Index) *IndexDiff { diff := &IndexDiff{ Missing: make([]*models.Index, 0), @@ -265,34 +291,87 @@ func compareIndexes(source, target map[string]*models.Index) *IndexDiff { Modified: make([]*IndexChange, 0), } - // Find missing and modified indexes + // Match by name first, then by definition. PostgreSQL and DBML can assign + // different names to the same index (for example, posts_user_id_title_idx + // and uidx_posts_user_id_title), so a name-only comparison reports false + // drift after a merge/diff round trip. + unmatchedSource := make(map[string]*models.Index, len(source)) + unmatchedTarget := make(map[string]*models.Index, len(target)) + for name, index := range source { + unmatchedSource[name] = index + } + for name, index := range target { + unmatchedTarget[name] = index + } + for _, name := range sortedKeys(source) { srcIdx := source[name] - if tgtIdx, exists := target[name]; !exists { + tgtIdx, exists := target[name] + if !exists { + continue + } + delete(unmatchedSource, name) + delete(unmatchedTarget, name) + if changes := compareIndexDetails(srcIdx, tgtIdx); len(changes) > 0 { + diff.Modified = append(diff.Modified, &IndexChange{ + Name: name, + Source: srcIdx, + Target: tgtIdx, + Changes: changes, + }) + } + } + + // Pair remaining indexes by their structural identity, independent of the + // generated/name field. The sorted iteration makes ambiguous matches + // deterministic; duplicate definitions are still represented as separate + // indexes by consuming one target at a time. + remainingTarget := make(map[string][]*models.Index) + for _, name := range sortedKeys(unmatchedTarget) { + index := unmatchedTarget[name] + key := indexDefinitionKey(index) + remainingTarget[key] = append(remainingTarget[key], index) + } + for _, name := range sortedKeys(unmatchedSource) { + srcIdx := unmatchedSource[name] + key := indexDefinitionKey(srcIdx) + candidates := remainingTarget[key] + if len(candidates) == 0 { diff.Missing = append(diff.Missing, srcIdx) - } else { - if changes := compareIndexDetails(srcIdx, tgtIdx); len(changes) > 0 { - diff.Modified = append(diff.Modified, &IndexChange{ - Name: name, - Source: srcIdx, - Target: tgtIdx, - Changes: changes, - }) - } + continue + } + tgtIdx := candidates[0] + remainingTarget[key] = candidates[1:] + if changes := compareIndexDetails(srcIdx, tgtIdx); len(changes) > 0 { + diff.Modified = append(diff.Modified, &IndexChange{ + Name: srcIdx.Name, + Source: srcIdx, + Target: tgtIdx, + Changes: changes, + }) } } - // Find extra indexes - for _, name := range sortedKeys(target) { - tgtIdx := target[name] - if _, exists := source[name]; !exists { - diff.Extra = append(diff.Extra, tgtIdx) + for _, key := range sortedKeys(remainingTarget) { + for _, index := range remainingTarget[key] { + diff.Extra = append(diff.Extra, index) } } - return diff } +func indexDefinitionKey(index *models.Index) string { + return fmt.Sprintf("%t:%s:%s", index.Unique, strings.Join(index.Columns, ","), strings.Join(index.Include, ",")) +} + +func comparableIndexType(indexType string) string { + indexType = strings.ToLower(strings.TrimSpace(indexType)) + if indexType == "" { + return "btree" + } + return indexType +} + func compareIndexDetails(source, target *models.Index) map[string]any { changes := make(map[string]any) @@ -302,7 +381,7 @@ func compareIndexDetails(source, target *models.Index) map[string]any { if source.Unique != target.Unique { changes["unique"] = map[string]bool{"source": source.Unique, "target": target.Unique} } - if source.Type != target.Type { + if comparableIndexType(source.Type) != comparableIndexType(target.Type) { changes["type"] = map[string]string{"source": source.Type, "target": target.Type} } if source.Where != target.Where { @@ -312,7 +391,26 @@ func compareIndexDetails(source, target *models.Index) map[string]any { return changes } +// Compare constraints. +// Primary-key constraints are excluded: a PK is already represented by the +// column's IsPrimaryKey flag, which compareColumns already compares. The +// PostgreSQL reader additionally materialises each PK as a primary_key +// constraint and a unique btree index; the DBML reader keeps PKs as column +// flags only. Comparing the constraint maps directly would therefore report +// every PK as an "extra" constraint and the generated index as an "extra" +// index on a freshly-applied schema. Filtering them here keeps the round +// trip stable without losing real PK information. func compareConstraints(source, target map[string]*models.Constraint) *ConstraintDiff { + filteredSource := filterPrimaryKeyConstraints(source) + filteredTarget := filterPrimaryKeyConstraints(target) + sourceByKey := make(map[string]*models.Constraint, len(filteredSource)) + targetByKey := make(map[string]*models.Constraint, len(filteredTarget)) + for _, constraint := range filteredSource { + sourceByKey[constraintCompareKey(constraint)] = constraint + } + for _, constraint := range filteredTarget { + targetByKey[constraintCompareKey(constraint)] = constraint + } diff := &ConstraintDiff{ Missing: make([]*models.Constraint, 0), Extra: make([]*models.Constraint, 0), @@ -320,9 +418,9 @@ func compareConstraints(source, target map[string]*models.Constraint) *Constrain } // Find missing and modified constraints - for _, name := range sortedKeys(source) { - srcCon := source[name] - if tgtCon, exists := target[name]; !exists { + for _, name := range sortedKeys(sourceByKey) { + srcCon := sourceByKey[name] + if tgtCon, exists := targetByKey[name]; !exists { diff.Missing = append(diff.Missing, srcCon) } else { if changes := compareConstraintDetails(srcCon, tgtCon); len(changes) > 0 { @@ -337,9 +435,9 @@ func compareConstraints(source, target map[string]*models.Constraint) *Constrain } // Find extra constraints - for _, name := range sortedKeys(target) { - tgtCon := target[name] - if _, exists := source[name]; !exists { + for _, name := range sortedKeys(targetByKey) { + tgtCon := targetByKey[name] + if _, exists := sourceByKey[name]; !exists { diff.Extra = append(diff.Extra, tgtCon) } } @@ -347,6 +445,29 @@ func compareConstraints(source, target map[string]*models.Constraint) *Constrain return diff } +// filterPrimaryKeyConstraints drops primary_key constraints from a single +// map. Primary keys are compared by the column IsPrimaryKey flag in +// compareColumns, so comparing the primary_key constraints here only +// produces duplicate "extra" entries (every PK is extra on the DBML side). +// Other constraint types are preserved untouched. +func filterPrimaryKeyConstraints(m map[string]*models.Constraint) map[string]*models.Constraint { + out := make(map[string]*models.Constraint, len(m)) + for name, c := range m { + if c.Type == models.PrimaryKeyConstraint { + continue + } + out[name] = c + } + return out +} + +func constraintCompareKey(constraint *models.Constraint) string { + if constraint.Type != models.ForeignKeyConstraint { + return constraint.SQLName() + } + return fmt.Sprintf("fk:%s:%s:%s:%s:%s:%s", strings.ToLower(constraint.Schema), strings.ToLower(constraint.Table), strings.Join(constraint.Columns, ","), strings.ToLower(constraint.ReferencedSchema), strings.ToLower(constraint.ReferencedTable), strings.Join(constraint.ReferencedColumns, ",")) +} + func compareConstraintDetails(source, target *models.Constraint) map[string]any { changes := make(map[string]any) @@ -362,16 +483,23 @@ func compareConstraintDetails(source, target *models.Constraint) map[string]any if !reflect.DeepEqual(source.ReferencedColumns, target.ReferencedColumns) { changes["referenced_columns"] = map[string][]string{"source": source.ReferencedColumns, "target": target.ReferencedColumns} } - if source.OnDelete != target.OnDelete { + if normalizeConstraintAction(source.OnDelete) != normalizeConstraintAction(target.OnDelete) { changes["on_delete"] = map[string]string{"source": source.OnDelete, "target": target.OnDelete} } - if source.OnUpdate != target.OnUpdate { + if normalizeConstraintAction(source.OnUpdate) != normalizeConstraintAction(target.OnUpdate) { changes["on_update"] = map[string]string{"source": source.OnUpdate, "target": target.OnUpdate} } return changes } +func normalizeConstraintAction(action string) string { + if strings.EqualFold(strings.TrimSpace(action), "NO ACTION") { + return "" + } + return strings.ToUpper(strings.TrimSpace(action)) +} + func compareRelationships(source, target map[string]*models.Relationship) *RelationshipDiff { diff := &RelationshipDiff{ Missing: make([]*models.Relationship, 0), diff --git a/pkg/diff/diff_test.go b/pkg/diff/diff_test.go index babb400..6cb170b 100644 --- a/pkg/diff/diff_test.go +++ b/pkg/diff/diff_test.go @@ -301,6 +301,22 @@ func TestCompareIndexes(t *testing.T) { return len(d.Modified) == 1 && d.Modified[0].Name == "idx_name" }, }, + { + name: "equivalent indexes with different generated names", + source: map[string]*models.Index{ + "uidx_posts_user_id_title": { + Name: "uidx_posts_user_id_title", Columns: []string{"user_id", "title"}, Unique: true, + }, + }, + target: map[string]*models.Index{ + "posts_user_id_title_idx": { + Name: "posts_user_id_title_idx", Columns: []string{"user_id", "title"}, Unique: true, Type: "btree", + }, + }, + want: func(d *IndexDiff) bool { + return len(d.Missing) == 0 && len(d.Extra) == 0 && len(d.Modified) == 0 + }, + }, } for _, tt := range tests { diff --git a/pkg/readers/dbml/reader.go b/pkg/readers/dbml/reader.go index f9cfe08..69c2bbb 100644 --- a/pkg/readers/dbml/reader.go +++ b/pkg/readers/dbml/reader.go @@ -571,6 +571,28 @@ func (r *Reader) parseDBML(content string) (*models.Database, error) { } } + // PostgreSQL readers derive relationships from foreign keys. Do the same + // for DBML refs so diffing equivalent schemas compares the same model. + for _, schema := range schemaMap { + for _, table := range schema.Tables { + for _, constraint := range table.Constraints { + if constraint.Type != models.ForeignKeyConstraint { + continue + } + name := fmt.Sprintf("%s_to_%s", table.Name, constraint.ReferencedTable) + relationship := models.InitRelationship(name, models.OneToMany) + relationship.FromTable = table.Name + relationship.FromSchema = table.Schema + relationship.FromColumns = append([]string(nil), constraint.Columns...) + relationship.ToTable = constraint.ReferencedTable + relationship.ToSchema = constraint.ReferencedSchema + relationship.ToColumns = append([]string(nil), constraint.ReferencedColumns...) + relationship.ForeignKey = constraint.Name + table.Relationships[name] = relationship + } + } + } + // Add schemas to database for _, schema := range schemaMap { db.Schemas = append(db.Schemas, schema) diff --git a/pkg/readers/pgsql/queries.go b/pkg/readers/pgsql/queries.go index b8809fa..1da7a46 100644 --- a/pkg/readers/pgsql/queries.go +++ b/pkg/readers/pgsql/queries.go @@ -538,8 +538,13 @@ func (r *Reader) queryCheckConstraints(schemaName string) (map[string][]*models. FROM information_schema.table_constraints tc JOIN information_schema.check_constraints cc ON tc.constraint_name = cc.constraint_name + AND cc.constraint_schema = tc.table_schema + JOIN pg_catalog.pg_constraint pc + ON pc.conname = tc.constraint_name + AND pc.connamespace = (SELECT oid FROM pg_namespace WHERE nspname = tc.table_schema) WHERE tc.constraint_type = 'CHECK' AND tc.table_schema = $1 + AND pc.contype = 'c' ` rows, err := r.conn.Query(r.ctx, query, schemaName) @@ -579,7 +584,12 @@ func (r *Reader) queryIndexes(schemaName string) (map[string][]*models.Index, er indexname, indexdef FROM pg_indexes + JOIN pg_catalog.pg_class idx ON idx.relname = indexname + JOIN pg_catalog.pg_index i ON i.indexrelid = idx.oid + JOIN pg_catalog.pg_namespace idx_ns ON idx_ns.oid = idx.relnamespace WHERE schemaname = $1 + AND idx_ns.nspname = schemaname + AND NOT i.indisprimary ORDER BY schemaname, tablename, indexname ` diff --git a/pkg/readers/pgsql/reader.go b/pkg/readers/pgsql/reader.go index 1adcd16..44f029e 100644 --- a/pkg/readers/pgsql/reader.go +++ b/pkg/readers/pgsql/reader.go @@ -341,8 +341,10 @@ func (r *Reader) deriveRelationship(table *models.Table, fk *models.Constraint) relationship := models.InitRelationship(relationshipName, models.OneToMany) relationship.FromTable = table.Name relationship.FromSchema = table.Schema + relationship.FromColumns = append([]string(nil), fk.Columns...) relationship.ToTable = fk.ReferencedTable relationship.ToSchema = fk.ReferencedSchema + relationship.ToColumns = append([]string(nil), fk.ReferencedColumns...) relationship.ForeignKey = fk.Name // Store constraint actions in properties diff --git a/pkg/readers/pgsql/reader_test.go b/pkg/readers/pgsql/reader_test.go index 142ad4e..2afbcfa 100644 --- a/pkg/readers/pgsql/reader_test.go +++ b/pkg/readers/pgsql/reader_test.go @@ -2,6 +2,7 @@ package pgsql import ( "os" + "reflect" "testing" "git.warky.dev/wdevs/relspecgo/pkg/models" @@ -359,6 +360,14 @@ func TestDeriveRelationship(t *testing.T) { t.Errorf("Expected ToTable 'users', got '%s'", rel.ToTable) } + if !reflect.DeepEqual(rel.FromColumns, []string{"user_id"}) { + t.Errorf("Expected FromColumns [user_id], got %v", rel.FromColumns) + } + + if !reflect.DeepEqual(rel.ToColumns, []string{"id"}) { + t.Errorf("Expected ToColumns [id], got %v", rel.ToColumns) + } + if rel.ForeignKey != "fk_orders_user_id" { t.Errorf("Expected ForeignKey 'fk_orders_user_id', got '%s'", rel.ForeignKey) } diff --git a/tests/postgres/issue21/schema.sql b/tests/postgres/issue21/schema.sql new file mode 100644 index 0000000..ab5db87 --- /dev/null +++ b/tests/postgres/issue21/schema.sql @@ -0,0 +1,30 @@ +CREATE TABLE public.users ( + id integer NOT NULL, + username varchar(255) NOT NULL, + email varchar(255) NOT NULL, + created_at timestamptz NOT NULL, + profile_id integer +); + +CREATE TABLE public.profiles ( + id integer NOT NULL, + bio text, + created_at timestamptz NOT NULL +); + +CREATE TABLE public.posts ( + id integer NOT NULL, + user_id integer NOT NULL, + title varchar(255) NOT NULL, + body text, + published_at timestamptz, + view_count integer DEFAULT 0 +); + +ALTER TABLE public.users ADD PRIMARY KEY (id); +ALTER TABLE public.profiles ADD PRIMARY KEY (id); +ALTER TABLE public.posts ADD PRIMARY KEY (id); + +CREATE UNIQUE INDEX posts_user_id_title_idx ON public.posts (user_id, title); + +ALTER TABLE public.users ADD CONSTRAINT users_profile_id_fkey FOREIGN KEY (profile_id) REFERENCES public.profiles (id); diff --git a/tests/postgres/issue21/spec.dbml b/tests/postgres/issue21/spec.dbml new file mode 100644 index 0000000..4402184 --- /dev/null +++ b/tests/postgres/issue21/spec.dbml @@ -0,0 +1,28 @@ +Table users { + id integer [pk, not null] + username varchar(255) [not null] + email varchar(255) [not null] + created_at timestamptz [not null] + profile_id integer +} + +Table profiles { + id integer [pk, not null] + bio text + created_at timestamptz [not null] +} + +Table posts { + id integer [pk, not null] + user_id integer [not null] + title varchar(255) [not null] + body text + published_at timestamptz + view_count integer default 0 + + Indexes { + (user_id, title) [unique] + } +} + +Ref: users.profile_id > profiles.id