Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from
+82 −5

Conversation

emilienkofman
Copy link

@emilienkofman emilienkofman commented May 10, 2024 •
edited

  • 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
a631807682 reviewed May 11, 2024
View reviewed changes
Copy link
Member

@a631807682 a631807682 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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/testify@v1.9.0/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/testify@v1.9.0/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/testify@v1.9.0/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
Copy link
Member

a631807682 commented May 14, 2024 •
edited

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/testify@v1.9.0/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/testify@v1.9.0/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/testify@v1.9.0/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
Copy link
Author

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)

a631807682
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
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
@emilienkofman emilienkofman force-pushed the fix/issue-7013-joins-preload-panic branch from 6afd2df to d7421cf Compare May 16, 2024 13:05
@emilienkofman 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
@emilienkofman
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a631807682 a631807682 a631807682 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@emilienkofman @a631807682