[release-17.0] schemadiff: fix missing DROP CONSTRAINT in duplicate…

…/redundant constraints scenario. (#14387) (#14390) Signed-off-by: Shlomi Noach <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
vitessio · Oct 30, 2023 · e3600d0 · e3600d0
1 parent f91a96b
commit e3600d0
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 2 deletions.
diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go
@@ -649,6 +649,20 @@ func TestSchemaDiff(t *testing.T) {
 			sequential:       true,
 			conflictingDiffs: 2,
 		},
+		{
+			name: "two identical foreign keys in table, drop one",
+			fromQueries: []string{
+				"create table parent (id int primary key)",
+				"create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
+			},
+			toQueries: []string{
+				"create table parent (id int primary key)",
+				"create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
+			},
+			expectDiffs: 1,
+			expectDeps:  0,
+			entityOrder: []string{"t1"},
+		},
 	}
 	hints := &DiffHints{RangeRotationStrategy: RangeRotationDistinctStatements}
 	for _, tc := range tt {

diff --git a/go/vt/schemadiff/table.go b/go/vt/schemadiff/table.go
@@ -1281,11 +1281,14 @@ func (c *CreateTableEntity) diffConstraints(alterTable *sqlparser.AlterTable,
 	}
 	t1ConstraintsMap := map[string]*sqlparser.ConstraintDefinition{}
 	t2ConstraintsMap := map[string]*sqlparser.ConstraintDefinition{}
+	t2ConstraintsCountMap := map[string]int{}
 	for _, constraint := range t1Constraints {
 		t1ConstraintsMap[normalizeConstraintName(t1Name, constraint)] = constraint
 	}
 	for _, constraint := range t2Constraints {
-		t2ConstraintsMap[normalizeConstraintName(t2Name, constraint)] = constraint
+		constraintName := normalizeConstraintName(t2Name, constraint)
+		t2ConstraintsMap[constraintName] = constraint
+		t2ConstraintsCountMap[constraintName]++
 	}
 
 	dropConstraintStatement := func(constraint *sqlparser.ConstraintDefinition) *sqlparser.DropKey {
@@ -1298,12 +1301,22 @@ func (c *CreateTableEntity) diffConstraints(alterTable *sqlparser.AlterTable,
 	// evaluate dropped constraints
 	//
 	for _, t1Constraint := range t1Constraints {
-		if _, ok := t2ConstraintsMap[normalizeConstraintName(t1Name, t1Constraint)]; !ok {
+		// Due to how we normalize the constraint string (e.g. in ConstraintNamesIgnoreAll we
+		// completely discard the constraint name), it's possible to have multiple constraints under
+		// the same string. Effectively, this means the schema design has duplicate/redundant constraints,
+		// which of course is poor design -- but still valid.
+		// To deal with dropping constraints, we need to not only account for the _existence_ of a constraint,
+		// but also to _how many times_ it appears.
+		constraintName := normalizeConstraintName(t1Name, t1Constraint)
+		if t2ConstraintsCountMap[constraintName] == 0 {
 			// constraint exists in t1 but not in t2, hence it is dropped
 			dropConstraint := dropConstraintStatement(t1Constraint)
 			alterTable.AlterOptions = append(alterTable.AlterOptions, dropConstraint)
+		} else {
+			t2ConstraintsCountMap[constraintName]--
 		}
 	}
+	// t2ConstraintsCountMap should not be used henceforth.
 
 	for _, t2Constraint := range t2Constraints {
 		normalizedT2ConstraintName := normalizeConstraintName(t2Name, t2Constraint)

diff --git a/go/vt/schemadiff/table_test.go b/go/vt/schemadiff/table_test.go
@@ -607,6 +607,14 @@ func TestCreateTableDiff(t *testing.T) {
 			cdiff:      "ALTER TABLE `t1` DROP CHECK `check3`",
 			constraint: ConstraintNamesIgnoreAll,
 		},
+		{
+			name:       "check constraints, remove duplicate",
+			from:       "create table t1 (id int primary key, i int, constraint `chk_123abc` CHECK ((`i` > 2)), constraint `check3` CHECK ((`i` > 2)), constraint `chk_789def` CHECK ((`i` < 5)))",
+			to:         "create table t2 (id int primary key, i int, constraint `chk_123abc` CHECK ((`i` > 2)), constraint `chk_789def` CHECK ((`i` < 5)))",
+			diff:       "alter table t1 drop check check3",
+			cdiff:      "ALTER TABLE `t1` DROP CHECK `check3`",
+			constraint: ConstraintNamesIgnoreAll,
+		},
 		{
 			name:       "check constraints, remove, ignore vitess, no match",
 			from:       "create table t1 (id int primary key, i int, constraint `chk_123abc` CHECK ((`i` > 2)), constraint `check3` CHECK ((`i` != 3)), constraint `chk_789def` CHECK ((`i` < 5)))",
@@ -658,6 +666,37 @@ func TestCreateTableDiff(t *testing.T) {
 			from: "create table t1 (id int primary key, i int, constraint f foreign key (i) references parent(id) on delete cascade)",
 			to:   "create table t2 (id int primary key, i int, constraint f foreign key (i) references parent(id) on delete cascade)",
 		},
+		{
+			name:  "two identical foreign keys, dropping one",
+			from:  "create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
+			to:    "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
+			diff:  "alter table t1 drop foreign key f2",
+			cdiff: "ALTER TABLE `t1` DROP FOREIGN KEY `f2`",
+		},
+		{
+			name:       "two identical foreign keys, dropping one, ignore vitess names",
+			from:       "create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
+			to:         "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
+			diff:       "alter table t1 drop foreign key f2",
+			cdiff:      "ALTER TABLE `t1` DROP FOREIGN KEY `f2`",
+			constraint: ConstraintNamesIgnoreVitess,
+		},
+		{
+			name:       "two identical foreign keys, dropping one, ignore all names",
+			from:       "create table t1 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
+			to:         "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id))",
+			diff:       "alter table t1 drop foreign key f2",
+			cdiff:      "ALTER TABLE `t1` DROP FOREIGN KEY `f2`",
+			constraint: ConstraintNamesIgnoreAll,
+		},
+		{
+			name:       "add two identical foreign key constraints, ignore all names",
+			from:       "create table t1 (id int primary key, i int, key i_idex (i))",
+			to:         "create table t2 (id int primary key, i int, key i_idex (i), constraint f1 foreign key (i) references parent(id), constraint f2 foreign key (i) references parent(id))",
+			diff:       "alter table t1 add constraint f1 foreign key (i) references parent (id), add constraint f2 foreign key (i) references parent (id)",
+			cdiff:      "ALTER TABLE `t1` ADD CONSTRAINT `f1` FOREIGN KEY (`i`) REFERENCES `parent` (`id`), ADD CONSTRAINT `f2` FOREIGN KEY (`i`) REFERENCES `parent` (`id`)",
+			constraint: ConstraintNamesIgnoreAll,
+		},
 		{
 			name: "implicit foreign key indexes",
 			from: "create table t1 (id int primary key, i int, key f(i), constraint f foreign key (i) references parent(id) on delete cascade)",