Skip to content

Commit

Permalink
Resolve an obscure bug where empty structs got loaded for NULL foreig…
Browse files Browse the repository at this point in the history 
…n keys

Closes #139

Signed-off-by: aeneasr <3372410+aeneasr@users.noreply.github.com>
  • Loading branch information
@aeneasr
aeneasr committed Dec 9, 2021
1 parent 950114c commit bbf58e6
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 22 deletions.
2 changes: 1 addition & 1 deletion executors_test.go
View file
Expand Up @@ -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)
Expand Down
17 changes: 12 additions & 5 deletions pop_test.go
View file
Expand Up @@ -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"`
Expand Down
45 changes: 35 additions & 10 deletions preload_associations.go
View file
Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand All @@ -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)
}
}
})

Expand All @@ -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
}
Expand All @@ -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))
Expand Down Expand Up @@ -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))
}
}
Expand Down
57 changes: 54 additions & 3 deletions preload_associations_test.go
View file
@@ -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) {
Expand Down Expand Up @@ -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},
}))
}
}
Expand Down Expand Up @@ -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")
Expand Down
3 changes: 2 additions & 1 deletion testdata/migrations/20181104140606_course_codes.down.fizz
View file
@@ -1 +1,2 @@
drop_table("course_codes")
drop_table('classes')
drop_table("course_codes")
11 changes: 9 additions & 2 deletions testdata/migrations/20181104140606_course_codes.up.fizz
View file
@@ -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()
}
}

0 comments on commit bbf58e6

Please sign in to comment.