Repository: cloudspannerecosystem/harbourbridge
Pull request #168 (2021-09-15 18:48:55)

Handle name collisions differing only in case
Store and compare all names in the usedNames map in lower case. 

MySQL allows table names with that differ only in case which is not allowed in spanner. That will be picked up in a separate PR since the table name collision function is used in many places.

agasheesh: Thanks for catching this and sending out a fix. Can you please also add test cases that would have failed without this fix? Such that we can catch any future regression in the behavior.
Deep1998: Modified dump files so integration test covers this case
agasheesh: The changes in this LGTM. I think it's worth adding changes for handling name collision for table in this PR itself so that we can reason about name collisions across table, index and fk.

Patch:
diff --git a/dynamodb/toddl.go b/dynamodb/toddl.go
index 1aa6844..1e9e957 100644
--- a/dynamodb/toddl.go
+++ b/dynamodb/toddl.go
@@ -28,19 +28,6 @@ import (
 // Spanner. It uses the source schema in conv.SrcSchema, and writes
 // the Spanner schema to conv.SpSchema.
 func schemaToDDL(conv *internal.Conv) error {
-	// Tracks Spanner names that have been used for indexes. We use this to ensure we generate unique names when
-	// we map from DynamoDB to Spanner since we want Spanner table names and index names to be distinct.
-	usedNames := make(map[string]bool)
-	// We need to pre-populate usedNames with Spanner table names to handle collisions
-	// with table names, foreign key names and index names.
-	for _, srcTable := range conv.SrcSchema {
-		spTableName, err := internal.GetSpannerTable(conv, srcTable.Name)
-		if err != nil {
-			conv.Unexpected(fmt.Sprintf("Couldn't map source table %s to Spanner: %s", srcTable.Name, err))
-			continue
-		}
-		usedNames[spTableName] = true
-	}
 	for _, srcTable := range conv.SrcSchema {
 		spTableName, err := internal.GetSpannerTable(conv, srcTable.Name)
 		if err != nil {
@@ -77,7 +64,7 @@ func schemaToDDL(conv *internal.Conv) error {
 			ColNames: spColNames,
 			ColDefs:  spColDef,
 			Pks:      cvtPrimaryKeys(conv, srcTable.Name, srcTable.PrimaryKeys),
-			Indexes:  cvtIndexes(conv, spTableName, srcTable.Name, srcTable.Indexes, usedNames),
+			Indexes:  cvtIndexes(conv, spTableName, srcTable.Name, srcTable.Indexes),
 			Comment:  comment}
 	}
 	return nil
@@ -131,7 +118,7 @@ func cvtPrimaryKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Key)
 	return spKeys
 }
 
-func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcIndexes []schema.Index, usedNames map[string]bool) []ddl.CreateIndex {
+func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcIndexes []schema.Index) []ddl.CreateIndex {
 	var spIndexes []ddl.CreateIndex
 	for _, srcIndex := range srcIndexes {
 		var spKeys []ddl.IndexKey
@@ -143,7 +130,7 @@ func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcInd
 			}
 			spKeys = append(spKeys, ddl.IndexKey{Col: spCol, Desc: k.Desc})
 		}
-		spIndexName := internal.ToSpannerIndexName(srcIndex.Name, usedNames)
+		spIndexName := internal.ToSpannerIndexName(conv, srcIndex.Name)
 		spIndex := ddl.CreateIndex{
 			Name:   spIndexName,
 			Table:  spTableName,
diff --git a/internal/convert.go b/internal/convert.go
index 80a7db6..91ba61d 100644
--- a/internal/convert.go
+++ b/internal/convert.go
@@ -31,6 +31,7 @@ type Conv struct {
 	Issues         map[string]map[string][]SchemaIssue // Maps source-DB table/col to list of schema conversion issues.
 	ToSpanner      map[string]NameAndCols              // Maps from source-DB table name to Spanner name and column mapping.
 	ToSource       map[string]NameAndCols              // Maps from Spanner table name to source-DB table name and column mapping.
+	UsedNames      map[string]bool                     // Map storing the names that are already assigned to tables, indices or foreign key contraints.
 	dataSink       func(table string, cols []string, values []interface{})
 	Location       *time.Location // Timezone (for timestamp conversion).
 	sampleBadRows  rowSamples     // Rows that generated errors during conversion.
@@ -128,6 +129,7 @@ func MakeConv() *Conv {
 		Issues:         make(map[string]map[string][]SchemaIssue),
 		ToSpanner:      make(map[string]NameAndCols),
 		ToSource:       make(map[string]NameAndCols),
+		UsedNames:      make(map[string]bool),
 		Location:       time.Local, // By default, use go's local time, which uses $TZ (when set).
 		sampleBadRows:  rowSamples{bytesLimit: 10 * 1000 * 1000},
 		Stats: stats{
diff --git a/internal/mapping.go b/internal/mapping.go
index 0bfb82f..cd28d27 100644
--- a/internal/mapping.go
+++ b/internal/mapping.go
@@ -30,29 +30,20 @@ import (
 // a) the new table name is legal
 // b) the new table name doesn't clash with other Spanner table names
 // c) we consistently return the same name for this table.
+//
+// conv.UsedNames tracks Spanner names that have been used for table names, foreign key constraints
+// and indexes. We use this to ensure we generate unique names when
+// we map from source dbs to Spanner since Spanner requires all these names to be
+// distinct and should not differ only in case.
 func GetSpannerTable(conv *Conv, srcTable string) (string, error) {
 	if srcTable == "" {
 		return "", fmt.Errorf("bad parameter: table string is empty")
 	}
+	// Once computed, return cached result.
 	if sp, found := conv.ToSpanner[srcTable]; found {
 		return sp.Name, nil
 	}
-	spTable, _ := FixName(srcTable)
-	if _, found := conv.ToSource[spTable]; found {
-		// s has been used before i.e. FixName caused a collision.
-		// Add unique postfix: use number of tables so far.
-		// However, there is a chance this has already been used,
-		// so need to iterate.
-		id := len(conv.ToSpanner)
-		for {
-			t := spTable + "_" + strconv.Itoa(id)
-			if _, found := conv.ToSource[t]; !found {
-				spTable = t
-				break
-			}
-			id++
-		}
-	}
+	spTable := getSpannerId(conv, srcTable)
 	if spTable != srcTable {
 		VerbosePrintf("Mapping source DB table %s to Spanner table %s\n", srcTable, spTable)
 	}
@@ -144,11 +135,11 @@ func GetSpannerCols(conv *Conv, srcTable string, srcCols []string) ([]string, er
 // (across the database). But in some source databases, such as PostgreSQL,
 // they only have to be unique for a table. Hence we must map each source
 // constraint name to a unique spanner constraint name.
-func ToSpannerForeignKey(srcId string, used map[string]bool) string {
+func ToSpannerForeignKey(conv *Conv, srcId string) string {
 	if srcId == "" {
 		return ""
 	}
-	return getSpannerId(srcId, used)
+	return getSpannerId(conv, srcId)
 }
 
 // ToSpannerIndexName maps source index name to legal Spanner index name.
@@ -160,28 +151,32 @@ func ToSpannerForeignKey(srcId string, used map[string]bool) string {
 // (across the database). But in some source databases, such as MySQL,
 // they only have to be unique for a table. Hence we must map each source
 // constraint name to a unique spanner constraint name.
-func ToSpannerIndexName(srcId string, used map[string]bool) string {
-	return getSpannerId(srcId, used)
+func ToSpannerIndexName(conv *Conv, srcId string) string {
+	return getSpannerId(conv, srcId)
 }
 
-func getSpannerId(srcId string, used map[string]bool) string {
+// conv.UsedNames tracks Spanner names that have been used for table names, foreign key constraints
+// and indexes. We use this to ensure we generate unique names when
+// we map from source dbs to Spanner since Spanner requires all these names to be
+// distinct and should not differ only in case.
+func getSpannerId(conv *Conv, srcId string) string {
 	spKeyName, _ := FixName(srcId)
-	if _, found := used[spKeyName]; found {
+	if _, found := conv.UsedNames[strings.ToLower(spKeyName)]; found {
 		// spKeyName has been used before.
 		// Add unique postfix: use number of keys so far.
 		// However, there is a chance this has already been used,
 		// so need to iterate.
-		id := len(used)
+		id := len(conv.UsedNames)
 		for {
 			c := spKeyName + "_" + strconv.Itoa(id)
-			if _, found := used[c]; !found {
+			if _, found := conv.UsedNames[strings.ToLower(c)]; !found {
 				spKeyName = c
 				break
 			}
 			id++
 		}
 	}
-	used[spKeyName] = true
+	conv.UsedNames[strings.ToLower(spKeyName)] = true
 	return spKeyName
 }
 
diff --git a/mysql/toddl.go b/mysql/toddl.go
index 2480bc9..bcccfe3 100644
--- a/mysql/toddl.go
+++ b/mysql/toddl.go
@@ -32,23 +32,6 @@ import (
 // Spanner. It uses the source schema in conv.SrcSchema, and writes
 // the Spanner schema to conv.SpSchema.
 func schemaToDDL(conv *internal.Conv) error {
-	// Tracks Spanner names that have been used for foreign key constraints
-	// and indexes. We use this to ensure we generate unique names when
-	// we map from MySQL to Spanner since Spanner requires all foreign
-	// key and index names to be distinct (you can't use the same name
-	// for a foreign key constraint and an index).
-	usedNames := make(map[string]bool)
-	// As Spanner uses same namespace for table names, foreign key constraint
-	// names and index names, we need to pre-populate usedNames with Spanner table
-	// names to handle collision with foreign key names and index names.
-	for _, srcTable := range conv.SrcSchema {
-		spTableName, err := internal.GetSpannerTable(conv, srcTable.Name)
-		if err != nil {
-			conv.Unexpected(fmt.Sprintf("Couldn't map source table %s to Spanner: %s", srcTable.Name, err))
-			continue
-		}
-		usedNames[spTableName] = true
-	}
 	for _, srcTable := range conv.SrcSchema {
 		spTableName, err := internal.GetSpannerTable(conv, srcTable.Name)
 		if err != nil {
@@ -99,8 +82,8 @@ func schemaToDDL(conv *internal.Conv) error {
 			ColNames: spColNames,
 			ColDefs:  spColDef,
 			Pks:      cvtPrimaryKeys(conv, srcTable.Name, srcTable.PrimaryKeys),
-			Fks:      cvtForeignKeys(conv, srcTable.Name, srcTable.ForeignKeys, usedNames),
-			Indexes:  cvtIndexes(conv, spTableName, srcTable.Name, srcTable.Indexes, usedNames),
+			Fks:      cvtForeignKeys(conv, srcTable.Name, srcTable.ForeignKeys),
+			Indexes:  cvtIndexes(conv, spTableName, srcTable.Name, srcTable.Indexes),
 			Comment:  comment}
 	}
 	internal.ResolveRefs(conv)
@@ -190,7 +173,7 @@ func cvtPrimaryKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Key)
 	return spKeys
 }
 
-func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.ForeignKey, usedNames map[string]bool) []ddl.Foreignkey {
+func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.ForeignKey) []ddl.Foreignkey {
 	var spKeys []ddl.Foreignkey
 	for _, key := range srcKeys {
 		if len(key.Columns) != len(key.ReferColumns) {
@@ -213,7 +196,7 @@ func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Forei
 			spCols = append(spCols, spCol)
 			spReferCols = append(spReferCols, spReferCol)
 		}
-		spKeyName := internal.ToSpannerForeignKey(key.Name, usedNames)
+		spKeyName := internal.ToSpannerForeignKey(conv, key.Name)
 		spKey := ddl.Foreignkey{
 			Name:         spKeyName,
 			Columns:      spCols,
@@ -224,7 +207,7 @@ func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Forei
 	return spKeys
 }
 
-func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcIndexes []schema.Index, usedNames map[string]bool) []ddl.CreateIndex {
+func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcIndexes []schema.Index) []ddl.CreateIndex {
 	var spIndexes []ddl.CreateIndex
 	for _, srcIndex := range srcIndexes {
 		var spKeys []ddl.IndexKey
@@ -241,7 +224,7 @@ func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcInd
 			// Collision of index name will be handled by ToSpannerIndexName.
 			srcIndex.Name = fmt.Sprintf("Index_%s", srcTable)
 		}
-		spIndexName := internal.ToSpannerIndexName(srcIndex.Name, usedNames)
+		spIndexName := internal.ToSpannerIndexName(conv, srcIndex.Name)
 		spIndex := ddl.CreateIndex{
 			Name:   spIndexName,
 			Table:  spTableName,
diff --git a/postgres/toddl.go b/postgres/toddl.go
index 4dc63c6..c32d39d 100644
--- a/postgres/toddl.go
+++ b/postgres/toddl.go
@@ -28,23 +28,6 @@ import (
 // Spanner. It uses the source schema in conv.SrcSchema, and writes
 // the Spanner schema to conv.SpSchema.
 func schemaToDDL(conv *internal.Conv) error {
-	// Tracks Spanner names that have been used for foreign key constraints
-	// and indexes. We use this to ensure we generate unique names when
-	// we map from Postgres to Spanner since Spanner requires all foreign
-	// key and index names to be distinct (you can't use the same name
-	// for a foreign key constraint and an index).
-	usedNames := make(map[string]bool)
-	// As Spanner uses same namespace for table names, foreign key constraint
-	// names and index names, we need to pre-populate usedNames with Spanner table
-	// names to handle collision with foreign key names and index names.
-	for _, srcTable := range conv.SrcSchema {
-		spTableName, err := internal.GetSpannerTable(conv, srcTable.Name)
-		if err != nil {
-			conv.Unexpected(fmt.Sprintf("Couldn't map source table %s to Spanner: %s", srcTable.Name, err))
-			continue
-		}
-		usedNames[spTableName] = true
-	}
 	for _, srcTable := range conv.SrcSchema {
 		spTableName, err := internal.GetSpannerTable(conv, srcTable.Name)
 		if err != nil {
@@ -98,8 +81,8 @@ func schemaToDDL(conv *internal.Conv) error {
 			ColNames: spColNames,
 			ColDefs:  spColDef,
 			Pks:      cvtPrimaryKeys(conv, srcTable.Name, srcTable.PrimaryKeys),
-			Fks:      cvtForeignKeys(conv, srcTable.Name, srcTable.ForeignKeys, usedNames),
-			Indexes:  cvtIndexes(conv, spTableName, srcTable.Name, srcTable.Indexes, usedNames),
+			Fks:      cvtForeignKeys(conv, srcTable.Name, srcTable.ForeignKeys),
+			Indexes:  cvtIndexes(conv, spTableName, srcTable.Name, srcTable.Indexes),
 			Comment:  comment}
 	}
 	internal.ResolveRefs(conv)
@@ -203,7 +186,7 @@ func cvtPrimaryKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Key)
 	return spKeys
 }
 
-func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.ForeignKey, usedNames map[string]bool) []ddl.Foreignkey {
+func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.ForeignKey) []ddl.Foreignkey {
 	var spKeys []ddl.Foreignkey
 	for _, key := range srcKeys {
 		if len(key.Columns) != len(key.ReferColumns) {
@@ -226,7 +209,7 @@ func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Forei
 			spCols = append(spCols, spCol)
 			spReferCols = append(spReferCols, spReferCol)
 		}
-		spKeyName := internal.ToSpannerForeignKey(key.Name, usedNames)
+		spKeyName := internal.ToSpannerForeignKey(conv, key.Name)
 		spKey := ddl.Foreignkey{
 			Name:         spKeyName,
 			Columns:      spCols,
@@ -237,7 +220,7 @@ func cvtForeignKeys(conv *internal.Conv, srcTable string, srcKeys []schema.Forei
 	return spKeys
 }
 
-func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcIndexes []schema.Index, usedNames map[string]bool) []ddl.CreateIndex {
+func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcIndexes []schema.Index) []ddl.CreateIndex {
 	var spIndexes []ddl.CreateIndex
 	for _, srcIndex := range srcIndexes {
 		var spKeys []ddl.IndexKey
@@ -254,7 +237,7 @@ func cvtIndexes(conv *internal.Conv, spTableName string, srcTable string, srcInd
 			// Collision of index name will be handled by ToSpannerIndexName.
 			srcIndex.Name = fmt.Sprintf("Index_%s", srcTable)
 		}
-		spIndexName := internal.ToSpannerIndexName(srcIndex.Name, usedNames)
+		spIndexName := internal.ToSpannerIndexName(conv, srcIndex.Name)
 		spIndex := ddl.CreateIndex{
 			Name:   spIndexName,
 			Table:  spTableName,


Test patch:
diff --git a/internal/mapping_test.go b/internal/mapping_test.go
index b242673..c739261 100644
--- a/internal/mapping_test.go
+++ b/internal/mapping_test.go
@@ -40,6 +40,8 @@ func TestGetSpannerTable(t *testing.T) {
 		{"Illegal start character", "2table", false, "Atable"},
 		{"Illegal start character with collision (1)", "_table", false, "Atable_8"},
 		{"Illegal start character with collision (2)", "\ntable", false, "Atable_9"},
+		{"Name differing only in case", "TABLE", false, "TABLE_10"},
+		{"Illegal differing only in case", "TAB\nLE_5", false, "TAB_LE_5_11"},
 	}
 	for _, tc := range basicTests {
 		spTable, err := GetSpannerTable(conv, tc.srcTable)
@@ -94,8 +96,7 @@ func TestGetSpannerCol(t *testing.T) {
 }
 
 func TestToSpannerForeignKey(t *testing.T) {
-	schemaForeignKeys := make(map[string]bool)
-
+	conv := MakeConv()
 	basicTests := []struct {
 		name       string // Name of test.
 		srcKeyName string // Source foreign key name.
@@ -105,14 +106,13 @@ func TestToSpannerForeignKey(t *testing.T) {
 		{"Empty name", "", ""},
 	}
 	for _, tc := range basicTests {
-		spKeyName := ToSpannerForeignKey(tc.srcKeyName, schemaForeignKeys)
+		spKeyName := ToSpannerForeignKey(conv, tc.srcKeyName)
 		assert.Equal(t, tc.spKeyName, spKeyName, tc.name)
 	}
 }
 
 func TestGetSpannerId(t *testing.T) {
-	schemaIndexKeys := make(map[string]bool)
-
+	conv := MakeConv()
 	basicTests := []struct {
 		name       string // Name of test.
 		srcKeyName string // Source key name.
@@ -130,9 +130,11 @@ func TestGetSpannerId(t *testing.T) {
 		{"Bad name with collision", "in\tdex", "in_dex_9"},
 		{"Bad name with collision 2", "in\ndex", "in_dex_10"},
 		{"Bad name with collision 3", "in?dex", "in_dex_11"},
+		{"Name with different case collision", "INdex1", "INdex1_12"},
+		{"Bad name with different case collision", "IN\tDex", "IN_Dex_13"},
 	}
 	for _, tc := range basicTests {
-		spKeyName := getSpannerId(tc.srcKeyName, schemaIndexKeys)
+		spKeyName := getSpannerId(conv, tc.srcKeyName)
 		assert.Equal(t, tc.spKeyName, spKeyName, tc.name)
 	}
 }
diff --git a/test_data/mysqldump.test.out b/test_data/mysqldump.test.out
index dccebe1..bc5af68 100644
--- a/test_data/mysqldump.test.out
+++ b/test_data/mysqldump.test.out
@@ -31,6 +31,8 @@ CREATE TABLE `cart` (
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 /*!40101 SET character_set_client = @saved_cs_client */;
 
+CREATE INDEX idx ON `cart` (`quantity`);
+
 --
 -- Dumping data for table `cart`
 --
@@ -41,6 +43,20 @@ INSERT INTO `cart` VALUES ('901e-a6cfc2b502dc','abc-123',1,'2020-07-20 05:10:26'
 /*!40000 ALTER TABLE `cart` ENABLE KEYS */;
 UNLOCK TABLES;
 
+--
+-- Table name `CART` differs only case from the table `cart`.
+-- This was added to cover more cases in our integration tests.
+--
+DROP TABLE IF EXISTS `CART`;
+/*!40101 SET @saved_cs_client     = @@character_set_client */;
+/*!40101 SET character_set_client = utf8 */;
+CREATE TABLE `CART` (
+  `usr_id` varchar(20) NOT NULL,
+  `prod_id` varchar(20) NOT NULL,
+  PRIMARY KEY (`usr_id`,`prod_id`)
+) ENGINE=InnoDB DEFAULT CHARSET=utf8;
+/*!40101 SET character_set_client = @saved_cs_client */;
+
 --
 -- Table structure for table `products`
 --
@@ -57,6 +73,12 @@ CREATE TABLE `products` (
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 /*!40101 SET character_set_client = @saved_cs_client */;
 
+--
+-- The index name `IDX` differs only in case from index idx on `cart`.
+-- This was added to cover more cases in our integration tests.
+--
+CREATE INDEX IDX ON `products` (`description`);
+
 --
 -- Dumping data for table `products`
 --