Repository: go-gorm/gorm
Pull request #6602 (2023-09-17 06:34:39)

Don't call MethodByName with a variable arg
Go 1.22 goes somewhat toward addressing the issue using reflect MethodByName disabling linker deadcode elimination (DCE) and the resultant large increase in binary size because the linker cannot prune unused code because it might be reached via reflection.

Go Issue https://github.com/golang/go/issues/62257 reduces the number of incidences of this problem by leveraging a compiler assist to avoid marking functions containing calls to MethodByName as ReflectMethods as long as the arguments are constants. 

An analysis of Uber Technologies code base however shows that a number of transitive imports still contain calls to MethodByName with a variable argument, including GORM.

In the case of GORM, the solution we are proposing is because the number of possible methods is finite, we will "unroll" this. This demonstrably shows that GORM is not longer a problem for DCE.


Before
```
    % go version
    go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64
    % go  test ./... -ldflags=-dumpdep   2>  >(grep -i -e  '->.*<reflectmethod>')
    gorm.io/gorm.(*Statement).BuildCondition -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod>
    type:reflect.Value <UsedInIface> -> reflect.(*Value).Method <ReflectMethod>
    type:reflect.Value <UsedInIface> -> reflect.(*Value).MethodByName <ReflectMethod>
    ok      gorm.io/gorm    (cached)
    ok      gorm.io/gorm/callbacks  (cached)
    gorm.io/gorm/clause_test.BenchmarkComplexSelect -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod>
    type:reflect.Value <UsedInIface> -> reflect.(*Value).Method <ReflectMethod>
    type:reflect.Value <UsedInIface> -> reflect.(*Value).MethodByName <ReflectMethod>
    ?       gorm.io/gorm/migrator   [no test files]
    ok      gorm.io/gorm/clause     (cached)
    ok      gorm.io/gorm/logger     (cached)
    gorm.io/gorm/schema_test.TestAdvancedDataTypeValuerAndSetter -> gorm.io/gorm/schema.ParseWithSpecialTableName <ReflectMethod>
    type:reflect.Value <UsedInIface> -> reflect.(*Value).Method <ReflectMethod>
    type:reflect.Value <UsedInIface> -> reflect.(*Value).MethodByName <ReflectMethod>
    ?       gorm.io/gorm/utils/tests        [no test files]
    ok      gorm.io/gorm/schema     (cached)
    ok      gorm.io/gorm/utils      (cached)
```
    
After
    
```
    %go version
    go version devel go1.22-2f3458a8ce Sat Sep 16 16:26:48 2023 -0700 darwin/arm64
    %go  test ./... -ldflags=-dumpdep   2>  >(grep -i -e  '->.*<reflectmethod>')
    ok      gorm.io/gorm    (cached)
    ok      gorm.io/gorm/callbacks  (cached)
    ?       gorm.io/gorm/migrator   [no test files]
    ?       gorm.io/gorm/utils/tests        [no test files]
    ok      gorm.io/gorm/clause     (cached)
    ok      gorm.io/gorm/logger     (cached)
    ok      gorm.io/gorm/schema     (cached)
    ok      gorm.io/gorm/utils      (cached)
```

- [ X ] Do only one thing
- [ X ] Non breaking API changes
- [ X ] Tested

### What did this pull request do?

It aims to stop any large Go project that imports gorm from having the link-level dead code elimination (DCE) turned off. This can result in the final binary being floated by 100+ MB.

While Go doesn't have a long term solution for this problem yet, the work done by Kubernetes can be leveraged in GORM to ensure GORM is not one of the packages that disable (DCE).

### User Case Description

Users who use GORM in very large binaries, such as the ones used here at Uber (100s of MB) may notice a drop in binary size, provided no single package breaks the DCE.


Patch:
diff --git a/clause/expression.go b/clause/expression.go
index 8d01052..3140846 100644
--- a/clause/expression.go
+++ b/clause/expression.go
@@ -126,7 +126,7 @@ func (expr NamedExpr) Build(builder Builder) {
 	for _, v := range []byte(expr.SQL) {
 		if v == '@' && !inName {
 			inName = true
-			name = []byte{}
+			name = name[:0]
 		} else if v == ' ' || v == ',' || v == ')' || v == '"' || v == '\'' || v == '`' || v == '\r' || v == '\n' || v == ';' {
 			if inName {
 				if nv, ok := namedMap[string(name)]; ok {
diff --git a/migrator/migrator.go b/migrator/migrator.go
index b15a43e..49bc937 100644
--- a/migrator/migrator.go
+++ b/migrator/migrator.go
@@ -217,7 +217,7 @@ func (m Migrator) CreateTable(values ...interface{}) error {
 				field := stmt.Schema.FieldsByDBName[dbName]
 				if !field.IgnoreMigration {
 					createTableSQL += "? ?"
-					hasPrimaryKeyInDataType = hasPrimaryKeyInDataType || strings.Contains(strings.ToUpper(string(field.DataType)), "PRIMARY KEY")
+					hasPrimaryKeyInDataType = hasPrimaryKeyInDataType || strings.Contains(strings.ToUpper(m.DataTypeOf(field)), "PRIMARY KEY")
 					values = append(values, clause.Column{Name: dbName}, m.DB.Migrator().FullDataTypeOf(field))
 					createTableSQL += ","
 				}
diff --git a/schema/schema.go b/schema/schema.go
index e13a5ed..3e7459c 100644
--- a/schema/schema.go
+++ b/schema/schema.go
@@ -13,6 +13,20 @@ import (
 	"gorm.io/gorm/logger"
 )
 
+type callbackType string
+
+const (
+	callbackTypeBeforeCreate callbackType = "BeforeCreate"
+	callbackTypeBeforeUpdate callbackType = "BeforeUpdate"
+	callbackTypeAfterCreate  callbackType = "AfterCreate"
+	callbackTypeAfterUpdate  callbackType = "AfterUpdate"
+	callbackTypeBeforeSave   callbackType = "BeforeSave"
+	callbackTypeAfterSave    callbackType = "AfterSave"
+	callbackTypeBeforeDelete callbackType = "BeforeDelete"
+	callbackTypeAfterDelete  callbackType = "AfterDelete"
+	callbackTypeAfterFind    callbackType = "AfterFind"
+)
+
 // ErrUnsupportedDataType unsupported data type
 var ErrUnsupportedDataType = errors.New("unsupported data type")
 
@@ -288,14 +302,20 @@ func ParseWithSpecialTableName(dest interface{}, cacheStore *sync.Map, namer Nam
 		}
 	}
 
-	callbacks := []string{"BeforeCreate", "AfterCreate", "BeforeUpdate", "AfterUpdate", "BeforeSave", "AfterSave", "BeforeDelete", "AfterDelete", "AfterFind"}
-	for _, name := range callbacks {
-		if methodValue := modelValue.MethodByName(name); methodValue.IsValid() {
+	callbackTypes := []callbackType{
+		callbackTypeBeforeCreate, callbackTypeAfterCreate,
+		callbackTypeBeforeUpdate, callbackTypeAfterUpdate,
+		callbackTypeBeforeSave, callbackTypeAfterSave,
+		callbackTypeBeforeDelete, callbackTypeAfterDelete,
+		callbackTypeAfterFind,
+	}
+	for _, cbName := range callbackTypes {
+		if methodValue := callBackToMethodValue(modelValue, cbName); methodValue.IsValid() {
 			switch methodValue.Type().String() {
 			case "func(*gorm.DB) error": // TODO hack
-				reflect.Indirect(reflect.ValueOf(schema)).FieldByName(name).SetBool(true)
+				reflect.Indirect(reflect.ValueOf(schema)).FieldByName(string(cbName)).SetBool(true)
 			default:
-				logger.Default.Warn(context.Background(), "Model %v don't match %vInterface, should be `%v(*gorm.DB) error`. Please see https://gorm.io/docs/hooks.html", schema, name, name)
+				logger.Default.Warn(context.Background(), "Model %v don't match %vInterface, should be `%v(*gorm.DB) error`. Please see https://gorm.io/docs/hooks.html", schema, cbName, cbName)
 			}
 		}
 	}
@@ -349,6 +369,39 @@ func ParseWithSpecialTableName(dest interface{}, cacheStore *sync.Map, namer Nam
 	return schema, schema.err
 }
 
+// This unrolling is needed to show to the compiler the exact set of methods
+// that can be used on the modelType.
+// Prior to go1.22 any use of MethodByName would cause the linker to
+// abandon dead code elimination for the entire binary.
+// As of go1.22 the compiler supports one special case of a string constant
+// being passed to MethodByName. For enterprise customers or those building
+// large binaries, this gives a significant reduction in binary size.
+// https://github.com/golang/go/issues/62257
+func callBackToMethodValue(modelType reflect.Value, cbType callbackType) reflect.Value {
+	switch cbType {
+	case callbackTypeBeforeCreate:
+		return modelType.MethodByName(string(callbackTypeBeforeCreate))
+	case callbackTypeAfterCreate:
+		return modelType.MethodByName(string(callbackTypeAfterCreate))
+	case callbackTypeBeforeUpdate:
+		return modelType.MethodByName(string(callbackTypeBeforeUpdate))
+	case callbackTypeAfterUpdate:
+		return modelType.MethodByName(string(callbackTypeAfterUpdate))
+	case callbackTypeBeforeSave:
+		return modelType.MethodByName(string(callbackTypeBeforeSave))
+	case callbackTypeAfterSave:
+		return modelType.MethodByName(string(callbackTypeAfterSave))
+	case callbackTypeBeforeDelete:
+		return modelType.MethodByName(string(callbackTypeBeforeDelete))
+	case callbackTypeAfterDelete:
+		return modelType.MethodByName(string(callbackTypeAfterDelete))
+	case callbackTypeAfterFind:
+		return modelType.MethodByName(string(callbackTypeAfterFind))
+	default:
+		return reflect.ValueOf(nil)
+	}
+}
+
 func getOrParse(dest interface{}, cacheStore *sync.Map, namer Namer) (*Schema, error) {
 	modelType := reflect.ValueOf(dest).Type()
 	for modelType.Kind() == reflect.Slice || modelType.Kind() == reflect.Array || modelType.Kind() == reflect.Ptr {
diff --git a/utils/utils.go b/utils/utils.go
index ddbca60..c8fec5b 100644
--- a/utils/utils.go
+++ b/utils/utils.go
@@ -89,19 +89,28 @@ func Contains(elems []string, elem string) bool {
 	return false
 }
 
-func AssertEqual(src, dst interface{}) bool {
-	if !reflect.DeepEqual(src, dst) {
-		if valuer, ok := src.(driver.Valuer); ok {
-			src, _ = valuer.Value()
-		}
+func AssertEqual(x, y interface{}) bool {
+	if reflect.DeepEqual(x, y) {
+		return true
+	}
+	if x == nil || y == nil {
+		return false
+	}
 
-		if valuer, ok := dst.(driver.Valuer); ok {
-			dst, _ = valuer.Value()
-		}
+	xval := reflect.ValueOf(x)
+	yval := reflect.ValueOf(y)
+	if xval.Kind() == reflect.Ptr && xval.IsNil() ||
+		yval.Kind() == reflect.Ptr && yval.IsNil() {
+		return false
+	}
 
-		return reflect.DeepEqual(src, dst)
+	if valuer, ok := x.(driver.Valuer); ok {
+		x, _ = valuer.Value()
+	}
+	if valuer, ok := y.(driver.Valuer); ok {
+		y, _ = valuer.Value()
 	}
-	return true
+	return reflect.DeepEqual(x, y)
 }
 
 func ToString(value interface{}) string {


Test patch:
diff --git a/tests/go.mod b/tests/go.mod
index aef26e3..5a0aedd 100644
--- a/tests/go.mod
+++ b/tests/go.mod
@@ -8,7 +8,7 @@ require (
 	github.com/lib/pq v1.10.9
 	gorm.io/driver/mysql v1.5.2-0.20230612053416-48b6526a21f0
 	gorm.io/driver/postgres v1.5.3-0.20230607070428-18bc84b75196
-	gorm.io/driver/sqlite v1.5.3
+	gorm.io/driver/sqlite v1.5.4
 	gorm.io/driver/sqlserver v1.5.2-0.20230613072041-6e2cde390b0a
 	gorm.io/gorm v1.25.4
 )
@@ -22,9 +22,9 @@ require (
 	github.com/jackc/pgx/v5 v5.4.3 // indirect
 	github.com/jinzhu/inflection v1.0.0 // indirect
 	github.com/mattn/go-sqlite3 v1.14.17 // indirect
-	github.com/microsoft/go-mssqldb v1.5.0 // indirect
-	golang.org/x/crypto v0.12.0 // indirect
-	golang.org/x/text v0.12.0 // indirect
+	github.com/microsoft/go-mssqldb v1.6.0 // indirect
+	golang.org/x/crypto v0.14.0 // indirect
+	golang.org/x/text v0.13.0 // indirect
 )
 
 replace gorm.io/gorm => ../
diff --git a/tests/migrate_test.go b/tests/migrate_test.go
index 849e2b7..cfd3e0a 100644
--- a/tests/migrate_test.go
+++ b/tests/migrate_test.go
@@ -862,6 +862,48 @@ func TestMigrateWithSpecialName(t *testing.T) {
 	AssertEqual(t, true, DB.Migrator().HasTable("coupon_product_2"))
 }
 
+// https://github.com/go-gorm/gorm/issues/4760
+func TestMigrateAutoIncrement(t *testing.T) {
+	type AutoIncrementStruct struct {
+		ID     int64   `gorm:"primarykey;autoIncrement"`
+		Field1 uint32  `gorm:"column:field1"`
+		Field2 float32 `gorm:"column:field2"`
+	}
+
+	if err := DB.AutoMigrate(&AutoIncrementStruct{}); err != nil {
+		t.Fatalf("AutoMigrate err: %v", err)
+	}
+
+	const ROWS = 10
+	for idx := 0; idx < ROWS; idx++ {
+		if err := DB.Create(&AutoIncrementStruct{}).Error; err != nil {
+			t.Fatalf("create auto_increment_struct fail, err: %v", err)
+		}
+	}
+
+	rows := make([]*AutoIncrementStruct, 0, ROWS)
+	if err := DB.Order("id ASC").Find(&rows).Error; err != nil {
+		t.Fatalf("find auto_increment_struct fail, err: %v", err)
+	}
+
+	ids := make([]int64, 0, len(rows))
+	for _, row := range rows {
+		ids = append(ids, row.ID)
+	}
+	lastID := ids[len(ids)-1]
+
+	if err := DB.Where("id IN (?)", ids).Delete(&AutoIncrementStruct{}).Error; err != nil {
+		t.Fatalf("delete auto_increment_struct fail, err: %v", err)
+	}
+
+	newRow := &AutoIncrementStruct{}
+	if err := DB.Create(newRow).Error; err != nil {
+		t.Fatalf("create auto_increment_struct fail, err: %v", err)
+	}
+
+	AssertEqual(t, newRow.ID, lastID+1)
+}
+
 // https://github.com/go-gorm/gorm/issues/5320
 func TestPrimarykeyID(t *testing.T) {
 	if DB.Dialector.Name() != "postgres" {
diff --git a/utils/utils_test.go b/utils/utils_test.go
index 71eef96..d048682 100644
--- a/utils/utils_test.go
+++ b/utils/utils_test.go
@@ -98,6 +98,7 @@ func TestAssertEqual(t *testing.T) {
 		{"error not equal", errors.New("1"), errors.New("2"), false},
 		{"driver.Valuer equal", ModifyAt{Time: now, Valid: true}, ModifyAt{Time: now, Valid: true}, true},
 		{"driver.Valuer not equal", ModifyAt{Time: now, Valid: true}, ModifyAt{Time: now.Add(time.Second), Valid: true}, false},
+		{"driver.Valuer equal (ptr to nil ptr)", (*ModifyAt)(nil), &ModifyAt{}, false},
 	}
 	for _, test := range assertEqualTests {
 		t.Run(test.name, func(t *testing.T) {