Repository: go-gorm/gorm
Pull request #7208 (2024-09-27 00:28:49)

fix: deterministic index ordering when migrating
<!--
Make sure these boxes checked before submitting your pull request.

For significant changes, please open an issue to make an agreement on an implementation design/plan first before starting it.
-->

- [x] Do only one thing
- [x] Non breaking API changes
- [x] Tested

### What did this pull request do?

Change ParseIndexes to return a list of indices rather than a map, so the callers will iterate in deterministic order.  This means when re-creating a database from the same schema, indexes will be created in a consistent order.

### User Case Description

Issue: We observed that, when creating a database based on the same gORM schema multiple times, indexes could appear in different orders, hurting determinism for use-cases like schema comparison.

bamo: If the maintainers are happy with this, I think it makes sense as well to do this for ParseUniqueConstraints and ParseCheckConstraints as well.

I'm happy to do those as follow-on PRs as well. Just let me know.
bamo: The test failure seems like a flake or something (container failed to start).  Looks like I don't have permissions to re-run it.
bamo: @jinzhu What do you think about this?
aashishkapur: I have the same issue - would be good to merge this fix.
jinzhu: Hi @bamo 

Can you rebase the master branch, it might fix the tests.


bamo: @jinzhu done!
bamo: Hey @jinzhu, I don't mean to bother you here, just wondering if there's anything else you'd like me to do for this or if you think we can land this soon.  Thanks!

Patch:
diff --git a/schema/index.go b/schema/index.go
index a90153f..786e210 100644
--- a/schema/index.go
+++ b/schema/index.go
@@ -27,8 +27,9 @@ type IndexOption struct {
 }
 
 // ParseIndexes parse schema indexes
-func (schema *Schema) ParseIndexes() map[string]Index {
-	indexes := map[string]Index{}
+func (schema *Schema) ParseIndexes() []*Index {
+	indexesByName := map[string]*Index{}
+	indexes := []*Index{}
 
 	for _, field := range schema.Fields {
 		if field.TagSettings["INDEX"] != "" || field.TagSettings["UNIQUEINDEX"] != "" {
@@ -38,7 +39,12 @@ func (schema *Schema) ParseIndexes() map[string]Index {
 				break
 			}
 			for _, index := range fieldIndexes {
-				idx := indexes[index.Name]
+				idx := indexesByName[index.Name]
+				if idx == nil {
+					idx = &Index{Name: index.Name}
+					indexesByName[index.Name] = idx
+					indexes = append(indexes, idx)
+				}
 				idx.Name = index.Name
 				if idx.Class == "" {
 					idx.Class = index.Class
@@ -60,8 +66,6 @@ func (schema *Schema) ParseIndexes() map[string]Index {
 				sort.Slice(idx.Fields, func(i, j int) bool {
 					return idx.Fields[i].priority < idx.Fields[j].priority
 				})
-
-				indexes[index.Name] = idx
 			}
 		}
 	}
@@ -76,15 +80,14 @@ func (schema *Schema) ParseIndexes() map[string]Index {
 func (schema *Schema) LookIndex(name string) *Index {
 	if schema != nil {
 		indexes := schema.ParseIndexes()
-
-		if index, found := indexes[name]; found {
-			return &index
-		}
-
 		for _, index := range indexes {
+			if index.Name == name {
+				return index
+			}
+
 			for _, field := range index.Fields {
 				if field.Name == name {
-					return &index
+					return index
 				}
 			}
 		}


Test patch:
diff --git a/schema/index_test.go b/schema/index_test.go
index 9971562..5c970d1 100644
--- a/schema/index_test.go
+++ b/schema/index_test.go
@@ -61,17 +61,17 @@ func TestParseIndex(t *testing.T) {
 		t.Fatalf("failed to parse user index, got error %v", err)
 	}
 
-	results := map[string]schema.Index{
-		"idx_user_indices_name": {
+	results := []*schema.Index{
+		{
 			Name:   "idx_user_indices_name",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "Name"}}},
 		},
-		"idx_name": {
+		{
 			Name:   "idx_name",
 			Class:  "UNIQUE",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "Name2", UniqueIndex: "idx_name"}}},
 		},
-		"idx_user_indices_name3": {
+		{
 			Name:  "idx_user_indices_name3",
 			Type:  "btree",
 			Where: "name3 != 'jinzhu'",
@@ -82,19 +82,19 @@ func TestParseIndex(t *testing.T) {
 				Length:  10,
 			}},
 		},
-		"idx_user_indices_name4": {
+		{
 			Name:   "idx_user_indices_name4",
 			Class:  "UNIQUE",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "Name4", UniqueIndex: "idx_user_indices_name4"}}},
 		},
-		"idx_user_indices_name5": {
+		{
 			Name:    "idx_user_indices_name5",
 			Class:   "FULLTEXT",
 			Comment: "hello , world",
 			Where:   "age > 10",
 			Fields:  []schema.IndexOption{{Field: &schema.Field{Name: "Name5"}}},
 		},
-		"profile": {
+		{
 			Name:    "profile",
 			Comment: "hello , world",
 			Where:   "age > 10",
@@ -104,21 +104,21 @@ func TestParseIndex(t *testing.T) {
 				Expression: "ABS(age)",
 			}},
 		},
-		"idx_id": {
+		{
 			Name:   "idx_id",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "MemberNumber"}}, {Field: &schema.Field{Name: "OID", UniqueIndex: "idx_oid"}}},
 		},
-		"idx_oid": {
+		{
 			Name:   "idx_oid",
 			Class:  "UNIQUE",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "OID", UniqueIndex: "idx_oid"}}},
 		},
-		"type": {
+		{
 			Name:   "type",
 			Type:   "",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "Name7"}}},
 		},
-		"idx_user_indices_name8": {
+		{
 			Name: "idx_user_indices_name8",
 			Type: "",
 			Fields: []schema.IndexOption{
@@ -127,7 +127,16 @@ func TestParseIndex(t *testing.T) {
 				{Field: &schema.Field{Name: "Name8"}, Collate: "utf8"},
 			},
 		},
-		"idx_user_indices_comp_id0": {
+		{
+			Class:  "UNIQUE",
+			Name:   "idx_user_indices_idx_compname_1",
+			Option: "NULLS NOT DISTINCT",
+			Fields: []schema.IndexOption{
+				{Field: &schema.Field{Name: "CompName1", NotNull: true}},
+				{Field: &schema.Field{Name: "CompName2"}},
+			},
+		},
+		{
 			Name: "idx_user_indices_comp_id0",
 			Type: "",
 			Fields: []schema.IndexOption{{
@@ -136,7 +145,7 @@ func TestParseIndex(t *testing.T) {
 				Field: &schema.Field{Name: "Data0B"},
 			}},
 		},
-		"idx_user_indices_comp_id1": {
+		{
 			Name: "idx_user_indices_comp_id1",
 			Fields: []schema.IndexOption{{
 				Field: &schema.Field{Name: "Data1A"},
@@ -146,7 +155,7 @@ func TestParseIndex(t *testing.T) {
 				Field: &schema.Field{Name: "Data1C"},
 			}},
 		},
-		"idx_user_indices_comp_id2": {
+		{
 			Name:  "idx_user_indices_comp_id2",
 			Class: "UNIQUE",
 			Fields: []schema.IndexOption{{
@@ -157,15 +166,6 @@ func TestParseIndex(t *testing.T) {
 				Field: &schema.Field{Name: "Data2B"},
 			}},
 		},
-		"idx_user_indices_idx_compname_1": {
-			Class:  "UNIQUE",
-			Name:   "idx_user_indices_idx_compname_1",
-			Option: "NULLS NOT DISTINCT",
-			Fields: []schema.IndexOption{
-				{Field: &schema.Field{Name: "CompName1", NotNull: true}},
-				{Field: &schema.Field{Name: "CompName2"}},
-			},
-		},
 	}
 
 	CheckIndices(t, results, user.ParseIndexes())
@@ -195,17 +195,17 @@ func TestParseIndexWithUniqueIndexAndUnique(t *testing.T) {
 		t.Fatalf("failed to parse user index, got error %v", err)
 	}
 	indices := indexSchema.ParseIndexes()
-	CheckIndices(t, map[string]schema.Index{
-		"idx_index_tests_field_a": {
+	expectedIndices := []*schema.Index{
+		{
 			Name:   "idx_index_tests_field_a",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "FieldA", Unique: true}}},
 		},
-		"idx_index_tests_field_c": {
+		{
 			Name:   "idx_index_tests_field_c",
 			Class:  "UNIQUE",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "FieldC", UniqueIndex: "idx_index_tests_field_c"}}},
 		},
-		"idx_index_tests_field_d": {
+		{
 			Name:  "idx_index_tests_field_d",
 			Class: "UNIQUE",
 			Fields: []schema.IndexOption{
@@ -214,7 +214,7 @@ func TestParseIndexWithUniqueIndexAndUnique(t *testing.T) {
 				{Field: &schema.Field{Name: "FieldD"}},
 			},
 		},
-		"uniq_field_e1_e2": {
+		{
 			Name:  "uniq_field_e1_e2",
 			Class: "UNIQUE",
 			Fields: []schema.IndexOption{
@@ -222,11 +222,7 @@ func TestParseIndexWithUniqueIndexAndUnique(t *testing.T) {
 				{Field: &schema.Field{Name: "FieldE2"}},
 			},
 		},
-		"idx_index_tests_field_f1": {
-			Name:   "idx_index_tests_field_f1",
-			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "FieldF1"}}},
-		},
-		"uniq_field_f1_f2": {
+		{
 			Name:  "uniq_field_f1_f2",
 			Class: "UNIQUE",
 			Fields: []schema.IndexOption{
@@ -234,12 +230,16 @@ func TestParseIndexWithUniqueIndexAndUnique(t *testing.T) {
 				{Field: &schema.Field{Name: "FieldF2"}},
 			},
 		},
-		"idx_index_tests_field_g": {
+		{
+			Name:   "idx_index_tests_field_f1",
+			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "FieldF1"}}},
+		},
+		{
 			Name:   "idx_index_tests_field_g",
 			Class:  "UNIQUE",
 			Fields: []schema.IndexOption{{Field: &schema.Field{Name: "FieldG", Unique: true, UniqueIndex: "idx_index_tests_field_g"}}},
 		},
-		"uniq_field_h1_h2": {
+		{
 			Name:  "uniq_field_h1_h2",
 			Class: "UNIQUE",
 			Fields: []schema.IndexOption{
@@ -247,20 +247,23 @@ func TestParseIndexWithUniqueIndexAndUnique(t *testing.T) {
 				{Field: &schema.Field{Name: "FieldH2"}},
 			},
 		},
-	}, indices)
+	}
+	CheckIndices(t, expectedIndices, indices)
 }
 
-func CheckIndices(t *testing.T, expected, actual map[string]schema.Index) {
-	for k, ei := range expected {
-		t.Run(k, func(t *testing.T) {
-			ai, ok := actual[k]
-			if !ok {
-				t.Errorf("expected index %q but actual missing", k)
-				return
-			}
+func CheckIndices(t *testing.T, expected, actual []*schema.Index) {
+	if len(expected) != len(actual) {
+		t.Errorf("expected %d indices, but got %d", len(expected), len(actual))
+		return
+	}
+
+	for i, ei := range expected {
+		t.Run(ei.Name, func(t *testing.T) {
+			ai := actual[i]
 			tests.AssertObjEqual(t, ai, ei, "Name", "Class", "Type", "Where", "Comment", "Option")
+
 			if len(ei.Fields) != len(ai.Fields) {
-				t.Errorf("expected index %q field length is %d but actual %d", k, len(ei.Fields), len(ai.Fields))
+				t.Errorf("expected index %q field length is %d but actual %d", ei.Name, len(ei.Fields), len(ai.Fields))
 				return
 			}
 			for i, ef := range ei.Fields {
@@ -268,9 +271,5 @@ func CheckIndices(t *testing.T, expected, actual map[string]schema.Index) {
 				tests.AssertObjEqual(t, af, ef, "Name", "Unique", "UniqueIndex", "Expression", "Sort", "Collate", "Length", "NotNull")
 			}
 		})
-		delete(actual, k)
-	}
-	for k := range actual {
-		t.Errorf("unexpected index %q", k)
 	}
 }