diff --git a/go/libraries/doltcore/merge/violations_fk_prolly.go b/go/libraries/doltcore/merge/violations_fk_prolly.go index df0b6440ab3..d24f56d853c 100644 --- a/go/libraries/doltcore/merge/violations_fk_prolly.go +++ b/go/libraries/doltcore/merge/violations_fk_prolly.go @@ -32,6 +32,10 @@ import ( "github.com/dolthub/dolt/go/store/val" ) +// prollyParentSecDiffFkConstraintViolations emits FK violations for rows removed from the +// parent table. It diffs |preParentSecIdx| against |postParent|'s secondary index. For each +// removed or modified row, it checks whether an equivalent parent value still exists, then +// scans the child's secondary index for child rows that reference the removed value. func prollyParentSecDiffFkConstraintViolations( ctx context.Context, foreignKey doltdb.ForeignKey, @@ -42,26 +46,22 @@ func prollyParentSecDiffFkConstraintViolations( if err != nil { return err } - postParentSecIdx, err := durable.ProllyMapFromIndex(postParent.IndexData) + postParentSecIdx, _, parentIdxPrefixDesc, err := fkIdxKeyDescs(postParent.IndexData, len(foreignKey.TableColumns)) if err != nil { return err } - childSecIdx, err := durable.ProllyMapFromIndex(postChild.IndexData) - if err != nil { - return err - } - - parentSecIdxDesc, _ := postParentSecIdx.Descriptors() - parentIdxPrefixDesc := parentSecIdxDesc.PrefixDesc(len(foreignKey.TableColumns)) parentIdxKb := val.NewTupleBuilder(parentIdxPrefixDesc, postParentRowData.NodeStore()) childPriIdx, err := durable.ProllyMapFromIndex(postChild.RowData) if err != nil { return err } - childPriIdxDesc, _ := childPriIdx.Descriptors() - childSecIdxDesc, _ := childSecIdx.Descriptors() + + childSecIdx, childSecIdxDesc, childSecIdxPrefixDesc, err := fkIdxKeyDescs(postChild.IndexData, len(foreignKey.TableColumns)) + if err != nil { + return err + } childPrimary := &indexAndKeyDescriptor{ index: childPriIdx, @@ -76,7 +76,7 @@ func prollyParentSecDiffFkConstraintViolations( // We allow foreign keys between types that don't have the same serialization bytes for the same logical values // in some contexts. If this lookup is one of those, we need to convert the child key to the parent key format. - compatibleTypes := foreignKeysAreCompatibleTypes(parentIdxPrefixDesc, childSecIdxDesc) + compatibleTypes := fkHandlersAreSerializationCompatible(parentIdxPrefixDesc, childSecIdxPrefixDesc) // TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed. considerAllRowsModified := false @@ -127,6 +127,10 @@ func prollyParentSecDiffFkConstraintViolations( return nil } +// prollyParentPriDiffFkConstraintViolations emits FK violations for rows removed from the +// parent table when the parent's secondary index was not present in the merge ancestor. It +// diffs |preParentRowData| against |postParent|'s primary index and applies the same +// checks as prollyParentSecDiffFkConstraintViolations. func prollyParentPriDiffFkConstraintViolations( ctx context.Context, foreignKey doltdb.ForeignKey, @@ -137,41 +141,37 @@ func prollyParentPriDiffFkConstraintViolations( if err != nil { return err } - postParentIndexData, err := durable.ProllyMapFromIndex(postParent.IndexData) + postParentIndexData, _, partialDesc, err := fkIdxKeyDescs(postParent.IndexData, len(foreignKey.TableColumns)) if err != nil { return err } - - idxDesc, _ := postParentIndexData.Descriptors() - partialDesc := idxDesc.PrefixDesc(len(foreignKey.TableColumns)) partialKB := val.NewTupleBuilder(partialDesc, postParentRowData.NodeStore()) childPriIdx, err := durable.ProllyMapFromIndex(postChild.RowData) if err != nil { return err } - childScndryIdx, err := durable.ProllyMapFromIndex(postChild.IndexData) + childPriIdxDesc, _ := childPriIdx.Descriptors() + + childSecIdx, childSecIdxDesc, childSecIdxPrefixDesc, err := fkIdxKeyDescs(postChild.IndexData, len(foreignKey.TableColumns)) if err != nil { return err } - childPriIdxDesc, _ := childPriIdx.Descriptors() - childSecIdxDesc, _ := childScndryIdx.Descriptors() - childPrimary := &indexAndKeyDescriptor{ index: childPriIdx, keyDesc: childPriIdxDesc, schema: postChild.Schema, } childSecondary := &indexAndKeyDescriptor{ - index: childScndryIdx, + index: childSecIdx, keyDesc: childSecIdxDesc, schema: postChild.IndexSchema, } // We allow foreign keys between types that don't have the same serialization bytes for the same logical values // in some contexts. If this lookup is one of those, we need to convert the child key to the parent key format. - compatibleTypes := foreignKeysAreCompatibleTypes(partialDesc, childSecIdxDesc) + compatibleTypes := fkHandlersAreSerializationCompatible(partialDesc, childSecIdxPrefixDesc) // TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed. considerAllRowsModified := false @@ -231,6 +231,11 @@ func prollyParentPriDiffFkConstraintViolations( return nil } +// prollyChildPriDiffFkConstraintViolations emits FK violations for rows added to or modified +// in the child table when the child's secondary index was not present in the merge ancestor. +// It diffs |preChildRowData| against |postChild|'s primary index. For each new or changed +// row, it builds a parent lookup key from the FK columns and verifies that a matching parent +// row exists. func prollyChildPriDiffFkConstraintViolations( ctx context.Context, foreignKey doltdb.ForeignKey, @@ -241,19 +246,20 @@ func prollyChildPriDiffFkConstraintViolations( if err != nil { return err } - parentSecondaryIdx, err := durable.ProllyMapFromIndex(postParent.IndexData) + parentSecondaryIdx, _, parentIdxPrefixDesc, err := fkIdxKeyDescs(postParent.IndexData, len(foreignKey.TableColumns)) if err != nil { return err } - - childPriIdxDesc, _ := postChildRowData.Descriptors() - parentIdxDesc, _ := parentSecondaryIdx.Descriptors() - parentIdxPrefixDesc := parentIdxDesc.PrefixDesc(len(foreignKey.TableColumns)) partialKB := val.NewTupleBuilder(parentIdxPrefixDesc, postChildRowData.NodeStore()) + _, _, childFkColsDesc, err := fkIdxKeyDescs(postChild.IndexData, len(foreignKey.TableColumns)) + if err != nil { + return err + } + // We allow foreign keys between types that don't have the same serialization bytes for the same logical values // in some contexts. If this lookup is one of those, we need to convert the child key to the parent key format. - compatibleTypes := foreignKeysAreCompatibleTypes(childPriIdxDesc, parentIdxPrefixDesc) + compatibleTypes := fkHandlersAreSerializationCompatible(childFkColsDesc, parentIdxPrefixDesc) // TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed. considerAllRowsModified := false @@ -277,7 +283,7 @@ func prollyChildPriDiffFkConstraintViolations( } if !compatibleTypes { - parentLookupKey, err = convertKeyBetweenTypes(ctx, parentLookupKey, childPriIdxDesc, parentIdxPrefixDesc, parentSecondaryIdx.NodeStore(), parentSecondaryIdx.Pool()) + parentLookupKey, err = convertKeyBetweenTypes(ctx, parentLookupKey, childFkColsDesc, parentIdxPrefixDesc, parentSecondaryIdx.NodeStore(), parentSecondaryIdx.Pool()) if err != nil { return err } @@ -300,6 +306,9 @@ func prollyChildPriDiffFkConstraintViolations( return nil } +// prollyChildSecDiffFkConstraintViolations emits FK violations for rows added to or modified +// in the child table. It diffs |preChildSecIdx| against |postChild|'s secondary index. For +// each new or changed row, it verifies that a matching parent row exists. func prollyChildSecDiffFkConstraintViolations( ctx context.Context, foreignKey doltdb.ForeignKey, @@ -311,24 +320,19 @@ func prollyChildSecDiffFkConstraintViolations( if err != nil { return err } - postChildSecIdx, err := durable.ProllyMapFromIndex(postChild.IndexData) + postChildSecIdx, _, childIdxPrefixDesc, err := fkIdxKeyDescs(postChild.IndexData, len(foreignKey.TableColumns)) if err != nil { return err } - parentSecIdx, err := durable.ProllyMapFromIndex(postParent.IndexData) + parentSecIdx, _, parentIdxPrefixDesc, err := fkIdxKeyDescs(postParent.IndexData, len(foreignKey.TableColumns)) if err != nil { return err } - - parentSecIdxDesc, _ := parentSecIdx.Descriptors() - parentIdxPrefixDesc := parentSecIdxDesc.PrefixDesc(len(foreignKey.TableColumns)) childPriKD, _ := postChildRowData.Descriptors() - childIdxDesc, _ := postChildSecIdx.Descriptors() - childIdxPrefixDesc := childIdxDesc.PrefixDesc(len(foreignKey.TableColumns)) // We allow foreign keys between types that don't have the same serialization bytes for the same logical values // in some contexts. If this lookup is one of those, we need to convert the child key to the parent key format. - compatibleTypes := foreignKeysAreCompatibleTypes(childIdxPrefixDesc, parentIdxPrefixDesc) + compatibleTypes := fkHandlersAreSerializationCompatible(childIdxPrefixDesc, parentIdxPrefixDesc) // TODO: Determine whether we should surface every row as a diff when the map's value descriptor has changed. considerAllRowsModified := false @@ -372,17 +376,36 @@ func prollyChildSecDiffFkConstraintViolations( return nil } -// foreignKeysAreCompatibleTypes returns whether the serializations for two tuple descriptors are binary compatible -func foreignKeysAreCompatibleTypes(keyDescA, keyDescB *val.TupleDesc) bool { - compatibleTypes := true +// fkIdxKeyDescs loads |idx| as a prolly map and returns the map, its full key descriptor, +// and a prefix descriptor covering the first |n| columns. +// +// The full descriptor is used when constructing indexAndKeyDescriptor values. The prefix +// descriptor is used for tuple builder construction and for serialization compatibility +// checks via fkHandlersAreSerializationCompatible. +func fkIdxKeyDescs(idx durable.Index, n int) (prolly.Map, *val.TupleDesc, *val.TupleDesc, error) { + prollyMap, err := durable.ProllyMapFromIndex(idx) + if err != nil { + return prolly.Map{}, nil, nil, err + } + kd, _ := prollyMap.Descriptors() + return prollyMap, kd, kd.PrefixDesc(n), nil +} + +// fkHandlersAreSerializationCompatible reports whether |keyDescA| and |keyDescB| have +// compatible type handlers at every position. Both descriptors must have equal length and +// their handlers must be in FK column order. Callers are responsible for trimming descriptors +// to the FK column count before calling this function. +// +// When any non-nil handler pair is incompatible, the child key must be converted to the +// parent key format before the parent index lookup. See convertKeyBetweenTypes. +func fkHandlersAreSerializationCompatible(keyDescA, keyDescB *val.TupleDesc) bool { for i, handlerA := range keyDescA.Handlers { handlerB := keyDescB.Handlers[i] if handlerA != nil && handlerB != nil && !handlerA.SerializationCompatible(handlerB) { - compatibleTypes = false - break + return false } } - return compatibleTypes + return true } // convertSerializedFkField converts a serialized foreign key value from one type handler to another. diff --git a/go/libraries/doltcore/merge/violations_fk_prolly_test.go b/go/libraries/doltcore/merge/violations_fk_prolly_test.go new file mode 100644 index 00000000000..b385f3ea5ed --- /dev/null +++ b/go/libraries/doltcore/merge/violations_fk_prolly_test.go @@ -0,0 +1,189 @@ +// Copyright 2026 Dolthub, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package merge + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dolthub/dolt/go/libraries/doltcore/doltdb" + "github.com/dolthub/dolt/go/libraries/doltcore/doltdb/durable" + "github.com/dolthub/dolt/go/libraries/doltcore/dtestutils" + "github.com/dolthub/dolt/go/libraries/doltcore/schema" + "github.com/dolthub/dolt/go/libraries/doltcore/schema/typeinfo" + "github.com/dolthub/dolt/go/store/prolly" + "github.com/dolthub/dolt/go/store/prolly/tree" + "github.com/dolthub/dolt/go/store/val" +) + +// TestFkIdxKeyDescs_FkColNotAtFront verifies that fkIdxKeyDescs returns a prefix descriptor +// with handlers in FK column order when the FK column is not at position 0 in the child's +// primary key. +// +// The test creates real Dolt tables so that IndexData is obtained through the same calls +// newConstraintViolationsLoadedTable makes: GetIndexRowData for the child secondary index +// and GetRowData for the primary and parent indexes. +// +// Because Dolt does not assign TupleTypeHandlers for standard column types, the test +// re-wraps each production TupleDesc with custom handlers injected at positions determined +// by AllTags. This makes handler-position mismatches observable without altering the +// type ordering that came from the production path. +func TestFkIdxKeyDescs_FkColNotAtFront(t *testing.T) { + ctx := context.Background() + ddb := dtestutils.CreateTestEnv().DoltDB(ctx) + vrw := ddb.ValueReadWriter() + ns := ddb.NodeStore() + + handlerA := fkTestHandler{id: 1} + handlerB := fkTestHandler{id: 2} + + const ( + pk1Tag uint64 = 1 + pk2Tag uint64 = 2 + fkColTag uint64 = 3 + parentPk uint64 = 4 + ) + tagToHandler := map[uint64]val.TupleTypeHandler{ + fkColTag: handlerB, + pk1Tag: handlerA, + pk2Tag: handlerA, + parentPk: handlerA, + } + + // Build child schema: PRIMARY KEY(pk1 INT, pk2 INT), fk_col VARCHAR. + childCols := schema.NewColCollection( + mustCol(t, "pk1", pk1Tag, typeinfo.Int32Type, true), + mustCol(t, "pk2", pk2Tag, typeinfo.Int32Type, true), + mustCol(t, "fk_col", fkColTag, typeinfo.StringDefaultType, false), + ) + childSch := schema.MustSchemaFromCols(childCols) + + // AddIndexByColTags calls combineAllTags([fkColTag], [pk1Tag, pk2Tag]) internally, + // producing allTags = [fkColTag, pk1Tag, pk2Tag]. fk_col is third in the schema + // definition above but moves to position 0 in AllTags because it is the indexed column. + const childIdxName = "idx_fk_col" + childSchIdx, err := childSch.Indexes().AddIndexByColTags(childIdxName, []uint64{fkColTag}, nil, schema.IndexProperties{IsUserDefined: true}) + require.NoError(t, err) + require.Equal(t, fkColTag, childSchIdx.AllTags()[0], + "fk_col must be at position 0 in AllTags even though it is third in the schema definition") + + parentCols := schema.NewColCollection( + mustCol(t, "pk", parentPk, typeinfo.Int32Type, true), + ) + parentSch := schema.MustSchemaFromCols(parentCols) + + childTbl, err := doltdb.NewEmptyTable(ctx, vrw, ns, childSch) + require.NoError(t, err) + parentTbl, err := doltdb.NewEmptyTable(ctx, vrw, ns, parentSch) + require.NoError(t, err) + + // Load IndexData the same way newConstraintViolationsLoadedTable does. + childSecIdxData, err := childTbl.GetIndexRowData(ctx, childIdxName) + require.NoError(t, err) + childPriIdxData, err := childTbl.GetRowData(ctx) + require.NoError(t, err) + parentIdxData, err := parentTbl.GetRowData(ctx) + require.NoError(t, err) + + // Inject custom handlers into each production TupleDesc. The type ordering comes from + // the production path. AllTags drives handler assignment for the secondary index so that + // the FK column's handler occupies the position combineAllTags placed it at. + childSecIdxData = withHandlers(t, ctx, ns, childSecIdxData, handlersFromTags(childSchIdx.AllTags(), tagToHandler)) + childPriIdxData = withHandlers(t, ctx, ns, childPriIdxData, handlersFromTags([]uint64{pk1Tag, pk2Tag}, tagToHandler)) + parentIdxData = withHandlers(t, ctx, ns, parentIdxData, handlersFromTags([]uint64{parentPk}, tagToHandler)) + + fkColCount := 1 + + _, _, parentPrefixDesc, err := fkIdxKeyDescs(parentIdxData, fkColCount) + require.NoError(t, err) + + _, childSecFullDesc, childFkColsDesc, err := fkIdxKeyDescs(childSecIdxData, fkColCount) + require.NoError(t, err) + + assert.Greater(t, len(childSecFullDesc.Handlers), len(childFkColsDesc.Handlers), + "prefix descriptor must be shorter than the full secondary index descriptor") + assert.Equal(t, len(childFkColsDesc.Handlers), len(parentPrefixDesc.Handlers), + "child and parent prefix descriptors must have equal length") + assert.False(t, + fkHandlersAreSerializationCompatible(childFkColsDesc, parentPrefixDesc), + "fk_col (handlerB) is not compatible with parent.pk (handlerA): conversion required", + ) + + // Using the primary index instead produces a false positive: pk1 sits at position 0 + // with handlerA, which matches parent.pk (handlerA), so the check reports compatible + // and conversion is silently skipped. + _, _, childPriPrefixDesc, err := fkIdxKeyDescs(childPriIdxData, fkColCount) + require.NoError(t, err) + assert.True(t, + fkHandlersAreSerializationCompatible(childPriPrefixDesc, parentPrefixDesc), + "pk1 (handlerA) matches parent.pk (handlerA): primary index produces a false positive, fk_col conversion is incorrectly skipped", + ) +} + +// handlersFromTags returns a handler slice ordered by |tags|, looking each up in |tagToHandler|. +func handlersFromTags(tags []uint64, tagToHandler map[uint64]val.TupleTypeHandler) []val.TupleTypeHandler { + handlers := make([]val.TupleTypeHandler, len(tags)) + for position, tag := range tags { + handlers[position] = tagToHandler[tag] + } + return handlers +} + +// withHandlers re-wraps |index| as a new [durable.Index] whose key TupleDesc carries |handlers|. +// The type encodings are taken from the existing descriptor so the production column ordering +// is preserved. +func withHandlers(t *testing.T, ctx context.Context, nodeStore tree.NodeStore, index durable.Index, handlers []val.TupleTypeHandler) durable.Index { + t.Helper() + prollyMap, err := durable.ProllyMapFromIndex(index) + require.NoError(t, err) + keyDesc, valDesc := prollyMap.Descriptors() + patchedKeyDesc := val.NewTupleDescriptorWithArgs(val.TupleDescriptorArgs{Handlers: handlers}, keyDesc.Types...) + patchedMap, err := prolly.NewMapFromTuples(ctx, nodeStore, patchedKeyDesc, valDesc) + require.NoError(t, err) + return durable.IndexFromProllyMap(patchedMap) +} + +// mustCol creates a [schema.Column] with the given typeinfo, panicking on error. +func mustCol(t *testing.T, name string, tag uint64, ti typeinfo.TypeInfo, partOfPK bool) schema.Column { + t.Helper() + col, err := schema.NewColumnWithTypeInfo(name, tag, ti, partOfPK, "", false, "") + require.NoError(t, err) + return col +} + +// fkTestHandler is a minimal [val.TupleTypeHandler] whose SerializationCompatible returns true +// only when both handlers share the same id. +type fkTestHandler struct{ id int } + +func (m fkTestHandler) SerializedCompare(_ context.Context, _, _ []byte) (int, error) { + return 0, nil +} +func (m fkTestHandler) SerializeValue(_ context.Context, _ any) ([]byte, error) { + return nil, nil +} +func (m fkTestHandler) DeserializeValue(_ context.Context, _ []byte) (any, error) { + return nil, nil +} +func (m fkTestHandler) FormatValue(_ any) (string, error) { return "", nil } +func (m fkTestHandler) SerializationCompatible(other val.TupleTypeHandler) bool { + o, ok := other.(fkTestHandler) + return ok && o.id == m.id +} +func (m fkTestHandler) ConvertSerialized(_ context.Context, _ val.TupleTypeHandler, v []byte) ([]byte, error) { + return v, nil +} diff --git a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go index 91d71bcc49e..5b642035324 100644 --- a/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go +++ b/go/libraries/doltcore/sqle/enginetest/dolt_queries_merge.go @@ -3209,6 +3209,54 @@ var MergeScripts = []queries.ScriptTest{ }, }, }, + { + // https://github.com/dolthub/dolt/issues/10676 + Name: "Merge does not panic when FK is dropped and re-added on one branch and child has composite PK with mixed types", + SetUpScript: []string{ + `CREATE TABLE parent (pk VARCHAR(20) PRIMARY KEY);`, + `CREATE TABLE child ( + pk1 INT, + pk2 BIGINT, + fk_col VARCHAR(20), + PRIMARY KEY (pk1, pk2), + CONSTRAINT fk1 FOREIGN KEY (fk_col) REFERENCES parent (pk) + );`, + `INSERT INTO parent VALUES ('a');`, + `INSERT INTO child VALUES (1, 1, 'a');`, + `CALL DOLT_COMMIT('-Am', 'initial setup');`, + `CALL DOLT_BRANCH('other_branch');`, + // Drop FK then drop its backing index so re-adding creates a fresh index with a new name. + // If we only drop the FK (not the index), re-adding reuses the old index name, which still + // exists in the merge ancestor and avoids the bug. + `ALTER TABLE child DROP FOREIGN KEY fk1;`, + `ALTER TABLE child DROP INDEX fk1;`, + `CALL DOLT_COMMIT('-Am', 'drop fk and its backing index');`, + `ALTER TABLE child ADD CONSTRAINT fk2 FOREIGN KEY (fk_col) REFERENCES parent (pk);`, + `CALL DOLT_COMMIT('-Am', 're-add fk, creates new backing index fk2');`, + `CALL DOLT_CHECKOUT('other_branch');`, + `INSERT INTO child VALUES (1, 2, 'a');`, + `CALL DOLT_COMMIT('-Am', 'add child row on other branch');`, + }, + Assertions: []queries.ScriptTestAssertion{ + { + Query: "CALL DOLT_MERGE('main')", + Expected: []sql.Row{{doltCommit, 0, 0, "merge successful"}}, + }, + { + Query: "SELECT pk FROM parent ORDER BY pk;", + Expected: []sql.Row{{"a"}}, + }, + { + Query: "SELECT pk1, pk2, fk_col FROM child ORDER BY pk1, pk2;", + Expected: []sql.Row{{1, 1, "a"}, {1, 2, "a"}}, + }, + { + Query: "INSERT INTO child VALUES (2, 1, 'z');", + // The re-added FK constraint (fk2) must still be enforced after the merge. + ExpectedErr: sql.ErrForeignKeyChildViolation, + }, + }, + }, } var KeylessMergeCVsAndConflictsScripts = []queries.ScriptTest{