From bbf58e60d78a3db4790f0475f4b4ad2980f5219c 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] 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 | 45 +++++++++++---- preload_associations_test.go | 57 ++++++++++++++++++- .../20181104140606_course_codes.down.fizz | 3 +- .../20181104140606_course_codes.up.fizz | 11 +++- 6 files changed, 113 insertions(+), 22 deletions(-) diff --git a/executors_test.go b/executors_test.go index 3202bfad0..db00a4284 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 fd2eb020a..c9922678a 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 d29856674..d7d871547 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)) @@ -360,7 +371,9 @@ func preloadBelongsTo(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMetaI fkids := []interface{}{} mmi.iterate(func(val reflect.Value) { if !isFieldNilPtr(val, fi) { - fkids = append(fkids, mmi.mapper.FieldByName(val, fi.Path).Interface()) + if realValue := mmi.mapper.FieldByName(val, fi.Path).Interface(); !IsZeroOfUnderlyingType(realValue) { + fkids = append(fkids, realValue) + } } }) @@ -376,7 +389,8 @@ func preloadBelongsTo(tx *Connection, asoc *AssociationMetaInfo, mmi *ModelMetaI q.eagerFields = []string{} slice := asoc.toSlice() - err := q.Where(fmt.Sprintf("%s in (?)", fk), fkids).All(slice.Interface()) + vals := slice.Interface() + err := q.Where(fmt.Sprintf("%s in (?)", fk), fkids).All(vals) if err != nil { return err } @@ -395,13 +409,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 +519,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 933ef0694..ba883c71b 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 1d48c2b8f..ddd542fd3 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 f5d811655..6405653f1 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 +}