schemadiff: validate uniqueness of CHECK and of FOREIGN KEY con…

…straints (#17627)

Signed-off-by: Shlomi Noach <[email protected]>

go/vt/schemadiff/errors.go

@@ -497,3 +497,21 @@ type NonDeterministicDefaultError struct {
 func (e *NonDeterministicDefaultError) Error() string {
 	return fmt.Sprintf("column %s.%s default value uses non-deterministic function: %s", sqlescape.EscapeID(e.Table), sqlescape.EscapeID(e.Column), e.Function)
 }
+
+type DuplicateCheckConstraintNameError struct {
+	Table      string
+	Constraint string
+}
+
+func (e *DuplicateCheckConstraintNameError) Error() string {
+	return fmt.Sprintf("duplicate check constraint name %s in table %s", sqlescape.EscapeID(e.Constraint), sqlescape.EscapeID(e.Table))
+}
+
+type DuplicateForeignKeyConstraintNameError struct {
+	Table      string
+	Constraint string
+}
+
+func (e *DuplicateForeignKeyConstraintNameError) Error() string {
+	return fmt.Sprintf("duplicate foreign key constraint name %s in table %s", sqlescape.EscapeID(e.Constraint), sqlescape.EscapeID(e.Table))
+}

go/vt/schemadiff/schema.go

@@ -415,6 +415,27 @@ func (s *Schema) normalize(hints *DiffHints) error {
 			}
 		}
 	}
+
+	// Validate uniqueness of check constraint and of foreign key constraint names
+	fkConstraintNames := map[string]bool{}
+	checkConstraintNames := map[string]bool{}
+	for _, t := range s.tables {
+		for _, cs := range t.TableSpec.Constraints {
+			if _, ok := cs.Details.(*sqlparser.ForeignKeyDefinition); ok {
+				if _, ok := fkConstraintNames[cs.Name.String()]; ok {
+					errs = errors.Join(errs, &DuplicateForeignKeyConstraintNameError{Table: t.Name(), Constraint: cs.Name.String()})
+				}
+				fkConstraintNames[cs.Name.String()] = true
+			}
+			if _, ok := cs.Details.(*sqlparser.CheckConstraintDefinition); ok {
+				if _, ok := checkConstraintNames[cs.Name.String()]; ok {
+					errs = errors.Join(errs, &DuplicateCheckConstraintNameError{Table: t.Name(), Constraint: cs.Name.String()})
+				}
+				checkConstraintNames[cs.Name.String()] = true
+			}
+		}
+	}
+
 	return errs
 }
 

go/vt/schemadiff/schema_diff_test.go

@@ -1327,6 +1327,10 @@ func TestSchemaDiff(t *testing.T) {
 // TestDiffFiles diffs two schema files on the local file system. It requires the $TEST_SCHEMADIFF_DIFF_FILES
 // environment variable to be set to a comma-separated list of two file paths, e.g. "/tmp/from.sql,/tmp/to.sql".
 // If the variable is unspecified, the test is skipped. It is useful for ad-hoc testing of schema diffs.
+// The way to run this test is:
+// ```sh
+// $ TEST_SCHEMADIFF_DIFF_FILES=/tmp/1.sql,/tmp/2.sql go test -v -count=1 -run TestDiffFiles ./go/vt/schemadiff/
+// ```
 func TestDiffFiles(t *testing.T) {
 	ctx := context.Background()
 

go/vt/schemadiff/schema_test.go

@@ -287,11 +287,11 @@ func TestTableForeignKeyOrdering(t *testing.T) {
 		"create table t11 (id int primary key, i int, key ix (i), constraint f12 foreign key (i) references t12(id) on delete restrict, constraint f20 foreign key (i) references t20(id) on delete restrict)",
 		"create table t15(id int, primary key(id))",
 		"create view v09 as select * from v13, t17",
-		"create table t20 (id int primary key, i int, key ix (i), constraint f15 foreign key (i) references t15(id) on delete restrict)",
+		"create table t20 (id int primary key, i int, key ix (i), constraint f2015 foreign key (i) references t15(id) on delete restrict)",
 		"create view v13 as select * from t20",
-		"create table t12 (id int primary key, i int, key ix (i), constraint f15 foreign key (i) references t15(id) on delete restrict)",
-		"create table t17 (id int primary key, i int, key ix (i), constraint f11 foreign key (i) references t11(id) on delete restrict, constraint f15 foreign key (i) references t15(id) on delete restrict)",
-		"create table t16 (id int primary key, i int, key ix (i), constraint f11 foreign key (i) references t11(id) on delete restrict, constraint f15 foreign key (i) references t15(id) on delete restrict)",
+		"create table t12 (id int primary key, i int, key ix (i), constraint f1215 foreign key (i) references t15(id) on delete restrict)",
+		"create table t17 (id int primary key, i int, key ix (i), constraint f1711 foreign key (i) references t11(id) on delete restrict, constraint f1715 foreign key (i) references t15(id) on delete restrict)",
+		"create table t16 (id int primary key, i int, key ix (i), constraint f1611 foreign key (i) references t11(id) on delete restrict, constraint f1615 foreign key (i) references t15(id) on delete restrict)",
 		"create table t14 (id int primary key, i int, key ix (i), constraint f14 foreign key (i) references t14(id) on delete restrict)",
 	}
 	expectSortedTableNames := []string{
@@ -522,6 +522,54 @@ func TestInvalidSchema(t *testing.T) {
 		{
 			schema: "create table post (id varchar(191) charset utf8mb4 not null, `title` text, primary key (`id`)); create table post_fks (id varchar(191) not null, `post_id` varchar(191) collate utf8mb4_0900_ai_ci, primary key (id), constraint post_fk foreign key (post_id) references post (id)) charset utf8mb4, collate utf8mb4_0900_as_ci;",
 		},
+		// constaint names
+		{
+			schema: `create table t1 (id int primary key, CONSTRAINT const_id CHECK (id > 0))`,
+		},
+		{
+			schema: `create table t1 (id int primary key, CONSTRAINT const_id1 CHECK (id > 0), CONSTRAINT const_id2 CHECK (id < 10));`,
+		},
+		{
+			schema: `
+				create table t1 (id int primary key, CONSTRAINT const_id CHECK (id > 0), CONSTRAINT const_id CHECK (id < 10));
+			`,
+			expectErr: &DuplicateCheckConstraintNameError{Table: "t1", Constraint: "const_id"},
+		},
+		{
+			schema: `
+			create table t1 (id int primary key, CONSTRAINT const_id1 CHECK (id > 0));
+			create table t2 (id int primary key, CONSTRAINT const_id2 CHECK (id > 0));
+			`,
+		},
+		{
+			schema: `
+			create table t1 (id int primary key, CONSTRAINT const_id CHECK (id > 0));
+			create table t2 (id int primary key, CONSTRAINT const_id CHECK (id > 0));
+			`,
+			expectErr: &DuplicateCheckConstraintNameError{Table: "t2", Constraint: "const_id"},
+		},
+		{
+			// OK for foreign key constraint and check constraint to have same name
+			schema: `
+			create table t1 (id int primary key, CONSTRAINT const_id CHECK (id > 0));
+			create table t2 (id int primary key, CONSTRAINT const_id FOREIGN KEY (id) REFERENCES t1 (id) ON DELETE CASCADE);
+			`,
+		},
+		{
+			schema: `
+			create table t1 (id int primary key, CONSTRAINT const_id CHECK (id > 0));
+			create table t2 (id int primary key, CONSTRAINT const_id FOREIGN KEY (id) REFERENCES t1 (id) ON DELETE CASCADE);
+			create table t3 (id int primary key, CONSTRAINT const_id FOREIGN KEY (id) REFERENCES t1 (id) ON DELETE CASCADE);
+			`,
+			expectErr: &DuplicateForeignKeyConstraintNameError{Table: "t3", Constraint: "const_id"},
+		},
+		{
+			schema: `
+			create table t1 (id int primary key, CONSTRAINT const_id CHECK (id > 0));
+			create table t2 (id int primary key, CONSTRAINT const_id FOREIGN KEY (id) REFERENCES t1 (id) ON DELETE CASCADE, CONSTRAINT const_id FOREIGN KEY (id) REFERENCES t1 (id) ON DELETE CASCADE);
+			`,
+			expectErr: &DuplicateForeignKeyConstraintNameError{Table: "t2", Constraint: "const_id"},
+		},
 	}
 	for _, ts := range tt {
 		t.Run(ts.schema, func(t *testing.T) {