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

componentModel jsr330 not working with dagger DI #571

Closed
afdia opened this Issue Jun 16, 2015 · 20 comments

Comments

Projects
None yet
8 participants
@afdia

afdia commented Jun 16, 2015

Currently if componentModel=jsr330, you're annotating the private fields with @Inject.

This works for most jsr330 based DI frameworks, but unfortunately not for dagger which is based on annotation processing and doesn't use reflection to access private fields (see https://google.github.io/dagger/)

Therefore to make the componentModel work with dagger, one of 2 things would be required:

  1. Use constructor injection instead of field injection
    -> then private fields can be injected without problems
  2. Make fields package private instead
    -> then the fields can be accessed without reflection

For dagger the first approach would be better, because they chose to require an @Inject annotated constructor for construction or otherwise the user has to create an appropriate @provides method (see discussion at square/dagger#412)

But the second approach would also work (and would be easier to implement I guess), even though the user has to make the injectMembers call himself.

And thanks for the nice mapping tool, it's good to have such powerful annotation processing based utilities around!

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Jun 26, 2015

Member

I'd prefer option one as well...

Question is... should we switch to constructor-injection for all jsr330 mappers? Or only if a certain option is set? Or should we add the component model "jsr330-constructor" as an alternative?

Member

agudian commented Jun 26, 2015

I'd prefer option one as well...

Question is... should we switch to constructor-injection for all jsr330 mappers? Or only if a certain option is set? Or should we add the component model "jsr330-constructor" as an alternative?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Jun 28, 2015

Member

+1 For using constructor injection. I think we could switch to that in general, no option should be needed.

And thanks for the nice mapping tool, it's good to have such powerful annotation processing based utilities around!

Thanks for the nice feedback! If you like, you can help by spreading the word on MapStruct by tweeting, blogging etc. :)

Member

gunnarmorling commented Jun 28, 2015

+1 For using constructor injection. I think we could switch to that in general, no option should be needed.

And thanks for the nice mapping tool, it's good to have such powerful annotation processing based utilities around!

Thanks for the nice feedback! If you like, you can help by spreading the word on MapStruct by tweeting, blogging etc. :)

@afdia

This comment has been minimized.

Show comment
Hide comment
@afdia

afdia Jun 28, 2015

I'm not that active on social media, but I've already told several developer collegues about mapstruct ;)

I also think there should be no problem with switching jsr303 to constructor injection except some minor changes:

  • users which subclass a Mapper implementation class would need to call the constructor to initialize the injected fields
  • someone who is using an abstract class for the mappings and writes a constructor could expect that the impl class constructor calls the constructor (I don't know how MapStruct handles this case now)

On the positive side, the mapping impl classes would be easier to initialize in unit-tests which often do not depend on a DI framework even if the production code does, and you could make the private fields final (don't know if that is desired but it would be possible now)

afdia commented Jun 28, 2015

I'm not that active on social media, but I've already told several developer collegues about mapstruct ;)

I also think there should be no problem with switching jsr303 to constructor injection except some minor changes:

  • users which subclass a Mapper implementation class would need to call the constructor to initialize the injected fields
  • someone who is using an abstract class for the mappings and writes a constructor could expect that the impl class constructor calls the constructor (I don't know how MapStruct handles this case now)

On the positive side, the mapping impl classes would be easier to initialize in unit-tests which often do not depend on a DI framework even if the production code does, and you could make the private fields final (don't know if that is desired but it would be possible now)

@megalobrainiac

This comment has been minimized.

Show comment
Hide comment
@megalobrainiac

megalobrainiac Oct 28, 2015

Constructor injection would also be very useful when using Spring DI. While Spring does allow field injection adding (or switching) to constructor injection would allow for a better testing/mocking experience, especially when running unit tests outside of a Spring context.

megalobrainiac commented Oct 28, 2015

Constructor injection would also be very useful when using Spring DI. While Spring does allow field injection adding (or switching) to constructor injection would allow for a better testing/mocking experience, especially when running unit tests outside of a Spring context.

@papo2608

This comment has been minimized.

Show comment
Hide comment
@papo2608

papo2608 Jan 27, 2017

Are there any updates regarding constructor injection/using dagger?

papo2608 commented Jan 27, 2017

Are there any updates regarding constructor injection/using dagger?

@igor-dmitriev

This comment has been minimized.

Show comment
Hide comment
@igor-dmitriev

igor-dmitriev Feb 18, 2017

Hi guys, I am going to implement constructor-injection support, but we need to discuss it first. How can we do that? If we don't change basic implementation we can add a flag useConstructorInjection=true , like @agudian proposed

igor-dmitriev commented Feb 18, 2017

Hi guys, I am going to implement constructor-injection support, but we need to discuss it first. How can we do that? If we don't change basic implementation we can add a flag useConstructorInjection=true , like @agudian proposed

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 18, 2017

Member

Hi @igor-dmitriev, awesome, that's great news! Does anything speaks against just always using constructor injection? If not, I'd prefer that. The less options and knobs, the better :)

Looking forward to the pull request, let us know if you run into an issues along the way. Thanks!

Member

gunnarmorling commented Feb 18, 2017

Hi @igor-dmitriev, awesome, that's great news! Does anything speaks against just always using constructor injection? If not, I'd prefer that. The less options and knobs, the better :)

Looking forward to the pull request, let us know if you run into an issues along the way. Thanks!

@igor-dmitriev

This comment has been minimized.

Show comment
Hide comment
@igor-dmitriev

igor-dmitriev Feb 18, 2017

@gunnarmorling cool, thanks, I agree, I prefer to use constructor-injection. I am wondering if it will be ok for all DI containers, cause I am just using Spring framework.

igor-dmitriev commented Feb 18, 2017

@gunnarmorling cool, thanks, I agree, I prefer to use constructor-injection. I am wondering if it will be ok for all DI containers, cause I am just using Spring framework.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Feb 18, 2017

Member

@igor-dmitriev awesome for stepping up for this one. As @gunnarmorling, I also prefer always using constructor injection. The only thing where I see constructor injection making problems is cyclic dependency. At least for Spring it will fail to generate the Beans.

In my opinion one should never introduce / use such cyclic dependency between beans, but it happens 😄. For this reason I think that we need to have a way to either use field or setter injection instead.

Member

filiphr commented Feb 18, 2017

@igor-dmitriev awesome for stepping up for this one. As @gunnarmorling, I also prefer always using constructor injection. The only thing where I see constructor injection making problems is cyclic dependency. At least for Spring it will fail to generate the Beans.

In my opinion one should never introduce / use such cyclic dependency between beans, but it happens 😄. For this reason I think that we need to have a way to either use field or setter injection instead.

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 18, 2017

Member

Ok, so Spring does not support constructor injection in case of cyclic dependencies between two beans? I thought they'd use proxies to resolve that. But if not, yes, then we should support both indeed.

Member

gunnarmorling commented Feb 18, 2017

Ok, so Spring does not support constructor injection in case of cyclic dependencies between two beans? I thought they'd use proxies to resolve that. But if not, yes, then we should support both indeed.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Feb 18, 2017

Member

I just found out that there is a workaround to support it in the constructor. However, not without annotating the parameters with @Lazy. This is a nice article explaining all the workarounds.

Do you know if other DI frameworks support injection of cyclic dependencies?

Member

filiphr commented Feb 18, 2017

I just found out that there is a workaround to support it in the constructor. However, not without annotating the parameters with @Lazy. This is a nice article explaining all the workarounds.

Do you know if other DI frameworks support injection of cyclic dependencies?

@gunnarmorling

This comment has been minimized.

Show comment
Hide comment
@gunnarmorling

gunnarmorling Feb 18, 2017

Member

Not out of my head. I'd expect CDI to support it. Would be worth trying.

Member

gunnarmorling commented Feb 18, 2017

Not out of my head. I'd expect CDI to support it. Would be worth trying.

@afdia

This comment has been minimized.

Show comment
Hide comment
@afdia

afdia Feb 18, 2017

Typically for jsr330 you should be able to inject a Provider<Dependency> instead of Dependencyand the provider instantiates the object after the first call of the get() method.
See also the Guice wiki.
The javadoc from the jsr330 Provider class mentions lazy injection or cyclic dependencies as a usecase

A problem with the provider is that every call to provider.get() will instantiate a new object (if it's not a singleton). So the generated code would have to make sure that the object is remembered after the first call.
Another minor problem is that I cannot call "new Mapper(new Dependency()) anymore but have to wrap the Dependency into a Provider interface.
Possible solutions:

  • Generate both constructors (all dependencies directly passed and all dependencies passed passed with Providers) but annotate the one with Providers with @Inject
  • add a switch (either between field and constructor injection or between direct and provider injection)

Some DI frameworks have proprietary ways to inject objects lazily (such as the link about Spring).
Dagger2 Lazy class is explained here: https://google.github.io/dagger/users-guide.html (section about lazy injection)
According to stackoverflow, CDI could use the Instance class but doesn't explicitly support lazy initialization

To sum things up I guess a switch between constructor and field injection would be the simplest solution, because typically you want to avoid dependency cycles anyways.

afdia commented Feb 18, 2017

Typically for jsr330 you should be able to inject a Provider<Dependency> instead of Dependencyand the provider instantiates the object after the first call of the get() method.
See also the Guice wiki.
The javadoc from the jsr330 Provider class mentions lazy injection or cyclic dependencies as a usecase

A problem with the provider is that every call to provider.get() will instantiate a new object (if it's not a singleton). So the generated code would have to make sure that the object is remembered after the first call.
Another minor problem is that I cannot call "new Mapper(new Dependency()) anymore but have to wrap the Dependency into a Provider interface.
Possible solutions:

  • Generate both constructors (all dependencies directly passed and all dependencies passed passed with Providers) but annotate the one with Providers with @Inject
  • add a switch (either between field and constructor injection or between direct and provider injection)

Some DI frameworks have proprietary ways to inject objects lazily (such as the link about Spring).
Dagger2 Lazy class is explained here: https://google.github.io/dagger/users-guide.html (section about lazy injection)
According to stackoverflow, CDI could use the Instance class but doesn't explicitly support lazy initialization

To sum things up I guess a switch between constructor and field injection would be the simplest solution, because typically you want to avoid dependency cycles anyways.

@igor-dmitriev

This comment has been minimized.

Show comment
Hide comment
@igor-dmitriev

igor-dmitriev Feb 19, 2017

@filiphr you are right, I think we should give an opportunity to use setter/field/constructor injections. Let's make constructor injection by default and an injection flag should be present, like injection=setter

igor-dmitriev commented Feb 19, 2017

@filiphr you are right, I think we should give an opportunity to use setter/field/constructor injections. Let's make constructor injection by default and an injection flag should be present, like injection=setter

@agudian

This comment has been minimized.

Show comment
Hide comment
@agudian

agudian Feb 19, 2017

Member

Switching to constructor injection by default might actually break things for current users.

So'd actually prefer leaving the field injection the default and allowing to use constructor injection using an option.... Having those two, I don't see much benefit of also adding setter-injection -- in the long run it'll be just another thing we have to take care about.

Member

agudian commented Feb 19, 2017

Switching to constructor injection by default might actually break things for current users.

So'd actually prefer leaving the field injection the default and allowing to use constructor injection using an option.... Having those two, I don't see much benefit of also adding setter-injection -- in the long run it'll be just another thing we have to take care about.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Feb 19, 2017

Member

I think that eventually we need to switch to constructor injection by default. I would propose that either we use field injection in the Beta and the switch it in the final release or we announce the change and then we switch it in the release after we introduce this feature.

Regarding the setter injection. Theoretically we could drop the field injection and use setter injection instead. Regarding Spring there are no difference about what you use. I am not sure about other frameworks though.

Member

filiphr commented Feb 19, 2017

I think that eventually we need to switch to constructor injection by default. I would propose that either we use field injection in the Beta and the switch it in the final release or we announce the change and then we switch it in the release after we introduce this feature.

Regarding the setter injection. Theoretically we could drop the field injection and use setter injection instead. Regarding Spring there are no difference about what you use. I am not sure about other frameworks though.

@kevcodez

This comment has been minimized.

Show comment
Hide comment
@kevcodez

kevcodez Apr 10, 2017

Contributor

Any news on this? Would really like to see constructor injection, especially for testability.

Contributor

kevcodez commented Apr 10, 2017

Any news on this? Would really like to see constructor injection, especially for testability.

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Apr 10, 2017

Member

@kevcodez it is still pending, I am not sure if someone is working on it yet.

Member

filiphr commented Apr 10, 2017

@kevcodez it is still pending, I am not sure if someone is working on it yet.

@kevcodez

This comment has been minimized.

Show comment
Hide comment
@kevcodez

kevcodez Jul 30, 2017

Contributor

I submited a PR to support constructor injection: #1263

Contributor

kevcodez commented Jul 30, 2017

I submited a PR to support constructor injection: #1263

filiphr added a commit to filiphr/mapstruct that referenced this issue Oct 20, 2017

#571 Add Constructor Injection for Annotation Based Component Model
- Allow configuring of injection strategy (Constructor / Field)
- Default injection strategy is Field

filiphr added a commit to filiphr/mapstruct that referenced this issue Oct 20, 2017

@filiphr filiphr added this to the 1.3.x milestone Oct 20, 2017

@filiphr filiphr added the feature label Oct 20, 2017

@filiphr

This comment has been minimized.

Show comment
Hide comment
@filiphr

filiphr Oct 20, 2017

Member

Thanks a lot to @kevcodez for providing the PR for using constructor injection in the generated mappers. The PR #1263 has been merged into master and would be released with the next 1.3.

We have kept the old field injection as default (for now) and there is an option to enable constructor injection. It would be possible via:

  • @Mapper(injectionStrategy = InjectionStrategy.CONSTRUCTOR)
  • @MapperConfig(injectionStrategy = InjectionStrategy.CONSTRUCTOR)
Member

filiphr commented Oct 20, 2017

Thanks a lot to @kevcodez for providing the PR for using constructor injection in the generated mappers. The PR #1263 has been merged into master and would be released with the next 1.3.

We have kept the old field injection as default (for now) and there is an option to enable constructor injection. It would be possible via:

  • @Mapper(injectionStrategy = InjectionStrategy.CONSTRUCTOR)
  • @MapperConfig(injectionStrategy = InjectionStrategy.CONSTRUCTOR)

@filiphr filiphr closed this Oct 20, 2017

spoerri pushed a commit to spoerri/mapstruct that referenced this issue Nov 26, 2017

# This is a combination of 9 commits.
# This is the 1st commit message:
#1269 use update methods for different sources for nested targets

# This is the commit message #2:

[maven-release-plugin] prepare release 1.2.0.CR2

# This is the commit message #3:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #4:

Switch to OpenJDK 7 on Travis (Oracle Java 7 is no longer present)

# This is the commit message #5:

Run build in VM (fixes issues when build is killed by Travis containers)

# This is the commit message #6:

Fix a few errors in reference guide and readme

# This is the commit message #7:

#1283 Handle no suitable empty constructor during BeanMappingMethod creation

# This is the commit message #8:

#1281 Do not depend on deprecated sonatype parent

# This is the commit message #9:

#1273 Fix for NullValueMappingStrategy#RETURN_DEFAULT when using collections

When mapping a collection using NullValueMappingStrategy#RETURN_DEFAULT and the source is null, the target will be an empty collection.

# This is the commit message #10:

Adding Kevin to copyright.txt

# This is the commit message #1:

#744 Improve support for Java 9

When compilig with Java 9 and and source version 1.8 Elements#getTypeElement(CharSequence) returns the types from all modules (such as java.xml.bind or java.xml.datatype).
However if the required modules are not added the classes cannot be used. Therefore, apart from using the Elements we are also checking if the class is also there.

If source version 9 is used then Elements#getTypeElement(CharSequence) works correctly and does not return the types if the modules are not there

# This is the commit message #2:

#1304 Add thrown exceptions to the generated nested mapping methods

# This is the commit message #3:

Add Darren to copyright.txt

# This is the commit message #4:

[maven-release-plugin] prepare release 1.2.0.Final

# This is the commit message #5:

[maven-release-plugin] prepare for next development iteration

# This is the commit message #6:

Update latest stable badge with 1.2.0.Final

# This is the commit message #7:

#1297 Add IntelliJ Formatter to CONTRIBUTING.md

# This is the commit message #8:

#571 Add Constructor Injection for Annotation Based Component Model

- Allow configuring of injection strategy (Constructor / Field)
- Default injection strategy is Field

# This is the commit message #9:

#571 Add test for default injection strategy

# This is the commit message #10:

#1312 Change MapStruct Version in README to latest 1.2.0.Final

# This is the commit message #1:

#610 Add support for unmappedSourcePolicy

# This is the commit message #2:

#610 remove unmapped source policy processor option

# This is the commit message #3:

610 unmappedSourcePolicy with tests

# This is the commit message #4:

#610 test MapperConfig

# This is the commit message #5:

initial attempt at support for ignoring an unmapped source field

# This is the commit message #6:

revert what pull -Xtheirs didn't

# This is the commit message #7:

revert what pull -Xtheirs didn't

# This is the commit message #8:

revert what pull -Xtheirs didn't

# This is the commit message #9:

revert what pull -Xtheirs didn't

# This is the commit message #10:

revert what pull -Xtheirs didn't

# This is the commit message #1:

revert what pull -Xtheirs didn't

# This is the commit message #2:

revert what pull -Xtheirs didn't

# This is the commit message #3:

revert what pull -Xtheirs didn't

# This is the commit message #4:

Update BeanMappingMethod.java

# This is the commit message #5:

Update IgnoreMapper.java

# This is the commit message #6:

Update UnmappedSourceTest.java

# This is the commit message #7:

Update Mapping.java

# This is the commit message #8:

Update Mapping.java

# This is the commit message #9:

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