Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

match mysql fk name generation #7711

Merged
merged 19 commits into from
Apr 9, 2024
2 changes: 1 addition & 1 deletion go/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ require (
github.com/cespare/xxhash v1.1.0
github.com/creasty/defaults v1.6.0
github.com/dolthub/flatbuffers/v23 v23.3.3-dh.2
github.com/dolthub/go-mysql-server v0.18.2-0.20240409193903-6b139dee1c42
github.com/dolthub/go-mysql-server v0.18.2-0.20240409224550-bfde35b66568
github.com/dolthub/swiss v0.1.0
github.com/goccy/go-json v0.10.2
github.com/google/go-github/v57 v57.0.0
Expand Down
4 changes: 2 additions & 2 deletions go/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,8 @@ github.com/dolthub/fslock v0.0.3 h1:iLMpUIvJKMKm92+N1fmHVdxJP5NdyDK5bK7z7Ba2s2U=
github.com/dolthub/fslock v0.0.3/go.mod h1:QWql+P17oAAMLnL4HGB5tiovtDuAjdDTPbuqx7bYfa0=
github.com/dolthub/go-icu-regex v0.0.0-20230524105445-af7e7991c97e h1:kPsT4a47cw1+y/N5SSCkma7FhAPw7KeGmD6c9PBZW9Y=
github.com/dolthub/go-icu-regex v0.0.0-20230524105445-af7e7991c97e/go.mod h1:KPUcpx070QOfJK1gNe0zx4pA5sicIK1GMikIGLKC168=
github.com/dolthub/go-mysql-server v0.18.2-0.20240409193903-6b139dee1c42 h1:IrKAURAyI9SQt0VVm0U2PkagYsNK9c88CGHGKdSmthE=
github.com/dolthub/go-mysql-server v0.18.2-0.20240409193903-6b139dee1c42/go.mod h1:uTqIti1oPqKEUS9N1zcT43a/QQ8n0PuBdZUMZziBaGU=
github.com/dolthub/go-mysql-server v0.18.2-0.20240409224550-bfde35b66568 h1:H2VdUQThLeXRdfKma4Llh9YjB6Ba76twe9MvHlDj8NM=
github.com/dolthub/go-mysql-server v0.18.2-0.20240409224550-bfde35b66568/go.mod h1:uTqIti1oPqKEUS9N1zcT43a/QQ8n0PuBdZUMZziBaGU=
github.com/dolthub/ishell v0.0.0-20221214210346-d7db0b066488 h1:0HHu0GWJH0N6a6keStrHhUAK5/o9LVfkh44pvsV4514=
github.com/dolthub/ishell v0.0.0-20221214210346-d7db0b066488/go.mod h1:ehexgi1mPxRTk0Mok/pADALuHbvATulTh6gzr7NzZto=
github.com/dolthub/jsonpath v0.0.2-0.20240227200619-19675ab05c71 h1:bMGS25NWAGTEtT5tOBsCuCrlYnLRKpbJVJkDbrTRhwQ=
Expand Down
23 changes: 3 additions & 20 deletions go/libraries/doltcore/doltdb/foreign_key_coll.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import (
"github.com/dolthub/dolt/go/libraries/utils/set"
"github.com/dolthub/dolt/go/store/hash"
"github.com/dolthub/dolt/go/store/types"

"github.com/dolthub/go-mysql-server/sql"
)

// ForeignKeyCollection represents the collection of foreign keys for a root value.
Expand Down Expand Up @@ -343,31 +345,12 @@ func NewForeignKeyCollection(keys ...ForeignKey) (*ForeignKeyCollection, error)
// both column counts are equal. All other validation should occur before being added to the collection.
func (fkc *ForeignKeyCollection) AddKeys(fks ...ForeignKey) error {
for _, key := range fks {
if key.Name == "" {
// assign a name based on the hash
// 8 char = 5 base32 bytes, should be collision resistant
// TODO: constraint names should be unique, and this isn't guaranteed to be.
// This logic needs to live at the table / DB level.
fkHash, err := key.HashOf()
if err != nil {
return err
}
key.Name = fkHash.String()[:8]
}

if _, ok := fkc.GetByNameCaseInsensitive(key.Name); ok {
return fmt.Errorf("a foreign key with the name `%s` already exists", key.Name)
return sql.ErrForeignKeyDuplicateName.New(key.Name)
}
if len(key.TableColumns) != len(key.ReferencedTableColumns) {
return fmt.Errorf("foreign keys must have the same number of columns declared and referenced")
}
if key.IsResolved() {
if _, ok := fkc.GetByTags(key.TableColumns, key.ReferencedTableColumns); ok {
// this differs from MySQL's logic
return fmt.Errorf("a foreign key over columns %v and referenced columns %v already exists",
key.TableColumns, key.ReferencedTableColumns)
}
}

fkHash, err := key.HashOf()
if err != nil {
Expand Down
10 changes: 5 additions & 5 deletions go/libraries/doltcore/doltdb/foreign_key_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ var foreignKeyTests = []foreignKeyTest{
},
fks: []doltdb.ForeignKey{
{
Name: "1ncba7pr",
Name: "child_ibfk_1",
TableName: "child",
TableIndex: "v1_idx",
TableColumns: []uint64{1215},
Expand All @@ -529,7 +529,7 @@ var foreignKeyTests = []foreignKeyTest{
},
fks: []doltdb.ForeignKey{
{
Name: "7nt5f9b0",
Name: "new_table_ibfk_1",
TableName: "new_table",
// unnamed indexes take the column name
TableIndex: "v1",
Expand All @@ -555,7 +555,7 @@ var foreignKeyTests = []foreignKeyTest{
},
fks: []doltdb.ForeignKey{
{
Name: "k48mbatd",
Name: "child_ibfk_1",
TableName: "child",
TableIndex: "v1v2_idx",
TableColumns: []uint64{1215, 8734},
Expand All @@ -581,7 +581,7 @@ var foreignKeyTests = []foreignKeyTest{
},
fks: []doltdb.ForeignKey{
{
Name: "1ncba7pr",
Name: "child_ibfk_1",
TableName: "child",
TableIndex: "v1_idx",
TableColumns: []uint64{1215},
Expand All @@ -594,7 +594,7 @@ var foreignKeyTests = []foreignKeyTest{
},
},
{
Name: "8geddp18",
Name: "child_ibfk_2",
TableName: "child",
TableIndex: "v2_idx",
TableColumns: []uint64{8734},
Expand Down
4 changes: 2 additions & 2 deletions go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ var MergeScripts = []queries.ScriptTest{
{
Query: "SELECT from_root_ish, violation_type, hex(dolt_row_hash), aColumn, bColumn, CAST(violation_info as CHAR) FROM dolt_constraint_violations_aTable;",
Expected: []sql.Row{
{doltCommit, "foreign key", "13F8480978D0556FA9AE6DF5745A7ACA", 2, -1, `{"Columns":["bColumn"],"ForeignKey":"ml92huct","Index":"bColumn","OnDelete":"RESTRICT","OnUpdate":"RESTRICT","ReferencedColumns":["pk"],"ReferencedIndex":"","ReferencedTable":"parent","Table":"aTable"}`},
{doltCommit, "foreign key", "13F8480978D0556FA9AE6DF5745A7ACA", 2, -1, `{"Columns":["bColumn"],"ForeignKey":"atable_ibfk_1","Index":"bColumn","OnDelete":"RESTRICT","OnUpdate":"RESTRICT","ReferencedColumns":["pk"],"ReferencedIndex":"","ReferencedTable":"parent","Table":"aTable"}`},
},
},
{
Expand Down Expand Up @@ -3474,7 +3474,7 @@ var MergeArtifactsScripts = []queries.ScriptTest{
Assertions: []queries.ScriptTestAssertion{
{
Query: "CALL DOLT_MERGE('right');",
ExpectedErrStr: "error storing constraint violation for primary key (( 1 )): another violation already exists\nnew violation: {\"Columns\":[\"col2\"],\"ForeignKey\":\"r8l98srn\",\"Index\":\"col2\",\"OnDelete\":\"RESTRICT\",\"OnUpdate\":\"RESTRICT\",\"ReferencedColumns\":[\"col2\"],\"ReferencedIndex\":\"par_col2_idx\",\"ReferencedTable\":\"parent\",\"Table\":\"child\"} old violation: ({\"Columns\":[\"col1\"],\"ForeignKey\":\"ut564qa1\",\"Index\":\"col1\",\"OnDelete\":\"RESTRICT\",\"OnUpdate\":\"RESTRICT\",\"ReferencedColumns\":[\"col1\"],\"ReferencedIndex\":\"par_col1_idx\",\"ReferencedTable\":\"parent\",\"Table\":\"child\"})",
ExpectedErrStr: "error storing constraint violation for primary key (( 1 )): another violation already exists\nnew violation: {\"Columns\":[\"col1\"],\"ForeignKey\":\"child_ibfk_1\",\"Index\":\"col1\",\"OnDelete\":\"RESTRICT\",\"OnUpdate\":\"RESTRICT\",\"ReferencedColumns\":[\"col1\"],\"ReferencedIndex\":\"par_col1_idx\",\"ReferencedTable\":\"parent\",\"Table\":\"child\"} old violation: ({\"Columns\":[\"col2\"],\"ForeignKey\":\"child_ibfk_2\",\"Index\":\"col2\",\"OnDelete\":\"RESTRICT\",\"OnUpdate\":\"RESTRICT\",\"ReferencedColumns\":[\"col2\"],\"ReferencedIndex\":\"par_col2_idx\",\"ReferencedTable\":\"parent\",\"Table\":\"child\"})",
},
{
Query: "SELECT * from parent;",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1998,15 +1998,15 @@ var DoltConstraintViolationTransactionTests = []queries.TransactionTest{
"To allow transactions to be committed with constraint violations from a merge or transaction sequencing set @@dolt_force_transaction_commit=1.\n" +
"Constraint violations: \n" +
"Type: Foreign Key Constraint Violation\n" +
"\tForeignKey: r4cf97vs,\n" +
"\tForeignKey: child_ibfk_1,\n" +
"\tTable: child,\n" +
"\tReferencedTable: ,\n" +
"\tIndex: parent_fk,\n" +
"\tReferencedIndex: ",
},
{
Query: "/* client b */ INSERT INTO child VALUES (1, 1);",
ExpectedErrStr: "cannot add or update a child row - Foreign key violation on fk: `r4cf97vs`, table: `child`, referenced table: `parent`, key: `[1]`",
ExpectedErrStr: "cannot add or update a child row - Foreign key violation on fk: `child_ibfk_1`, table: `child`, referenced table: `parent`, key: `[1]`",
},
},
},
Expand Down
4 changes: 2 additions & 2 deletions integration-tests/bats/diff.bats
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ SQL

run dolt diff
[ "$status" -eq 0 ]
[[ "$output" =~ "+ CONSTRAINT \`qvvgfpe7\` FOREIGN KEY (\`j\`) REFERENCES \`parent\` (\`i\`)" ]] || false
[[ "$output" =~ "+ CONSTRAINT \`child_ibfk_1\` FOREIGN KEY (\`j\`) REFERENCES \`parent\` (\`i\`)" ]] || false
}

@test "diff: new foreign key added without foreign key check, and does not resolve" {
Expand All @@ -695,7 +695,7 @@ SQL

run dolt diff
[ "$status" -eq 0 ]
[[ ! "$output" =~ "+ CONSTRAINT \`npsbmr30\` FOREIGN KEY (\`j\`) REFERENCES \`parent\` (\`i\`)" ]] || false
[[ ! "$output" =~ "+ CONSTRAINT \`child_ibfk_1\` FOREIGN KEY (\`j\`) REFERENCES \`parent\` (\`i\`)" ]] || false
}

@test "diff: existing foreign key that was resolved is deleted" {
Expand Down
47 changes: 43 additions & 4 deletions integration-tests/bats/foreign-keys.bats
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,17 @@ SQL
run dolt sql -q "select constraint_name, table_name, column_name, ordinal_position, position_in_unique_constraint, referenced_table_name, referenced_column_name from information_schema.KEY_COLUMN_USAGE;" -r csv
[[ "$output" =~ "PRIMARY,colors,id,1,,," ]] || false
[[ "$output" =~ "PRIMARY,materials,id,1,,," ]] || false
[[ "$output" =~ "npjt0g99,materials,color,1,1,colors,color" ]] || false
[[ "$output" =~ "materials_ibfk_1,materials,color,1,1,colors,color" ]] || false
[[ "$output" =~ "PRIMARY,objects,id,1,,," ]] || false
[[ "$output" =~ "ootftvit,objects,color,1,1,materials,color" ]] || false
[[ "$output" =~ "ootftvit,objects,material,2,2,materials,material" ]] || false
[[ "$output" =~ "objects_ibfk_1,objects,color,1,1,materials,color" ]] || false
[[ "$output" =~ "objects_ibfk_1,objects,material,2,2,materials,material" ]] || false
[[ "$output" =~ "PRIMARY,child,id,1,,," ]] || false
[[ "$output" =~ "PRIMARY,parent,id,1,,," ]] || false

# check information_schema.TABLE_CONSTRAINTS table
run dolt sql -q "select * from information_schema.TABLE_CONSTRAINTS where table_name = 'materials';" -r csv
[[ "$output" =~ "def,dolt-repo-$$,PRIMARY,dolt-repo-$$,materials,PRIMARY KEY,YES" ]] || false
[[ "$output" =~ "def,dolt-repo-$$,npjt0g99,dolt-repo-$$,materials,FOREIGN KEY,YES" ]] || false
[[ "$output" =~ "def,dolt-repo-$$,materials_ibfk_1,dolt-repo-$$,materials,FOREIGN KEY,YES" ]] || false

# check information_schema.TABLE_CONSTRAINTS_EXTENSIONS table
run dolt sql -q "select constraint_name from information_schema.TABLE_CONSTRAINTS_EXTENSIONS where table_name = 'materials';" -r csv
Expand Down Expand Up @@ -1289,6 +1289,45 @@ SQL
[[ "$output" =~ "child,1" ]] || false
}

@test "foreign-keys: different foreign keys with same name is schema conflict" {
dolt commit -Am "initial commit"

dolt checkout -b other
dolt sql -q "alter table child add foreign key (id) references parent (v1)"
dolt commit -Am "other"

dolt checkout main
dolt sql -q "alter table child add foreign key (id) references parent (v2)"
dolt commit -Am "main"

run dolt merge other
[ "$status" -eq "1" ]
[[ "$output" =~ "duplicate foreign key constraint name" ]] || false

run dolt sql -q "alter table child rename constraint foreign key child_ibfk_1 to child_ibfk_2"
[ "$status" -eq "0" ]

dolt commit -Am "rename"

run dolt merge other
[ "$status" -eq "0" ]
}

@test "foreign-keys: same foreign keys with same name is ok" {
dolt commit -Am "initial commit"

dolt checkout -b other
dolt sql -q "alter table child add foreign key (id) references parent (id)"
dolt commit -Am "other"

dolt checkout main
dolt sql -q "alter table child add foreign key (id) references parent (id)"
dolt commit -Am "main"

run dolt merge other
[ "$status" -eq "0" ]
}

@test "foreign-keys: Resolve catches violations" {
dolt sql <<SQL
ALTER TABLE child ADD CONSTRAINT fk_v1 FOREIGN KEY (v1) REFERENCES parent(v1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ export const diffTests = [
" `test_pk` int,\n" +
" PRIMARY KEY (`id`),\n" +
" KEY `test_pk` (`test_pk`),\n" +
" CONSTRAINT `lfbtivir` FOREIGN KEY (`test_pk`) REFERENCES `test` (`pk`)\n" +
" CONSTRAINT `test_info_ibfk_1` FOREIGN KEY (`test_pk`) REFERENCES `test` (`pk`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin;",
to_create_statement: "",
},
Expand Down Expand Up @@ -306,7 +306,7 @@ export const diffTests = [
" `test_pk` int,\n" +
" PRIMARY KEY (`id`),\n" +
" KEY `test_pk` (`test_pk`),\n" +
" CONSTRAINT `lfbtivir` FOREIGN KEY (`test_pk`) REFERENCES `test` (`pk`)\n" +
" CONSTRAINT `test_info_ibfk_1` FOREIGN KEY (`test_pk`) REFERENCES `test` (`pk`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_bin;",
to_create_statement: "",
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ export const tableTests = [
{
CONSTRAINT_CATALOG: "def",
CONSTRAINT_SCHEMA: `${dbName}/main`,
CONSTRAINT_NAME: "lfbtivir",
CONSTRAINT_NAME: "test_info_ibfk_1",
TABLE_CATALOG: "def",
TABLE_SCHEMA: `${dbName}/main`,
TABLE_NAME: "test_info",
Expand Down
Loading