From e69ffaa9c44642e0deb7b105eadafd9305f02320 Mon Sep 17 00:00:00 2001 From: Richard Knop Date: Wed, 16 Dec 2015 00:18:36 +0800 Subject: [PATCH 1/7] Fixed a bug with invalid zero value in preload When using pointers in model structs, there is an edge case. When using preload to eagerly preload a many to many relationship and the main row is not found by a primary key, use of reflect.Indirect in preload.go results in a zero value and the subsequent call to FieldByName panics the program. --- preload.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/preload.go b/preload.go index 417a0d8c..f3f9dabc 100644 --- a/preload.go +++ b/preload.go @@ -10,8 +10,15 @@ import ( func getRealValue(value reflect.Value, columns []string) (results []interface{}) { for _, column := range columns { - if reflect.Indirect(value).FieldByName(column).IsValid() { - result := reflect.Indirect(value).FieldByName(column).Interface() + pointedValue := reflect.Indirect(value) + // If v is a nil pointer, Indirect returns a zero Value! + // Therefor we need to check for a zero value, + // as FieldByName could panic + if !pointedValue.IsValid() { + continue + } + if pointedValue.FieldByName(column).IsValid() { + result := pointedValue.FieldByName(column).Interface() if r, ok := result.(driver.Valuer); ok { result, _ = r.Value() } @@ -290,6 +297,12 @@ func (scope *Scope) handleManyToManyPreload(field *Field, conditions []interface } } } else { + // If v is a nil pointer, Indirect returns a zero Value! + // Therefor we need to check for a zero value, + // as FieldByName could panic + if !scope.IndirectValue().IsValid() { + return + } object := scope.IndirectValue() source := getRealValue(object, associationForeignStructFieldNames) field := object.FieldByName(field.Name) From 15169e635a23f7aa1a0e5033c30d7355771cc696 Mon Sep 17 00:00:00 2001 From: Jinzhu Date: Wed, 16 Dec 2015 10:23:29 +0800 Subject: [PATCH 2/7] Add two more nested many2many preload tests --- preload_test.go | 129 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 122 insertions(+), 7 deletions(-) diff --git a/preload_test.go b/preload_test.go index d526e324..e1b4efe2 100644 --- a/preload_test.go +++ b/preload_test.go @@ -1,6 +1,7 @@ package gorm_test import ( + "database/sql" "encoding/json" "os" "reflect" @@ -698,11 +699,11 @@ func TestManyToManyPreloadWithMultiPrimaryKeys(t *testing.T) { func TestManyToManyPreloadForPointer(t *testing.T) { type ( Level1 struct { - ID uint `gorm:"primary_key;"` + ID uint Value string } Level2 struct { - ID uint `gorm:"primary_key;"` + ID uint Value string Level1s []*Level1 `gorm:"many2many:levels;"` } @@ -776,20 +777,134 @@ func TestManyToManyPreloadForPointer(t *testing.T) { } } -func TestNilPointerSlice(t *testing.T) { +func TestNestedManyToManyPreload(t *testing.T) { type ( - Level3 struct { - ID uint `gorm:"primary_key;"` + Level1 struct { + ID uint Value string } Level2 struct { - ID uint `gorm:"primary_key;"` + ID uint + Value string + Level1s []*Level1 `gorm:"many2many:level1_level2;"` + } + Level3 struct { + ID uint + Value string + Level2s []Level2 `gorm:"many2many:level2_level3;"` + } + ) + + DB.DropTableIfExists(&Level1{}) + DB.DropTableIfExists(&Level2{}) + DB.DropTableIfExists(&Level3{}) + DB.DropTableIfExists("level1_level2") + DB.DropTableIfExists("level2_level3") + + if err := DB.AutoMigrate(&Level3{}, &Level2{}, &Level1{}).Error; err != nil { + panic(err) + } + + want := Level3{ + Value: "Level3", + Level2s: []Level2{ + { + Value: "Bob", + Level1s: []*Level1{ + {Value: "ru"}, + {Value: "en"}, + }, + }, { + Value: "Tom", + Level1s: []*Level1{ + {Value: "zh"}, + {Value: "de"}, + }, + }, + }, + } + + if err := DB.Save(&want).Error; err != nil { + panic(err) + } + + var got Level3 + if err := DB.Preload("Level2s").Preload("Level2s.Level1s").Find(&got, "value = ?", "Level3").Error; err != nil { + panic(err) + } + + if !reflect.DeepEqual(got, want) { + t.Errorf("got %s; want %s", toJSONString(got), toJSONString(want)) + } +} + +func TestNestedManyToManyPreload2(t *testing.T) { + type ( + Level1 struct { + ID uint + Value string + } + Level2 struct { + ID uint + Value string + Level1s []*Level1 `gorm:"many2many:level1_level2;"` + } + Level3 struct { + ID uint + Value string + Level2ID sql.NullInt64 + Level2 *Level2 + } + ) + + DB.DropTableIfExists(&Level1{}) + DB.DropTableIfExists(&Level2{}) + DB.DropTableIfExists(&Level3{}) + DB.DropTableIfExists("level1_level2") + + if err := DB.AutoMigrate(&Level3{}, &Level2{}, &Level1{}).Error; err != nil { + panic(err) + } + + want := Level3{ + Value: "Level3", + Level2: &Level2{ + Value: "Bob", + Level1s: []*Level1{ + {Value: "ru"}, + {Value: "en"}, + }, + }, + } + + if err := DB.Save(&want).Error; err != nil { + panic(err) + } + + var got Level3 + if err := DB.Preload("Level2.Level1s").Find(&got, "value = ?", "Level3").Error; err != nil { + panic(err) + } + + if !reflect.DeepEqual(got, want) { + t.Errorf("got %s; want %s", toJSONString(got), toJSONString(want)) + } +} + +func TestNilPointerSlice(t *testing.T) { + type ( + Level3 struct { + ID uint + Value string + } + Level2 struct { + ID uint Value string Level3ID uint Level3 *Level3 } Level1 struct { - ID uint `gorm:"primary_key;"` + ID uint Value string Level2ID uint Level2 *Level2 From 861c477a3331754c3bf45fc13965793ce295d186 Mon Sep 17 00:00:00 2001 From: Jinzhu Date: Wed, 16 Dec 2015 10:35:22 +0800 Subject: [PATCH 3/7] Test Preload won't panic when nothing found --- preload_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/preload_test.go b/preload_test.go index e1b4efe2..36bfaae5 100644 --- a/preload_test.go +++ b/preload_test.go @@ -6,6 +6,8 @@ import ( "os" "reflect" "testing" + + "github.com/jinzhu/gorm" ) func getPreloadUser(name string) *User { @@ -130,6 +132,10 @@ func TestNestedPreload1(t *testing.T) { if !reflect.DeepEqual(got, want) { t.Errorf("got %s; want %s", toJSONString(got), toJSONString(want)) } + + if err := DB.Preload("Level2").Preload("Level2.Level1").Find(&got, "name = ?", "not_found").Error; err != gorm.RecordNotFound { + panic(err) + } } func TestNestedPreload2(t *testing.T) { @@ -836,6 +842,10 @@ func TestNestedManyToManyPreload(t *testing.T) { if !reflect.DeepEqual(got, want) { t.Errorf("got %s; want %s", toJSONString(got), toJSONString(want)) } + + if err := DB.Preload("Level2s.Level1s").Find(&got, "value = ?", "not_found").Error; err != gorm.RecordNotFound { + panic(err) + } } func TestNestedManyToManyPreload2(t *testing.T) { @@ -889,6 +899,10 @@ func TestNestedManyToManyPreload2(t *testing.T) { if !reflect.DeepEqual(got, want) { t.Errorf("got %s; want %s", toJSONString(got), toJSONString(want)) } + + if err := DB.Preload("Level2.Level1s").Find(&got, "value = ?", "not_found").Error; err != gorm.RecordNotFound { + panic(err) + } } func TestNilPointerSlice(t *testing.T) { From c6a22c50962028255a718f22fe7e8959e8c67884 Mon Sep 17 00:00:00 2001 From: Jinzhu Date: Wed, 16 Dec 2015 10:40:57 +0800 Subject: [PATCH 4/7] Refact fix invalid zero value for Preload --- preload.go | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/preload.go b/preload.go index f3f9dabc..69efc01b 100644 --- a/preload.go +++ b/preload.go @@ -9,20 +9,18 @@ import ( ) func getRealValue(value reflect.Value, columns []string) (results []interface{}) { - for _, column := range columns { - pointedValue := reflect.Indirect(value) - // If v is a nil pointer, Indirect returns a zero Value! - // Therefor we need to check for a zero value, - // as FieldByName could panic - if !pointedValue.IsValid() { - continue - } - if pointedValue.FieldByName(column).IsValid() { - result := pointedValue.FieldByName(column).Interface() - if r, ok := result.(driver.Valuer); ok { - result, _ = r.Value() + // If value is a nil pointer, Indirect returns a zero Value! + // Therefor we need to check for a zero value, + // as FieldByName could panic + if pointedValue := reflect.Indirect(value); pointedValue.IsValid() { + for _, column := range columns { + if pointedValue.FieldByName(column).IsValid() { + result := pointedValue.FieldByName(column).Interface() + if r, ok := result.(driver.Valuer); ok { + result, _ = r.Value() + } + results = append(results, result) } - results = append(results, result) } } return @@ -297,12 +295,6 @@ func (scope *Scope) handleManyToManyPreload(field *Field, conditions []interface } } } else { - // If v is a nil pointer, Indirect returns a zero Value! - // Therefor we need to check for a zero value, - // as FieldByName could panic - if !scope.IndirectValue().IsValid() { - return - } object := scope.IndirectValue() source := getRealValue(object, associationForeignStructFieldNames) field := object.FieldByName(field.Name) From 39e6f5811105dda8b3fd49cb3ab357ef5d0f5a8e Mon Sep 17 00:00:00 2001 From: Richard Knop Date: Wed, 16 Dec 2015 16:58:45 +0800 Subject: [PATCH 5/7] There was still an issue with preloading and pointers. This was actually broken with the refactoring. I added a test to catch the other place the error was occuring. See the new TestManyToManyPreloadForNestedPointer test in preload_test.go. --- preload.go | 11 ++--- preload_test.go | 106 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 5 deletions(-) diff --git a/preload.go b/preload.go index 69efc01b..2c981a79 100644 --- a/preload.go +++ b/preload.go @@ -295,11 +295,12 @@ func (scope *Scope) handleManyToManyPreload(field *Field, conditions []interface } } } else { - object := scope.IndirectValue() - source := getRealValue(object, associationForeignStructFieldNames) - field := object.FieldByName(field.Name) - for _, link := range linkHash[toString(source)] { - field.Set(reflect.Append(field, link)) + if object := scope.IndirectValue(); object.IsValid() { + source := getRealValue(object, associationForeignStructFieldNames) + field := object.FieldByName(field.Name) + for _, link := range linkHash[toString(source)] { + field.Set(reflect.Append(field, link)) + } } } } diff --git a/preload_test.go b/preload_test.go index 36bfaae5..b5188628 100644 --- a/preload_test.go +++ b/preload_test.go @@ -771,6 +771,9 @@ func TestManyToManyPreloadForPointer(t *testing.T) { panic(err) } + var got5 Level2 + DB.Preload("Level1s").First(&got5, "value = ?", "bogus") + var ruLevel1 Level1 var zhLevel1 Level1 DB.First(&ruLevel1, "value = ?", "ru") @@ -783,6 +786,109 @@ func TestManyToManyPreloadForPointer(t *testing.T) { } } +func TestManyToManyPreloadForNestedPointer(t *testing.T) { + type ( + Level1 struct { + ID uint + Value string + } + Level2 struct { + ID uint + Value string + Level1s []*Level1 `gorm:"many2many:levels;"` + } + Level3 struct { + ID uint + Value string + Level2ID sql.NullInt64 + Level2 *Level2 + } + ) + + DB.DropTableIfExists(&Level3{}) + DB.DropTableIfExists(&Level2{}) + DB.DropTableIfExists(&Level1{}) + DB.Table("levels").DropTableIfExists("levels") + + if err := DB.AutoMigrate(&Level3{}, &Level2{}, &Level1{}).Error; err != nil { + panic(err) + } + + want := Level3{ + Value: "Bob", + Level2: &Level2{ + Value: "Foo", + Level1s: []*Level1{ + {Value: "ru"}, + {Value: "en"}, + }, + }, + } + if err := DB.Save(&want).Error; err != nil { + panic(err) + } + + want2 := Level3{ + Value: "Tom", + Level2: &Level2{ + Value: "Bar", + Level1s: []*Level1{ + {Value: "zh"}, + {Value: "de"}, + }, + }, + } + if err := DB.Save(&want2).Error; err != nil { + panic(err) + } + + var got Level3 + if err := DB.Preload("Level2.Level1s").Find(&got, "value = ?", "Bob").Error; err != nil { + panic(err) + } + + if !reflect.DeepEqual(got, want) { + t.Errorf("got %s; want %s", toJSONString(got), toJSONString(want)) + } + + var got2 Level3 + if err := DB.Preload("Level2.Level1s").Find(&got2, "value = ?", "Tom").Error; err != nil { + panic(err) + } + + if !reflect.DeepEqual(got2, want2) { + t.Errorf("got %s; want %s", toJSONString(got2), toJSONString(want2)) + } + + var got3 []Level3 + if err := DB.Preload("Level2.Level1s").Find(&got3, "value IN (?)", []string{"Bob", "Tom"}).Error; err != nil { + panic(err) + } + + if !reflect.DeepEqual(got3, []Level3{got, got2}) { + t.Errorf("got %s; want %s", toJSONString(got3), toJSONString([]Level3{got, got2})) + } + + var got4 []Level3 + if err := DB.Preload("Level2.Level1s", "value IN (?)", []string{"zh", "ru"}).Find(&got4, "value IN (?)", []string{"Bob", "Tom"}).Error; err != nil { + panic(err) + } + + var got5 Level3 + DB.Preload("Level2.Level1s").Find(&got5, "value = ?", "bogus") + + var ruLevel1 Level1 + var zhLevel1 Level1 + DB.First(&ruLevel1, "value = ?", "ru") + DB.First(&zhLevel1, "value = ?", "zh") + + got.Level2.Level1s = []*Level1{&ruLevel1} + got2.Level2.Level1s = []*Level1{&zhLevel1} + if !reflect.DeepEqual(got4, []Level3{got, got2}) { + t.Errorf("got %s; want %s", toJSONString(got4), toJSONString([]Level3{got, got2})) + } +} + func TestNestedManyToManyPreload(t *testing.T) { type ( Level1 struct { From e541ca5cdf1b24eb2b18e6662d22ffa286391a58 Mon Sep 17 00:00:00 2001 From: Jinzhu Date: Wed, 16 Dec 2015 21:00:56 +0800 Subject: [PATCH 6/7] Fix DropTableIfExists with string --- main.go | 4 ++++ preload_test.go | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/main.go b/main.go index 26fc3f18..8e5ed1ac 100644 --- a/main.go +++ b/main.go @@ -392,6 +392,10 @@ func (s *DB) DropTable(values ...interface{}) *DB { func (s *DB) DropTableIfExists(values ...interface{}) *DB { db := s.clone() for _, value := range values { + if tableName, ok := value.(string); ok { + db = db.Table(tableName) + } + db = db.NewScope(value).dropTableIfExists().db } return db diff --git a/preload_test.go b/preload_test.go index b5188628..48d5db02 100644 --- a/preload_test.go +++ b/preload_test.go @@ -632,7 +632,7 @@ func TestManyToManyPreloadWithMultiPrimaryKeys(t *testing.T) { DB.DropTableIfExists(&Level2{}) DB.DropTableIfExists(&Level1{}) - DB.Table("levels").DropTableIfExists("levels") + DB.DropTableIfExists("levels") if err := DB.AutoMigrate(&Level2{}, &Level1{}).Error; err != nil { panic(err) @@ -717,7 +717,7 @@ func TestManyToManyPreloadForPointer(t *testing.T) { DB.DropTableIfExists(&Level2{}) DB.DropTableIfExists(&Level1{}) - DB.Table("levels").DropTableIfExists("levels") + DB.DropTableIfExists("levels") if err := DB.AutoMigrate(&Level2{}, &Level1{}).Error; err != nil { panic(err) @@ -808,7 +808,7 @@ func TestManyToManyPreloadForNestedPointer(t *testing.T) { DB.DropTableIfExists(&Level3{}) DB.DropTableIfExists(&Level2{}) DB.DropTableIfExists(&Level1{}) - DB.Table("levels").DropTableIfExists("levels") + DB.DropTableIfExists("levels") if err := DB.AutoMigrate(&Level3{}, &Level2{}, &Level1{}).Error; err != nil { panic(err) From 2f85c0e4d6a4e04cb68f6635445ceaeee9c26b30 Mon Sep 17 00:00:00 2001 From: Levin Alexander Date: Fri, 18 Dec 2015 11:50:11 +0100 Subject: [PATCH 7/7] clean up wording in README made the paragraph about transactions in callbacks more straightforward --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 38ee3cd0..a9492313 100644 --- a/README.md +++ b/README.md @@ -1064,10 +1064,10 @@ func (u *User) AfterCreate() (err error) { } ``` -As you know, save/delete operations in gorm are running in a transaction, -This is means if changes made in the transaction is not visiable unless it is commited, -So if you want to use those changes in your callbacks, you need to run SQL in same transaction. -Fortunately, gorm support pass transaction to callbacks as you needed, you could do it like this: +Save/delete operations in gorm are running in a transaction. +Changes made in that transaction are not visible unless it is commited. +So if you want to use those changes in your callbacks, you need to run your SQL in the same transaction. +For this Gorm supports passing transactions to callbacks like this: ```go func (u *User) AfterCreate(tx *gorm.DB) (err error) {