From 7fd60cced1b185582d84a2c7c29e4c9e76f99813 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Wed, 8 Dec 2021 10:35:00 +0100 Subject: [PATCH 01/13] Resolve `EagerPreload` panic caused for pointer references Resolves a panic where `EagerPreload` tried to set `reflect.Struct` for a `reflect.Pointer` on 1.. associations. Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- pop_test.go | 21 +++++++++++++++++++++ preload_associations.go | 9 ++++++--- preload_associations_test.go | 12 ++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/pop_test.go b/pop_test.go index bfe7ea78..26184ae1 100644 --- a/pop_test.go +++ b/pop_test.go @@ -106,6 +106,27 @@ type User struct { Houses Addresses `many_to_many:"users_addresses"` } +type UserPointerAssocs struct { + ID int `db:"id"` + UserName string `db:"user_name"` + Email string `db:"email"` + Name nulls.String `db:"name"` + Alive nulls.Bool `db:"alive"` + CreatedAt time.Time `db:"created_at"` + UpdatedAt time.Time `db:"updated_at"` + BirthDate nulls.Time `db:"birth_date"` + Bio nulls.String `db:"bio"` + Price nulls.Float64 `db:"price"` + FullName nulls.String `db:"full_name" select:"name as full_name"` + Books Books `has_many:"books" order_by:"title asc"` + FavoriteSong *Song `has_one:"song" fk_id:"u_id"` + Houses Addresses `many_to_many:"users_addresses"` +} + +func (UserPointerAssocs) TableName() string { + return "users" +} + // Validate gets run every time you call a "Validate*" (ValidateAndSave, ValidateAndCreate, ValidateAndUpdate) method. // This method is not required and may be deleted. func (u *User) Validate(tx *Connection) (*validate.Errors, error) { diff --git a/preload_associations.go b/preload_associations.go index 72a7f4c3..d2985667 100644 --- a/preload_associations.go +++ b/preload_associations.go @@ -335,11 +335,14 @@ func preloadHasOne(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMetaInfo asocValue := slice.Elem().Index(i) if mmi.mapper.FieldByName(mvalue, "ID").Interface() == mmi.mapper.FieldByName(asocValue, foreignField.Path).Interface() || reflect.DeepEqual(mmi.mapper.FieldByName(mvalue, "ID"), mmi.mapper.FieldByName(asocValue, foreignField.Path)) { - if modelAssociationField.Kind() == reflect.Slice || modelAssociationField.Kind() == reflect.Array { + switch { + case modelAssociationField.Kind() == reflect.Slice || modelAssociationField.Kind() == reflect.Array: modelAssociationField.Set(reflect.Append(modelAssociationField, asocValue)) - continue + case modelAssociationField.Kind() == reflect.Ptr: + modelAssociationField.Elem().Set(asocValue) + default: + modelAssociationField.Set(asocValue) } - modelAssociationField.Set(asocValue) } } }) diff --git a/preload_associations_test.go b/preload_associations_test.go index 879531c2..933ef069 100644 --- a/preload_associations_test.go +++ b/preload_associations_test.go @@ -53,6 +53,18 @@ func Test_New_Implementation_For_Nplus1(t *testing.T) { a.Len(book.Writers, 1) a.Equal("Larry", book.Writers[0].Name) a.Equal("Mark", book.User.Name.String) + + usersWithPointers := []UserPointerAssocs{} + a.NoError(tx.All(&usersWithPointers)) + + // FILL THE HAS-MANY and HAS_ONE + a.NoError(preload(tx, &usersWithPointers)) + + a.Len(usersWithPointers[0].Books, 1) + a.Len(usersWithPointers[1].Books, 1) + a.Len(usersWithPointers[2].Books, 1) + a.Equal(usersWithPointers[0].FavoriteSong.UserID, users[0].ID) + a.Len(usersWithPointers[0].Houses, 1) }) } From e3824f8ffae7c3c581c1a44c102af42b8ec5588b Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Wed, 8 Dec 2021 13:45:11 +0100 Subject: [PATCH 02/13] Resolve `EagerPreload` panic caused by NullUUID Resolves a panic where `EagerPreload` was trying to set UUID into NullUUID. Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- associations/belongs_to_association.go | 21 ++++++++++++++------- associations/belongs_to_association_test.go | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/associations/belongs_to_association.go b/associations/belongs_to_association.go index fce161d0..c64d7208 100644 --- a/associations/belongs_to_association.go +++ b/associations/belongs_to_association.go @@ -128,15 +128,22 @@ func (b *belongsToAssociation) BeforeInterface() interface{} { func (b *belongsToAssociation) BeforeSetup() error { ownerID := reflect.Indirect(reflect.ValueOf(b.ownerModel.Interface())).FieldByName("ID") - if b.ownerID.CanSet() { - if n := nulls.New(b.ownerID.Interface()); n != nil { - b.ownerID.Set(reflect.ValueOf(n.Parse(ownerID.Interface()))) - } else if b.ownerID.Kind() == reflect.Ptr { - b.ownerID.Set(ownerID.Addr()) + toSet := b.ownerID + switch b.ownerID.Type().Name() { + case "NullUUID": + b.ownerID.FieldByName("Valid").Set(reflect.ValueOf(true)) + toSet = b.ownerID.FieldByName("UUID") + } + + if toSet.CanSet() { + if n := nulls.New(toSet.Interface()); n != nil { + toSet.Set(reflect.ValueOf(n.Parse(ownerID.Interface()))) + } else if toSet.Kind() == reflect.Ptr { + toSet.Set(ownerID.Addr()) } else { - b.ownerID.Set(ownerID) + toSet.Set(ownerID) } return nil } - return fmt.Errorf("could not set '%s' to '%s'", ownerID, b.ownerID) + return fmt.Errorf("could not set '%s' to '%s'", ownerID, toSet) } diff --git a/associations/belongs_to_association_test.go b/associations/belongs_to_association_test.go index f7a09ba3..ca72c137 100644 --- a/associations/belongs_to_association_test.go +++ b/associations/belongs_to_association_test.go @@ -22,6 +22,11 @@ type barBelongsTo struct { Foo fooBelongsTo `belongs_to:"foo"` } +type barBelongsToNullable struct { + FooID uuid.NullUUID `db:"foo_id"` + Foo *fooBelongsTo `belongs_to:"foo"` +} + func Test_Belongs_To_Association(t *testing.T) { a := require.New(t) @@ -50,3 +55,17 @@ func Test_Belongs_To_Association(t *testing.T) { a.Equal(nil, before[index].BeforeInterface()) } } + +func Test_Belongs_To_Nullable_Association(t *testing.T) { + a := require.New(t) + id, _ := uuid.NewV1() + + bar := barBelongsToNullable{Foo: &fooBelongsTo{id}} + as, err := associations.ForStruct(&bar, "Foo") + a.NoError(err) + + before := as.AssociationsBeforeCreatable() + for index := range before { + a.Equal(nil, before[index].BeforeSetup()) + } +} From ff8d5e4bf4f259f759c651d653f32ae4654489e4 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Thu, 9 Dec 2021 14:21:15 +0100 Subject: [PATCH 03/13] Support pointers in n+1 `Eager` loading Resolves an issue where n+1 eager associations would error with a double pointer in `associations.ForStruct`. Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- finders.go | 9 ++++++++- finders_test.go | 36 ++++++++++++++++++++++++++++++++++++ pop_test.go | 13 +++++++------ 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/finders.go b/finders.go index c90d1032..0df9cc21 100644 --- a/finders.go +++ b/finders.go @@ -282,7 +282,14 @@ func (q *Query) eagerDefaultAssociations(model interface{}) error { v = reflect.Indirect(reflect.ValueOf(model)).FieldByName(inner.Name) innerQuery := Q(query.Connection) innerQuery.eagerFields = inner.Fields - err = innerQuery.eagerAssociations(v.Addr().Interface()) + + switch v.Kind() { + case reflect.Ptr: + err = innerQuery.eagerAssociations(v.Interface()) + case reflect.Struct: + err = innerQuery.eagerAssociations(v.Addr().Interface()) + } + if err != nil { return err } diff --git a/finders_test.go b/finders_test.go index 7e2daa02..cc5bd925 100644 --- a/finders_test.go +++ b/finders_test.go @@ -314,6 +314,42 @@ func Test_Find_Eager_Has_One(t *testing.T) { }) } +func Test_Find_Eager_Has_One_With_Inner_Associations_Pointer(t *testing.T) { + if PDB == nil { + t.Skip("skipping integration tests") + } + transaction(func(tx *Connection) { + r := require.New(t) + + user := UserPointerAssocs{Name: nulls.NewString("Mark")} + err := tx.Create(&user) + r.NoError(err) + + coolSong := Song{Title: "Hook - Blues Traveler", UserID: user.ID} + err = tx.Create(&coolSong) + r.NoError(err) + + u := UserPointerAssocs{} + err = tx.Eager("FavoriteSong.ComposedBy").Find(&u, user.ID) + r.NoError(err) + + r.NotEqual(u.ID, 0) + r.Equal(u.Name.String, "Mark") + r.Equal(u.FavoriteSong.ID, coolSong.ID) + + // eager should work with rawquery + uid := u.ID + u = UserPointerAssocs{} + err = tx.RawQuery("select * from users where id=?", uid).First(&u) + r.NoError(err) + r.Nil(u.FavoriteSong) + + err = tx.RawQuery("select * from users where id=?", uid).Eager("FavoriteSong").First(&u) + r.NoError(err) + r.Equal(u.FavoriteSong.ID, coolSong.ID) + }) +} + func Test_Find_Eager_Has_One_With_Inner_Associations_Struct(t *testing.T) { if PDB == nil { t.Skip("skipping integration tests") diff --git a/pop_test.go b/pop_test.go index 26184ae1..fd2eb020 100644 --- a/pop_test.go +++ b/pop_test.go @@ -118,7 +118,7 @@ type UserPointerAssocs struct { Bio nulls.String `db:"bio"` Price nulls.Float64 `db:"price"` FullName nulls.String `db:"full_name" select:"name as full_name"` - Books Books `has_many:"books" order_by:"title asc"` + Books Books `has_many:"books" order_by:"title asc" fk_id:"user_id"` FavoriteSong *Song `has_one:"song" fk_id:"u_id"` Houses Addresses `many_to_many:"users_addresses"` } @@ -214,11 +214,12 @@ type Address struct { type Addresses []Address type UsersAddress struct { - ID int `db:"id"` - UserID int `db:"user_id"` - AddressID int `db:"address_id"` - CreatedAt time.Time `db:"created_at"` - UpdatedAt time.Time `db:"updated_at"` + ID int `db:"id"` + UserPointerAssocsID int `db:"user_id"` + UserID int `db:"user_id"` + AddressID int `db:"address_id"` + CreatedAt time.Time `db:"created_at"` + UpdatedAt time.Time `db:"updated_at"` } type UsersAddressQuery struct { From c47e802e9a22e9ae58824d057204acca21d76b57 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Thu, 9 Dec 2021 14:21:33 +0100 Subject: [PATCH 04/13] Improve error message of associations.ForStruct Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- associations/associations_for_struct.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/associations/associations_for_struct.go b/associations/associations_for_struct.go index 67c8ca76..544e8786 100644 --- a/associations/associations_for_struct.go +++ b/associations/associations_for_struct.go @@ -1,7 +1,6 @@ package associations import ( - "errors" "fmt" "reflect" "regexp" @@ -30,7 +29,7 @@ var associationBuilders = map[string]associationBuilder{} func ForStruct(s interface{}, fields ...string) (Associations, error) { t, v := getModelDefinition(s) if t.Kind() != reflect.Struct { - return nil, errors.New("could not get struct associations: not a struct") + return nil, fmt.Errorf("could not get struct associations: not a struct but %T", s) } fields = trimFields(fields) associations := Associations{} From 950114c2917b5f6e83db2b5807403783dcb9ec58 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Thu, 9 Dec 2021 14:34:01 +0100 Subject: [PATCH 05/13] Add test cases for `IsZeroOfUnderlyingType` Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- associations/association.go | 4 ++++ associations/association_test.go | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 associations/association_test.go diff --git a/associations/association.go b/associations/association.go index 436c4581..8ed165a4 100644 --- a/associations/association.go +++ b/associations/association.go @@ -160,5 +160,9 @@ func fieldIsNil(f reflect.Value) bool { // IsZeroOfUnderlyingType will check if the value of anything is the equal to the Zero value of that type. func IsZeroOfUnderlyingType(x interface{}) bool { + if x == nil { + return true + } + return reflect.DeepEqual(x, reflect.Zero(reflect.TypeOf(x)).Interface()) } diff --git a/associations/association_test.go b/associations/association_test.go new file mode 100644 index 00000000..504c52e9 --- /dev/null +++ b/associations/association_test.go @@ -0,0 +1,37 @@ +package associations + +import ( + "database/sql" + "fmt" + "github.com/gobuffalo/nulls" + "github.com/gofrs/uuid" + "github.com/stretchr/testify/assert" + "testing" +) + +func Test_IsZeroOfUnderlyingType(t *testing.T) { + for k, tc := range []struct { + in interface{} + zero bool + }{ + {in: nil, zero: true}, + {in: 0, zero: true}, + {in: 1, zero: false}, + {in: false, zero: true}, + {in: "", zero: true}, + {in: interface{}(nil), zero: true}, + {in: uuid.NullUUID{}, zero: true}, + {in: uuid.UUID{}, zero: true}, + {in: uuid.NullUUID{Valid: true}, zero: false}, + {in: nulls.Int{}, zero: true}, + {in: nulls.String{}, zero: true}, + {in: nulls.Bool{}, zero: true}, + {in: nulls.Float64{}, zero: true}, + {in: sql.NullString{}, zero: true}, + {in: sql.NullString{Valid: true}, zero: false}, + } { + t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { + assert.EqualValues(t, tc.zero, IsZeroOfUnderlyingType(tc.in)) + }) + } +} From f87775871b13b1b109d73918d57988cb844c677e Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Thu, 9 Dec 2021 17:23:25 +0100 Subject: [PATCH 06/13] Resolve an obscure bug where empty structs got loaded for NULL foreign keys Closes https://github.com/gobuffalo/pop/issues/139 Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- executors_test.go | 2 +- pop_test.go | 17 ++++-- preload_associations.go | 38 ++++++++++--- preload_associations_test.go | 57 ++++++++++++++++++- .../20181104140606_course_codes.down.fizz | 3 +- .../20181104140606_course_codes.up.fizz | 11 +++- 6 files changed, 108 insertions(+), 20 deletions(-) diff --git a/executors_test.go b/executors_test.go index 3202bfad..db00a428 100644 --- a/executors_test.go +++ b/executors_test.go @@ -1193,7 +1193,7 @@ func Test_Eager_Creation_Without_Associations(t *testing.T) { transaction(func(tx *Connection) { r := require.New(t) code := CourseCode{ - Course: Course{}, + Course: &Course{}, } err := tx.Eager().Create(&code) diff --git a/pop_test.go b/pop_test.go index fd2eb020..c9922678 100644 --- a/pop_test.go +++ b/pop_test.go @@ -291,14 +291,21 @@ type Course struct { } type CourseCode struct { - ID uuid.UUID `json:"id" db:"id"` - CreatedAt time.Time `json:"created_at" db:"created_at"` - UpdatedAt time.Time `json:"updated_at" db:"updated_at"` - CourseID uuid.UUID `json:"course_id" db:"course_id"` - Course Course `json:"-" belongs_to:"course"` + ID uuid.UUID `json:"id" db:"id"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + CourseID uuid.NullUUID `json:"course_id" db:"course_id"` + Course *Course `json:"course" belongs_to:"course" fk_id:"CourseID"` + ClassID uuid.UUID `json:"class_id" db:"class_id"` // Course Course `belongs_to:"course"` } +type Class struct { + ID uuid.UUID `json:"id" db:"id"` + Topic string `json:"topic" db:"topic"` + CourseCodes []CourseCode `json:"course_code_id" has_many:"course_codes"` +} + type ValidatableCar struct { ID int64 `db:"id"` Name string `db:"name"` diff --git a/preload_associations.go b/preload_associations.go index d2985667..6e8fec10 100644 --- a/preload_associations.go +++ b/preload_associations.go @@ -268,13 +268,18 @@ func preloadHasMany(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMetaInf // 3) iterate over every model and fill it with the assoc. foreignField := asoc.getDBFieldTaggedWith(fk) mmi.iterate(func(mvalue reflect.Value) { - modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) for i := 0; i < slice.Elem().Len(); i++ { asocValue := slice.Elem().Index(i) valueField := reflect.Indirect(mmi.mapper.FieldByName(asocValue, foreignField.Path)) if mmi.mapper.FieldByName(mvalue, "ID").Interface() == valueField.Interface() || reflect.DeepEqual(mmi.mapper.FieldByName(mvalue, "ID"), valueField) { - + // IMPORTANT + // + // FieldByName will initialize the value. It is important that this happens AFTER + // we checked whether the field should be set. Otherwise, we'll set a zero value! + // + // This is most likely the reason for https://github.com/gobuffalo/pop/issues/139 + modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) switch { case modelAssociationField.Kind() == reflect.Slice || modelAssociationField.Kind() == reflect.Array: modelAssociationField.Set(reflect.Append(modelAssociationField, asocValue)) @@ -330,11 +335,17 @@ func preloadHasOne(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMetaInfo // 3) iterate over every model and fill it with the assoc. foreignField := asoc.getDBFieldTaggedWith(fk) mmi.iterate(func(mvalue reflect.Value) { - modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) for i := 0; i < slice.Elem().Len(); i++ { asocValue := slice.Elem().Index(i) if mmi.mapper.FieldByName(mvalue, "ID").Interface() == mmi.mapper.FieldByName(asocValue, foreignField.Path).Interface() || reflect.DeepEqual(mmi.mapper.FieldByName(mvalue, "ID"), mmi.mapper.FieldByName(asocValue, foreignField.Path)) { + // IMPORTANT + // + // FieldByName will initialize the value. It is important that this happens AFTER + // we checked whether the field should be set. Otherwise, we'll set a zero value! + // + // This is most likely the reason for https://github.com/gobuffalo/pop/issues/139 + modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) switch { case modelAssociationField.Kind() == reflect.Slice || modelAssociationField.Kind() == reflect.Array: modelAssociationField.Set(reflect.Append(modelAssociationField, asocValue)) @@ -395,13 +406,18 @@ func preloadBelongsTo(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMetaI if isFieldNilPtr(mvalue, fi) { return } - modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) for i := 0; i < slice.Elem().Len(); i++ { asocValue := slice.Elem().Index(i) fkField := reflect.Indirect(mmi.mapper.FieldByName(mvalue, fi.Path)) - if fkField.Interface() == mmi.mapper.FieldByName(asocValue, "ID").Interface() || - reflect.DeepEqual(fkField, mmi.mapper.FieldByName(asocValue, "ID")) { - + field := mmi.mapper.FieldByName(asocValue, "ID") + if fkField.Interface() == field.Interface() || reflect.DeepEqual(fkField, field) { + // IMPORTANT + // + // FieldByName will initialize the value. It is important that this happens AFTER + // we checked whether the field should be set. Otherwise, we'll set a zero value! + // + // This is most likely the reason for https://github.com/gobuffalo/pop/issues/139 + modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) switch { case modelAssociationField.Kind() == reflect.Slice || modelAssociationField.Kind() == reflect.Array: modelAssociationField.Set(reflect.Append(modelAssociationField, asocValue)) @@ -500,11 +516,17 @@ func preloadManyToMany(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMeta mmi.iterate(func(mvalue reflect.Value) { id := mmi.mapper.FieldByName(mvalue, "ID").Interface() if assocFkIds, ok := mapAssoc[fmt.Sprintf("%v", id)]; ok { - modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) for i := 0; i < slice.Elem().Len(); i++ { asocValue := slice.Elem().Index(i) for _, fkid := range assocFkIds { if fmt.Sprintf("%v", fkid) == fmt.Sprintf("%v", mmi.mapper.FieldByName(asocValue, "ID").Interface()) { + // IMPORTANT + // + // FieldByName will initialize the value. It is important that this happens AFTER + // we checked whether the field should be set. Otherwise, we'll set a zero value! + // + // This is most likely the reason for https://github.com/gobuffalo/pop/issues/139 + modelAssociationField := mmi.mapper.FieldByName(mvalue, asoc.Name) modelAssociationField.Set(reflect.Append(modelAssociationField, asocValue)) } } diff --git a/preload_associations_test.go b/preload_associations_test.go index 933ef069..ba883c71 100644 --- a/preload_associations_test.go +++ b/preload_associations_test.go @@ -1,10 +1,10 @@ package pop import ( - "testing" - "github.com/gobuffalo/nulls" + "github.com/gofrs/uuid" "github.com/stretchr/testify/require" + "testing" ) func Test_New_Implementation_For_Nplus1(t *testing.T) { @@ -82,7 +82,7 @@ func Test_New_Implementation_For_Nplus1_With_UUID(t *testing.T) { courses = append(courses, course) if i == 0 { a.NoError(tx.Create(&CourseCode{ - CourseID: course.ID, + CourseID: uuid.NullUUID{UUID: course.ID, Valid: true}, })) } } @@ -113,6 +113,57 @@ func Test_New_Implementation_For_Nplus1_With_UUID(t *testing.T) { }) } +func Test_New_Implementation_For_Nplus1_With_NullUUIDs_And_FK_ID(t *testing.T) { + if PDB == nil { + t.Skip("skipping integration tests") + } + + // This test suite prevents regressions of an obscure bug in the preload code which caused + // pointer values to be set with their empty values when relations did not exist. + // + // See also: https://github.com/gobuffalo/pop/issues/139 + transaction(func(tx *Connection) { + a := require.New(t) + + var course Course + a.NoError(tx.Create(&course)) + + class := &Class{Topic: "math", + // The bug only appears when we have two elements in the slice where + // one has a relation and the other one has no such relation. + CourseCodes: []CourseCode{ + {Course: &course}, + {}, + }} + + // This code basically just sets up + a.NoError(tx.Eager().Create(class)) + + var expected Class + a.NoError(tx.EagerPreload("CourseCodes.Course").First(&expected)) + + // What would happen before the patch resolved this issue is that: + // + // Classes.CourseCodes[0].Course would be the correct value (a filled struct) + // + // "course": { + // "id": "fa51f71f-e884-4641-8005-923258b814f9", + // "created_at": "2021-12-09T23:20:10.208019+01:00", + // "updated_at": "2021-12-09T23:20:10.208019+01:00" + // }, + // + // Classes.CourseCodes[1].Course would an "empty" struct of Course even though there is no relation set up: + // + // "course": { + // "id": "00000000-0000-0000-0000-000000000000", + // "created_at": "0001-01-01T00:00:00Z", + // "updated_at": "0001-01-01T00:00:00Z" + // }, + a.NotNil(expected.CourseCodes[0].Course) + a.Nil(expected.CourseCodes[1].Course) + }) +} + func Test_New_Implementation_For_Nplus1_Single(t *testing.T) { if PDB == nil { t.Skip("skipping integration tests") diff --git a/testdata/migrations/20181104140606_course_codes.down.fizz b/testdata/migrations/20181104140606_course_codes.down.fizz index 1d48c2b8..ddd542fd 100644 --- a/testdata/migrations/20181104140606_course_codes.down.fizz +++ b/testdata/migrations/20181104140606_course_codes.down.fizz @@ -1 +1,2 @@ -drop_table("course_codes") \ No newline at end of file +drop_table('classes') +drop_table("course_codes") diff --git a/testdata/migrations/20181104140606_course_codes.up.fizz b/testdata/migrations/20181104140606_course_codes.up.fizz index f5d81165..6405653f 100644 --- a/testdata/migrations/20181104140606_course_codes.up.fizz +++ b/testdata/migrations/20181104140606_course_codes.up.fizz @@ -1,5 +1,12 @@ +create_table("classes") { + t.Column("id", "uuid", {"primary": true}) + t.Column("topic", "string") + t.DisableTimestamps() +} + create_table("course_codes") { t.Column("id", "uuid", {"primary": true}) - t.Column("course_id", "uuid", {}) + t.Column("course_id", "uuid", {"null":true}) + t.Column("class_id", "uuid") t.Timestamps() -} \ No newline at end of file +} From faa2a005308a97a7044229da894f0970f81afae7 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 09:57:06 +0100 Subject: [PATCH 07/13] Resolve association regression in finders.go Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- finders.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/finders.go b/finders.go index 0df9cc21..ccd4bbd5 100644 --- a/finders.go +++ b/finders.go @@ -286,7 +286,7 @@ func (q *Query) eagerDefaultAssociations(model interface{}) error { switch v.Kind() { case reflect.Ptr: err = innerQuery.eagerAssociations(v.Interface()) - case reflect.Struct: + default: err = innerQuery.eagerAssociations(v.Addr().Interface()) } From 6c837443751b0892feef29c7d28bf8b4d78ab5ba Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 10:13:36 +0100 Subject: [PATCH 08/13] Use dedicated migrations for preloading regression test Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- executors_test.go | 2 +- pop_test.go | 30 ++++++++++++------- preload_associations_test.go | 23 +++++++------- .../20181104140606_course_codes.down.fizz | 3 +- .../20181104140606_course_codes.up.fizz | 11 ++----- .../20210104145901_network.down.fizz | 3 ++ .../migrations/20210104145901_network.up.fizz | 18 +++++++++++ 7 files changed, 56 insertions(+), 34 deletions(-) create mode 100644 testdata/migrations/20210104145901_network.down.fizz create mode 100644 testdata/migrations/20210104145901_network.up.fizz diff --git a/executors_test.go b/executors_test.go index db00a428..3202bfad 100644 --- a/executors_test.go +++ b/executors_test.go @@ -1193,7 +1193,7 @@ func Test_Eager_Creation_Without_Associations(t *testing.T) { transaction(func(tx *Connection) { r := require.New(t) code := CourseCode{ - Course: &Course{}, + Course: Course{}, } err := tx.Eager().Create(&code) diff --git a/pop_test.go b/pop_test.go index c9922678..bb250e9b 100644 --- a/pop_test.go +++ b/pop_test.go @@ -291,19 +291,29 @@ type Course struct { } type CourseCode struct { - ID uuid.UUID `json:"id" db:"id"` - CreatedAt time.Time `json:"created_at" db:"created_at"` - UpdatedAt time.Time `json:"updated_at" db:"updated_at"` - CourseID uuid.NullUUID `json:"course_id" db:"course_id"` - Course *Course `json:"course" belongs_to:"course" fk_id:"CourseID"` - ClassID uuid.UUID `json:"class_id" db:"class_id"` + ID uuid.UUID `json:"id" db:"id"` + CreatedAt time.Time `json:"created_at" db:"created_at"` + UpdatedAt time.Time `json:"updated_at" db:"updated_at"` + CourseID uuid.UUID `json:"course_id" db:"course_id"` + Course Course `json:"-" belongs_to:"course"` // Course Course `belongs_to:"course"` } -type Class struct { - ID uuid.UUID `json:"id" db:"id"` - Topic string `json:"topic" db:"topic"` - CourseCodes []CourseCode `json:"course_code_id" has_many:"course_codes"` +type NetClient struct { + ID uuid.UUID `json:"id" db:"id"` + Hops []Hop `json:"hop_id" has_many:"course_codes"` +} + +type Hop struct { + ID uuid.UUID `json:"id" db:"id"` + NetClient *NetClient `json:"net_client" belongs_to:"net_client" fk_id:"NetClientID"` + NetClientID uuid.UUID `json:"net_client_id" db:"net_client_id"` + Server *Server `json:"course" belongs_to:"server" fk_id:"CourseID"` + ServerID uuid.UUID `json:"server_id" db:"server_id"` +} + +type Server struct { + ID uuid.UUID `json:"id" db:"id"` } type ValidatableCar struct { diff --git a/preload_associations_test.go b/preload_associations_test.go index ba883c71..7fd2313a 100644 --- a/preload_associations_test.go +++ b/preload_associations_test.go @@ -2,7 +2,6 @@ package pop import ( "github.com/gobuffalo/nulls" - "github.com/gofrs/uuid" "github.com/stretchr/testify/require" "testing" ) @@ -82,7 +81,7 @@ func Test_New_Implementation_For_Nplus1_With_UUID(t *testing.T) { courses = append(courses, course) if i == 0 { a.NoError(tx.Create(&CourseCode{ - CourseID: uuid.NullUUID{UUID: course.ID, Valid: true}, + CourseID: course.ID, })) } } @@ -125,28 +124,28 @@ func Test_New_Implementation_For_Nplus1_With_NullUUIDs_And_FK_ID(t *testing.T) { transaction(func(tx *Connection) { a := require.New(t) - var course Course - a.NoError(tx.Create(&course)) + var server Server + a.NoError(tx.Create(&server)) - class := &Class{Topic: "math", + class := &NetClient{ // The bug only appears when we have two elements in the slice where // one has a relation and the other one has no such relation. - CourseCodes: []CourseCode{ - {Course: &course}, + Hops: []Hop{ + {Server: &server}, {}, }} // This code basically just sets up a.NoError(tx.Eager().Create(class)) - var expected Class + var expected NetClient a.NoError(tx.EagerPreload("CourseCodes.Course").First(&expected)) // What would happen before the patch resolved this issue is that: // // Classes.CourseCodes[0].Course would be the correct value (a filled struct) // - // "course": { + // "server": { // "id": "fa51f71f-e884-4641-8005-923258b814f9", // "created_at": "2021-12-09T23:20:10.208019+01:00", // "updated_at": "2021-12-09T23:20:10.208019+01:00" @@ -154,13 +153,13 @@ func Test_New_Implementation_For_Nplus1_With_NullUUIDs_And_FK_ID(t *testing.T) { // // Classes.CourseCodes[1].Course would an "empty" struct of Course even though there is no relation set up: // - // "course": { + // "server": { // "id": "00000000-0000-0000-0000-000000000000", // "created_at": "0001-01-01T00:00:00Z", // "updated_at": "0001-01-01T00:00:00Z" // }, - a.NotNil(expected.CourseCodes[0].Course) - a.Nil(expected.CourseCodes[1].Course) + a.NotNil(expected.Hops[0].Server) + a.Nil(expected.Hops[1].Server) }) } diff --git a/testdata/migrations/20181104140606_course_codes.down.fizz b/testdata/migrations/20181104140606_course_codes.down.fizz index ddd542fd..1d48c2b8 100644 --- a/testdata/migrations/20181104140606_course_codes.down.fizz +++ b/testdata/migrations/20181104140606_course_codes.down.fizz @@ -1,2 +1 @@ -drop_table('classes') -drop_table("course_codes") +drop_table("course_codes") \ No newline at end of file diff --git a/testdata/migrations/20181104140606_course_codes.up.fizz b/testdata/migrations/20181104140606_course_codes.up.fizz index 6405653f..f5d81165 100644 --- a/testdata/migrations/20181104140606_course_codes.up.fizz +++ b/testdata/migrations/20181104140606_course_codes.up.fizz @@ -1,12 +1,5 @@ -create_table("classes") { - t.Column("id", "uuid", {"primary": true}) - t.Column("topic", "string") - t.DisableTimestamps() -} - create_table("course_codes") { t.Column("id", "uuid", {"primary": true}) - t.Column("course_id", "uuid", {"null":true}) - t.Column("class_id", "uuid") + t.Column("course_id", "uuid", {}) t.Timestamps() -} +} \ No newline at end of file diff --git a/testdata/migrations/20210104145901_network.down.fizz b/testdata/migrations/20210104145901_network.down.fizz new file mode 100644 index 00000000..151bae5c --- /dev/null +++ b/testdata/migrations/20210104145901_network.down.fizz @@ -0,0 +1,3 @@ +drop_table("clients") +drop_table("hops") +drop_table("servers") diff --git a/testdata/migrations/20210104145901_network.up.fizz b/testdata/migrations/20210104145901_network.up.fizz new file mode 100644 index 00000000..738b5414 --- /dev/null +++ b/testdata/migrations/20210104145901_network.up.fizz @@ -0,0 +1,18 @@ +create_table("net_clients") { + t.Column("id", "uuid", {"primary": true}) + t.Column("hop_id", "uuid", {"null":true}) + t.ForeignKey("hop_id", {"hops": ["id"]}, {}) + t.DisableTimestamps() +} + +create_table("hops") { + t.Column("id", "uuid", {"primary": true}) + t.DisableTimestamps() +} + +create_table("servers") { + t.Column("id", "uuid", {"primary": true}) + t.Column("client_id", "uuid") + t.ForeignKey("client_id", {"clients": ["id"]}, {}) + t.DisableTimestamps() +} From 9b68e0473633f31468560ffaa3b5a0147e5ec59d Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 10:16:22 +0100 Subject: [PATCH 09/13] Fix code regression Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- pop_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pop_test.go b/pop_test.go index bb250e9b..66410330 100644 --- a/pop_test.go +++ b/pop_test.go @@ -214,12 +214,11 @@ type Address struct { type Addresses []Address type UsersAddress struct { - ID int `db:"id"` - UserPointerAssocsID int `db:"user_id"` - UserID int `db:"user_id"` - AddressID int `db:"address_id"` - CreatedAt time.Time `db:"created_at"` - UpdatedAt time.Time `db:"updated_at"` + ID int `db:"id"` + UserID int `db:"user_id"` + AddressID int `db:"address_id"` + CreatedAt time.Time `db:"created_at"` + UpdatedAt time.Time `db:"updated_at"` } type UsersAddressQuery struct { From 07963a067c387a79b752b2eea3bf1901fb591bf6 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 10:22:55 +0100 Subject: [PATCH 10/13] Fix test code regressions Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- pop_test.go | 12 ++++++------ preload_associations_test.go | 2 +- ...rk.down.fizz => 20210104145902_network.down.fizz} | 0 ...etwork.up.fizz => 20210104145902_network.up.fizz} | 8 ++++---- 4 files changed, 11 insertions(+), 11 deletions(-) rename testdata/migrations/{20210104145901_network.down.fizz => 20210104145902_network.down.fizz} (100%) rename testdata/migrations/{20210104145901_network.up.fizz => 20210104145902_network.up.fizz} (59%) diff --git a/pop_test.go b/pop_test.go index 66410330..43eac628 100644 --- a/pop_test.go +++ b/pop_test.go @@ -300,15 +300,15 @@ type CourseCode struct { type NetClient struct { ID uuid.UUID `json:"id" db:"id"` - Hops []Hop `json:"hop_id" has_many:"course_codes"` + Hops []Hop `json:"hop_id" has_many:"hops"` } type Hop struct { - ID uuid.UUID `json:"id" db:"id"` - NetClient *NetClient `json:"net_client" belongs_to:"net_client" fk_id:"NetClientID"` - NetClientID uuid.UUID `json:"net_client_id" db:"net_client_id"` - Server *Server `json:"course" belongs_to:"server" fk_id:"CourseID"` - ServerID uuid.UUID `json:"server_id" db:"server_id"` + ID uuid.UUID `json:"id" db:"id"` + NetClient *NetClient `json:"net_client" belongs_to:"net_client" fk_id:"NetClientID"` + NetClientID uuid.UUID `json:"net_client_id" db:"net_client_id"` + Server *Server `json:"course" belongs_to:"server" fk_id:"ServerID"` + ServerID uuid.NullUUID `json:"server_id" db:"server_id"` } type Server struct { diff --git a/preload_associations_test.go b/preload_associations_test.go index 7fd2313a..a953b392 100644 --- a/preload_associations_test.go +++ b/preload_associations_test.go @@ -139,7 +139,7 @@ func Test_New_Implementation_For_Nplus1_With_NullUUIDs_And_FK_ID(t *testing.T) { a.NoError(tx.Eager().Create(class)) var expected NetClient - a.NoError(tx.EagerPreload("CourseCodes.Course").First(&expected)) + a.NoError(tx.EagerPreload("Hops.Server").First(&expected)) // What would happen before the patch resolved this issue is that: // diff --git a/testdata/migrations/20210104145901_network.down.fizz b/testdata/migrations/20210104145902_network.down.fizz similarity index 100% rename from testdata/migrations/20210104145901_network.down.fizz rename to testdata/migrations/20210104145902_network.down.fizz diff --git a/testdata/migrations/20210104145901_network.up.fizz b/testdata/migrations/20210104145902_network.up.fizz similarity index 59% rename from testdata/migrations/20210104145901_network.up.fizz rename to testdata/migrations/20210104145902_network.up.fizz index 738b5414..3b6afc06 100644 --- a/testdata/migrations/20210104145901_network.up.fizz +++ b/testdata/migrations/20210104145902_network.up.fizz @@ -1,18 +1,18 @@ create_table("net_clients") { t.Column("id", "uuid", {"primary": true}) - t.Column("hop_id", "uuid", {"null":true}) - t.ForeignKey("hop_id", {"hops": ["id"]}, {}) t.DisableTimestamps() } create_table("hops") { t.Column("id", "uuid", {"primary": true}) + t.Column("server_id", "uuid", {"null":true}) + t.ForeignKey("server_id", {"servers": ["id"]}, {}) + t.Column("net_client_id", "uuid") + t.ForeignKey("net_client_id", {"net_clients": ["id"]}, {}) t.DisableTimestamps() } create_table("servers") { t.Column("id", "uuid", {"primary": true}) - t.Column("client_id", "uuid") - t.ForeignKey("client_id", {"clients": ["id"]}, {}) t.DisableTimestamps() } From b8b0043eb53f785a9fb7ebd63522a7eded9639aa Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 10:28:23 +0100 Subject: [PATCH 11/13] Fix sql migration order Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- testdata/migrations/20210104145902_network.up.fizz | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/testdata/migrations/20210104145902_network.up.fizz b/testdata/migrations/20210104145902_network.up.fizz index 3b6afc06..dc624e6c 100644 --- a/testdata/migrations/20210104145902_network.up.fizz +++ b/testdata/migrations/20210104145902_network.up.fizz @@ -3,6 +3,11 @@ create_table("net_clients") { t.DisableTimestamps() } +create_table("servers") { + t.Column("id", "uuid", {"primary": true}) + t.DisableTimestamps() +} + create_table("hops") { t.Column("id", "uuid", {"primary": true}) t.Column("server_id", "uuid", {"null":true}) @@ -11,8 +16,3 @@ create_table("hops") { t.ForeignKey("net_client_id", {"net_clients": ["id"]}, {}) t.DisableTimestamps() } - -create_table("servers") { - t.Column("id", "uuid", {"primary": true}) - t.DisableTimestamps() -} From e4f3f7e1f090c3def841b1e9eedd3163425521eb Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 13:44:21 +0100 Subject: [PATCH 12/13] Resolve order issue in test --- pop_test.go | 2 +- preload_associations_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pop_test.go b/pop_test.go index 43eac628..0166e9d0 100644 --- a/pop_test.go +++ b/pop_test.go @@ -307,7 +307,7 @@ type Hop struct { ID uuid.UUID `json:"id" db:"id"` NetClient *NetClient `json:"net_client" belongs_to:"net_client" fk_id:"NetClientID"` NetClientID uuid.UUID `json:"net_client_id" db:"net_client_id"` - Server *Server `json:"course" belongs_to:"server" fk_id:"ServerID"` + Server *Server `json:"course" belongs_to:"server" fk_id:"ServerID" oder_by:"id asc"` ServerID uuid.NullUUID `json:"server_id" db:"server_id"` } diff --git a/preload_associations_test.go b/preload_associations_test.go index a953b392..507d04e4 100644 --- a/preload_associations_test.go +++ b/preload_associations_test.go @@ -158,7 +158,7 @@ func Test_New_Implementation_For_Nplus1_With_NullUUIDs_And_FK_ID(t *testing.T) { // "created_at": "0001-01-01T00:00:00Z", // "updated_at": "0001-01-01T00:00:00Z" // }, - a.NotNil(expected.Hops[0].Server) + a.NotNil(expected.Hops[0].Server, "%+v", expected.Hops[0]) a.Nil(expected.Hops[1].Server) }) } From 31bd5b50fda35d981694d7c99bbaf1ea7eb2dd52 Mon Sep 17 00:00:00 2001 From: aeneasr <3372410+aeneasr@users.noreply.github.com> Date: Fri, 10 Dec 2021 13:54:52 +0100 Subject: [PATCH 13/13] Ignore order when testing for nil values in test --- preload_associations_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/preload_associations_test.go b/preload_associations_test.go index 507d04e4..3d70bad9 100644 --- a/preload_associations_test.go +++ b/preload_associations_test.go @@ -158,8 +158,19 @@ func Test_New_Implementation_For_Nplus1_With_NullUUIDs_And_FK_ID(t *testing.T) { // "created_at": "0001-01-01T00:00:00Z", // "updated_at": "0001-01-01T00:00:00Z" // }, - a.NotNil(expected.Hops[0].Server, "%+v", expected.Hops[0]) - a.Nil(expected.Hops[1].Server) + var foundValid, foundEmpty int + for _, hop := range expected.Hops { + if hop.ServerID.Valid { + foundValid++ + a.NotNil(hop.Server, "%+v", hop) + } else { + foundEmpty++ + a.Nil(hop.Server, "%+v", hop) + } + } + + a.Equal(1, foundValid) + a.Equal(1, foundEmpty) }) }