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 committed Dec 9, 2021
1 parent 950114c commit f877758
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 20 deletions.
2 changes: 1 addition & 1 deletion executors_test.go
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
38 changes: 30 additions & 8 deletions preload_associations.go
Original file line number Diff line number Diff line change
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 Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
}
}
Expand Down
57 changes: 54 additions & 3 deletions preload_associations_test.go
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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 f877758

Please sign in to comment.