From 3899ddf05a8254a7d31dd1c55bd5ff0a0b4b577b Mon Sep 17 00:00:00 2001 From: Jason Fulghum Date: Mon, 15 May 2023 15:26:21 -0700 Subject: [PATCH] First pass at FK equality checking to handle resolved/unresolved FKs. --- go/libraries/doltcore/diff/table_deltas.go | 44 +++++----- .../doltcore/doltdb/foreign_key_coll.go | 81 ++++++++++++++++++- integration-tests/bats/diff.bats | 36 ++++++--- 3 files changed, 127 insertions(+), 34 deletions(-) diff --git a/go/libraries/doltcore/diff/table_deltas.go b/go/libraries/doltcore/diff/table_deltas.go index ae24acfe1a..0bc6337e2f 100644 --- a/go/libraries/doltcore/diff/table_deltas.go +++ b/go/libraries/doltcore/diff/table_deltas.go @@ -398,7 +398,29 @@ func (td TableDelta) CurName() string { } func (td TableDelta) HasFKChanges() bool { - return !fkSlicesAreEqual(td.FromFks, td.ToFks) + if len(td.FromFks) != len(td.ToFks) { + return true + } + + sort.Slice(td.FromFks, func(i, j int) bool { + return td.FromFks[i].Name < td.FromFks[j].Name + }) + sort.Slice(td.ToFks, func(i, j int) bool { + return td.ToFks[i].Name < td.ToFks[j].Name + }) + + fromSchemaMap := td.FromFksParentSch + fromSchemaMap[td.FromName] = td.FromSch + toSchemaMap := td.ToFksParentSch + toSchemaMap[td.ToName] = td.ToSch + + for i := range td.FromFks { + if !td.FromFks[i].Equals(td.ToFks[i], fromSchemaMap, toSchemaMap) { + return true + } + } + + return false } // GetSchemas returns the table's schema at the fromRoot and toRoot, or schema.Empty if the table did not exist. @@ -538,26 +560,6 @@ func (td TableDelta) GetRowData(ctx context.Context) (from, to durable.Index, er return from, to, nil } -func fkSlicesAreEqual(from, to []doltdb.ForeignKey) bool { - if len(from) != len(to) { - return false - } - - sort.Slice(from, func(i, j int) bool { - return from[i].Name < from[j].Name - }) - sort.Slice(to, func(i, j int) bool { - return to[i].Name < to[j].Name - }) - - for i := range from { - if !from[i].DeepEquals(to[i]) { - return false - } - } - return true -} - // SqlSchemaDiff returns a slice of DDL statements that will transform the schema in the from delta to the schema in // the to delta. func SqlSchemaDiff(ctx context.Context, td TableDelta, toSchemas map[string]schema.Schema) ([]string, error) { diff --git a/go/libraries/doltcore/doltdb/foreign_key_coll.go b/go/libraries/doltcore/doltdb/foreign_key_coll.go index 5a21204822..b42700b3e8 100644 --- a/go/libraries/doltcore/doltdb/foreign_key_coll.go +++ b/go/libraries/doltcore/doltdb/foreign_key_coll.go @@ -124,7 +124,86 @@ func (fk ForeignKey) EqualDefs(other ForeignKey) bool { fk.OnDelete == other.OnDelete } -// DeepEquals compares all attributes of a foreign key to another, including name and table names. +// Equals compares this ForeignKey to |other| and returns true if they are equal. Foreign keys can either be in +// a "resolved" state, where the referenced columns in the parent and child tables are identified by column tags, +// or in an "unresolved" state where the reference columns in the parent and child are still identified by strings. +// If one foreign key is resolved and one is unresolved, the logic for comparing them requires resolving the string +// column names to column tags, which is why |fkSchemasByName| and |otherSchemasByName| are passed in. Each of these +// is a map of table schemas for |fk| and |other|, where the child table and every parent table referenced in the +// foreign key is present in the map. +func (fk ForeignKey) Equals(other ForeignKey, fkSchemasByName, otherSchemasByName map[string]schema.Schema) bool { + // If both FKs are resolved or unresolved, we can just deeply compare them + if fk.IsResolved() == other.IsResolved() { + return fk.DeepEquals(other) + } + + // Otherwise, one FK is resolved and one is not, so we need to work a little harder + // to calculate equality since their referenced columns are represented differently. + // First check the attributes that don't change when an FK is resolved or unresolved. + if fk.Name != other.Name && + fk.TableName != other.TableName && + fk.ReferencedTableName != other.ReferencedTableName && + fk.TableIndex != other.TableIndex && + fk.ReferencedTableIndex != other.ReferencedTableIndex && + fk.OnUpdate == other.OnUpdate && + fk.OnDelete == other.OnDelete { + return false + } + + // Sort out which FK is resolved and which is not + var resolvedFK, unresolvedFK ForeignKey + var resolvedSchemasByName map[string]schema.Schema + if fk.IsResolved() { + resolvedFK, unresolvedFK, resolvedSchemasByName = fk, other, fkSchemasByName + } else { + resolvedFK, unresolvedFK, resolvedSchemasByName = other, fk, otherSchemasByName + } + + // Check the columns on the child table + if len(resolvedFK.TableColumns) != len(unresolvedFK.UnresolvedFKDetails.TableColumns) { + return false + } + for i, tag := range resolvedFK.TableColumns { + unresolvedColName := unresolvedFK.UnresolvedFKDetails.TableColumns[i] + resolvedSch, ok := resolvedSchemasByName[resolvedFK.TableName] + if !ok { + return false + } + resolvedCol, ok := resolvedSch.GetAllCols().GetByTag(tag) + if !ok { + return false + } + if resolvedCol.Name != unresolvedColName { + return false + } + } + + // Check the columns on the parent table + if len(resolvedFK.ReferencedTableColumns) != len(unresolvedFK.UnresolvedFKDetails.ReferencedTableColumns) { + return false + } + for i, tag := range resolvedFK.ReferencedTableColumns { + unresolvedColName := unresolvedFK.UnresolvedFKDetails.ReferencedTableColumns[i] + resolvedSch, ok := resolvedSchemasByName[unresolvedFK.ReferencedTableName] + if !ok { + return false + } + resolvedCol, ok := resolvedSch.GetAllCols().GetByTag(tag) + if !ok { + return false + } + if resolvedCol.Name != unresolvedColName { + return false + } + } + + return true +} + +// DeepEquals compares all attributes of a foreign key to another, including name and +// table names. Note that if one foreign key is resolved and the other is NOT resolved, +// then this function will not calculate equality correctly. When comparing a resolved +// FK with an unresolved FK, the ForeignKey.Equals() function should be used instead. func (fk ForeignKey) DeepEquals(other ForeignKey) bool { if !fk.EqualDefs(other) { return false diff --git a/integration-tests/bats/diff.bats b/integration-tests/bats/diff.bats index 6678590fef..fef8f0c770 100644 --- a/integration-tests/bats/diff.bats +++ b/integration-tests/bats/diff.bats @@ -771,23 +771,35 @@ SQL [[ "$output" =~ 'resolved foreign key' ]] || false } -@test "diff: existing foreign key is resolved" { +@test "diff: resolved FKs don't show up in diff results" { dolt sql <