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

auto-detect @Inject method parameter types #172

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

dev-qnz
Copy link
Contributor

@dev-qnz dev-qnz commented Mar 2, 2024

In their default behaviour, the extensions will automatically start/stop Weld SE container and inject into all your `@Inject` fields and method parameters in the given test instance.
says that the extension will

inject into all your @Inject fields and method parameters in the given test instance

However, it looks like only the field type is detected and added to the container and not the method parameter type.

Also,

    private Foo foo;

    @Inject
    private void setFoo(Foo foo) {
        this.foo = foo;
    }

should essentially be short for

    @Inject
    private Foo foo;

But only the field injection works as of now as far as I figure

@manovotn
Copy link
Collaborator

manovotn commented Mar 2, 2024

In their default behaviour, the extensions will automatically start/stop Weld SE container and inject into all your `@Inject` fields and method parameters in the given test instance.

says that the extension will

inject into all your @Inject fields and method parameters in the given test instance

However, it looks like only the field type is detected and added to the container and not the method parameter type.

Yes, the README talks about what Weld tries to inject (mainly due to its greedy nature) and not really about what it scans in the auto-mode.

Also,

    private Foo foo;

    @Inject
    private void setFoo(Foo foo) {
        this.foo = foo;
    }

should essentially be short for

    @Inject
    private Foo foo;

But only the field injection works as of now as far as I figure

The two injections shouldn't be equal, at least not in case of junit - test instances used by junit are not beans provided by Weld meaning declaring an initializer in there makes little sense.
There was a discussion leading to this gotcha in #156 and also in #144

@manovotn
Copy link
Collaborator

manovotn commented Mar 2, 2024

Ok, I was fairly surprised why this works in the first place but now I see why.

We treat the test instance as an InjectionTarget where we invoke injectionTarget.inject(...) - this, among other things, means that it will look for initializer and invoke that as well.

@@ -101,7 +101,7 @@ static void scanForRequiredBeanClasses(List<Class<?>> testClasses, Weld weld, bo
.forEach(cls -> addClassesToProcess(classesToProcess, cls));

AnnotationSupport.findAnnotatedMethods(currClass, Inject.class, HierarchyTraversalMode.BOTTOM_UP).stream()
.map(Method::getReturnType)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, I haven't understood the purpose of getReturnType as it was there before. I also can't find initializer method return types mentioned in the specs (https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0#declaring_initializer). Could it possibly have been a bug, here, to evaluate the return type or shall that be kept and if so what for?
It may not hurt if it works now, regardless of how the test is instantiated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't understand it either and did some digging too - I think it's a bug/oversight from when the annotation based approach was added.
In CDI, @Inject usage on methods is basically either initializer or annotated constructor neither of which has interesting return type that we should process here.

@manovotn manovotn linked an issue Mar 4, 2024 that may be closed by this pull request
Copy link
Collaborator

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

@manovotn manovotn merged commit ae37349 into weld:master Mar 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-detect @Inject method parameter types
2 participants