fix: use reflect.Append when preloading nested associations instead of making a slice with fixed size #7014

emilienkofman · 2024-05-10T11:38:07Z

Do only one thing
Non breaking API changes
Tested

What did this pull request do?

attempt to fix issue #7013

User Case Description

Joins Preload causes panic for a large (>20) number of items, see the issue.

@a631807682 I've tried this since I guess it's better to have a slice with the proper len and cap (if we know it when making the slice) instead of appending items to it. However I'm not very comfortable with the preload func and could not figure out which len and cap values to use for the other schema.MakeSlice calls.

Also, can you tell me where I could add a test case for that? Something that would look like the playground test I guess. maybe joins_test.go ?

a631807682

However I'm not very comfortable with the preload func and could not figure out which len and cap values to use for the other schema.MakeSlice calls.

In fact, in most cases we cannot determine Len and Cap, and it is reasonable to provide a default Cap.

Also, can you tell me where I could add a test case for that? Something that would look like the playground test I guess. maybe joins_test.go ?

Yes, please add unit tests in joins_test.go

emilienkofman · 2024-05-13T09:21:58Z

I've tried adding a unit test, I could reproduce the issue but I still get a panic stack trace in here in func schema.GetIdentityFieldValuesMap and cannot figure out exactly what happens...

full stack trace:

 joins_test.go:418: 
        	Error Trace:	/home/emilien/Workspace/src/github.com/emilienkofman/gorm/tests/joins_test.go:418
        	Error:      	func (assert.PanicTestFunc)(0xaeafe0) should not panic
        	            		Panic value:	reflect: call of reflect.Value.Field on zero Value
        	            		Panic stack:	goroutine 18 [running]:
        	            	runtime/debug.Stack()
        	            		/home/emilien/Workspace/go/src/runtime/debug/stack.go:24 +0x5e
        	            	github.com/stretchr/testify/assert.didPanic.func1()
        	            		/home/emilien/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:1196 +0x6b
        	            	panic({0xcdf020?, 0xc0004a6648?})
        	            		/home/emilien/Workspace/go/src/runtime/panic.go:770 +0x132
        	            	reflect.Value.Field({0x0?, 0x0?, 0x79c092630f18?}, 0x10?)
        	            		/home/emilien/Workspace/go/src/reflect/value.go:1284 +0xcf
        	            	gorm.io/gorm/schema.(*Field).setupValuerAndSetter.func2({0xceb680?, 0xc0004c4348?}, {0xc9c380?, 0xc0004880b0?, 0x0?})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/schema/field.go:467 +0xb6
        	            	gorm.io/gorm/schema.GetIdentityFieldValuesMap({0xf0d2f0, 0x1566380}, {0xca6ec0?, 0xc0004a6618?, 0xd13fc0?}, {0xc0004164f0, 0x1, 0xc000432020?})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/schema/utils.go:153 +0x953
        	            	gorm.io/gorm/callbacks.preload(0xc000453ad0, 0xc0002235f0, {0x0, 0x0, 0x0?}, 0xc000453aa0)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/preload.go:260 +0x2e73
        	            	gorm.io/gorm/callbacks.preloadEntryPoint(0xc000453a10, {0xc0004a0b50, 0x1, 0x1}, 0xc000290de8, 0xc000453950, {0x0, 0x0, 0x0})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/preload.go:155 +0x895
        	            	gorm.io/gorm/callbacks.preloadEntryPoint(0xc0004538c0, {0xc0004a0b20, 0x1, 0x1}, 0xc000290de8, 0xc0004533e0, {0x0, 0x0, 0x0})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/preload.go:138 +0xd14
        	            	gorm.io/gorm/callbacks.Preload(0xc000453380)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/query.go:283 +0x2bc
        	            	gorm.io/gorm.(*processor).Execute(0xc00021eaa0, 0xc00020def0?)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks.go:130 +0x3cc
        	            	gorm.io/gorm.(*DB).Find(0xce9a60?, {0xca6f40, 0xc000455230}, {0x0, 0x0, 0x0})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/finisher_api.go:170 +0x134
        	            	gorm.io/gorm/tests_test.TestJoinsPreload_Issue7013.func1()
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/tests/joins_test.go:422 +0x109
        	            	github.com/stretchr/testify/assert.didPanic(0x1491d50?)
        	            		/home/emilien/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:1201 +0x82
        	            	github.com/stretchr/testify/assert.NotPanics({0xf050c0, 0xc00012bba0}, 0xc000455248, {0x0, 0x0, 0x0})
        	            		/home/emilien/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:1272 +0x7e
        	            	gorm.io/gorm/tests_test.TestJoinsPreload_Issue7013(0xc00012bba0)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/tests/joins_test.go:418 +0x27a
        	            	testing.tRunner(0xc00012bba0, 0xe285b0)
        	            		/home/emilien/Workspace/go/src/testing/testing.go:1689 +0xfb
        	            	created by testing.(*T).Run in goroutine 1
        	            		/home/emilien/Workspace/go/src/testing/testing.go:1742 +0x390
        	Test:       	TestJoinsPreload_Issue7013

Any suggestion is welcome!

a631807682 · 2024-05-14T01:50:22Z

I've tried adding a unit test, I could reproduce the issue but I still get a panic stack trace in here in func schema.GetIdentityFieldValuesMap and cannot figure out exactly what happens...

full stack trace:

 joins_test.go:418: 
        	Error Trace:	/home/emilien/Workspace/src/github.com/emilienkofman/gorm/tests/joins_test.go:418
        	Error:      	func (assert.PanicTestFunc)(0xaeafe0) should not panic
        	            		Panic value:	reflect: call of reflect.Value.Field on zero Value
        	            		Panic stack:	goroutine 18 [running]:
        	            	runtime/debug.Stack()
        	            		/home/emilien/Workspace/go/src/runtime/debug/stack.go:24 +0x5e
        	            	github.com/stretchr/testify/assert.didPanic.func1()
        	            		/home/emilien/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:1196 +0x6b
        	            	panic({0xcdf020?, 0xc0004a6648?})
        	            		/home/emilien/Workspace/go/src/runtime/panic.go:770 +0x132
        	            	reflect.Value.Field({0x0?, 0x0?, 0x79c092630f18?}, 0x10?)
        	            		/home/emilien/Workspace/go/src/reflect/value.go:1284 +0xcf
        	            	gorm.io/gorm/schema.(*Field).setupValuerAndSetter.func2({0xceb680?, 0xc0004c4348?}, {0xc9c380?, 0xc0004880b0?, 0x0?})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/schema/field.go:467 +0xb6
        	            	gorm.io/gorm/schema.GetIdentityFieldValuesMap({0xf0d2f0, 0x1566380}, {0xca6ec0?, 0xc0004a6618?, 0xd13fc0?}, {0xc0004164f0, 0x1, 0xc000432020?})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/schema/utils.go:153 +0x953
        	            	gorm.io/gorm/callbacks.preload(0xc000453ad0, 0xc0002235f0, {0x0, 0x0, 0x0?}, 0xc000453aa0)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/preload.go:260 +0x2e73
        	            	gorm.io/gorm/callbacks.preloadEntryPoint(0xc000453a10, {0xc0004a0b50, 0x1, 0x1}, 0xc000290de8, 0xc000453950, {0x0, 0x0, 0x0})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/preload.go:155 +0x895
        	            	gorm.io/gorm/callbacks.preloadEntryPoint(0xc0004538c0, {0xc0004a0b20, 0x1, 0x1}, 0xc000290de8, 0xc0004533e0, {0x0, 0x0, 0x0})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/preload.go:138 +0xd14
        	            	gorm.io/gorm/callbacks.Preload(0xc000453380)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks/query.go:283 +0x2bc
        	            	gorm.io/gorm.(*processor).Execute(0xc00021eaa0, 0xc00020def0?)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/callbacks.go:130 +0x3cc
        	            	gorm.io/gorm.(*DB).Find(0xce9a60?, {0xca6f40, 0xc000455230}, {0x0, 0x0, 0x0})
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/finisher_api.go:170 +0x134
        	            	gorm.io/gorm/tests_test.TestJoinsPreload_Issue7013.func1()
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/tests/joins_test.go:422 +0x109
        	            	github.com/stretchr/testify/assert.didPanic(0x1491d50?)
        	            		/home/emilien/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:1201 +0x82
        	            	github.com/stretchr/testify/assert.NotPanics({0xf050c0, 0xc00012bba0}, 0xc000455248, {0x0, 0x0, 0x0})
        	            		/home/emilien/go/pkg/mod/github.com/stretchr/[email protected]/assert/assertions.go:1272 +0x7e
        	            	gorm.io/gorm/tests_test.TestJoinsPreload_Issue7013(0xc00012bba0)
        	            		/home/emilien/Workspace/src/github.com/emilienkofman/gorm/tests/joins_test.go:418 +0x27a
        	            	testing.tRunner(0xc00012bba0, 0xe285b0)
        	            		/home/emilien/Workspace/go/src/testing/testing.go:1689 +0xfb
        	            	created by testing.(*T).Run in goroutine 1
        	            		/home/emilien/Workspace/go/src/testing/testing.go:1742 +0x390
        	Test:       	TestJoinsPreload_Issue7013

Any suggestion is welcome!

When joins relation is nil, we need to avoid preload nil values. This happens with Joins and LeftJoins.
In this example, not all Users have Manager , so the panic occurs.
We can not specify the length of the slice and add it through reflect.Append based on conditional filtering.

emilienkofman · 2024-05-14T09:29:00Z

ok thanks @a631807682 I think I got it rigth this time. Tell me if something needs to be improved or modified. What about the failing test for mysql env 🤔

2024/05/14 09:23:45 /home/runner/work/gorm/gorm/tests/upsert_test.go:283 Error 1062 (23000): Duplicate entry '375' for key 'users.PRIMARY'
[1.033ms] [rows:0] INSERT INTO `users` (`created_at`,`updated_at`,`deleted_at`,`name`,`age`,`birthday`,`company_id`,`manager_id`,`active`,`id`) VALUES ('2024-05-14 09:23:45.598','2024-05-14 09:23:45.602',NULL,'find or create 4',44,NULL,NULL,NULL,false,375)
FAIL
FAIL	gorm.io/gorm/tests	16.064s
FAIL

Also, am I allowed to use testify/assert lib (it's quite common)

callbacks/preload.go

tests/joins_test.go

tests/tests_test.go

tests/joins_test.go

emilienkofman · 2024-05-16T13:07:25Z

I fixed as you suggested and squashed my commits, let me know if you need anyting else regarding this issue

System-Glitch · 2024-05-27T08:05:57Z

Any ETA for this to be merged? For now I have to downgrade to 1.12.9.

ChronosMasterOfAllTime · 2024-07-10T19:09:22Z

Can we please get this released @jinzhu @a631807682? It's made 1.25.10 a breaking version for us, and we need to use v1.25.9

a631807682 reviewed May 11, 2024

View reviewed changes

a631807682 reviewed May 14, 2024

View reviewed changes

callbacks/preload.go Show resolved Hide resolved

callbacks/preload.go Show resolved Hide resolved

tests/joins_test.go Show resolved Hide resolved

a631807682 reviewed May 16, 2024

View reviewed changes

tests/tests_test.go Outdated Show resolved Hide resolved

tests/joins_test.go Show resolved Hide resolved

fix: use reflect.Append when preloading nested associations

d7421cf

emilienkofman force-pushed the fix/issue-7013-joins-preload-panic branch from 6afd2df to d7421cf Compare May 16, 2024 13:05

emilienkofman changed the title ~~fix: consider len and cap in schema.MakeSlice~~ fix: use reflect.Append when preloading nested associations instead of making a slice with fixed size May 16, 2024

a631807682 approved these changes May 17, 2024

View reviewed changes

jinzhu merged commit 05167fd into go-gorm:master Jun 12, 2024
28 of 36 checks passed

pedro-rijo-deel mentioned this pull request Aug 30, 2024

Join Preload panics #7013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use reflect.Append when preloading nested associations instead of making a slice with fixed size #7014

fix: use reflect.Append when preloading nested associations instead of making a slice with fixed size #7014

emilienkofman commented May 10, 2024 •

edited

Loading

a631807682 left a comment

emilienkofman commented May 13, 2024

a631807682 commented May 14, 2024 •

edited

Loading

emilienkofman commented May 14, 2024

emilienkofman commented May 16, 2024

System-Glitch commented May 27, 2024

ChronosMasterOfAllTime commented Jul 10, 2024 •

edited

Loading

fix: use reflect.Append when preloading nested associations instead of making a slice with fixed size #7014

fix: use reflect.Append when preloading nested associations instead of making a slice with fixed size #7014

Conversation

emilienkofman commented May 10, 2024 • edited Loading

What did this pull request do?

User Case Description

a631807682 left a comment

Choose a reason for hiding this comment

emilienkofman commented May 13, 2024

a631807682 commented May 14, 2024 • edited Loading

emilienkofman commented May 14, 2024

emilienkofman commented May 16, 2024

System-Glitch commented May 27, 2024

ChronosMasterOfAllTime commented Jul 10, 2024 • edited Loading

emilienkofman commented May 10, 2024 •

edited

Loading

a631807682 commented May 14, 2024 •

edited

Loading

ChronosMasterOfAllTime commented Jul 10, 2024 •

edited

Loading