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.

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

7 invalid inject #52

Merged

Conversation

tristanexsquare
Copy link
Contributor

Added a test case for the issue and started with first approach to detect struct receivers by comparing the instance with an empty instance. It seems not to be possible to find out struct receivers using reflection.

Current solution is not working for cases with struts that have no fields. See failing test cases...

@bastianccm
Copy link
Contributor

Hi @tristanessquare,

thank you for looking into this issue!
The current approach of comparing the result will probably fail if an empty struct is used in a binding (which is a valid case), so after requestInjection the resulting struct is still empty.

Currently, the logic checks for a Inject method on the pointer type, then calls that and dereferences the element:

dingo/dingo.go

Lines 698 to 708 in 72b6052

case reflect.Ptr:
if setup := current.MethodByName("Inject"); setup.IsValid() {
args := make([]reflect.Value, setup.Type().NumIn())
for i := range args {
if args[i], err = injector.getInstance(setup.Type().In(i), "", circularTrace); err != nil {
return wrapErr(err)
}
}
setup.Call(args)
}
injectlist = append(injectlist, current.Elem())

After dereferencing, it goes through all fields if it's a struct type.
I think we should check if it's not a pointer but still has an Inject method present, and if so, we should fail there.

Something along the lines of

if ctype.Kind() != reflect.Ptr && current.MethodByName("Inject").IsValid() {
    return errInvalidInject
}

prior to the switch

dingo/dingo.go

Line 696 in 72b6052

switch ctype.Kind() {

What do you think?

@tristanexsquare tristanexsquare force-pushed the 7-invalid-inject branch 2 times, most recently from 58efddd to 52911a7 Compare October 28, 2022 12:43
@tristanexsquare
Copy link
Contributor Author

Hi @bastianccm ,

thank you for the great idea. This seems to solve the problem. Now my new test and the existing ones are green.

@bastianccm
Copy link
Contributor

Hi @tristanessquare,

thank you for fixing it! Just mark this ready when you are, and I'm happy to merge it!

@tristanexsquare tristanexsquare marked this pull request as ready for review November 1, 2022 19:49
dingo_test.go Outdated Show resolved Hide resolved
dingo_test.go Show resolved Hide resolved
dingo.go Outdated Show resolved Hide resolved
@bastianccm bastianccm merged commit bea6ce0 into i-love-flamingo:master Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants