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

IMPROVEMENT: Discover existing mappers instead of providing "Mapper.uses" class array #1509

Open
ldolbir opened this issue May 29, 2018 · 8 comments

Comments

@ldolbir
Copy link

ldolbir commented May 29, 2018

Would it be possible to enable dynamic discovery of the mapper classes used in a generated code according to the source and target class through the package annotation configuration like:
@Mapper (usesPackage = {"org.company.componenent.mappers"})

@filiphr
Copy link
Member

filiphr commented Jun 1, 2018

This is an interesting proposal. We would need to make sure that the mapper itself doesn't get detected. However, apart from that it should be fine.

Will you expect to find mappers in sub packages as well? Or only in a single one?

@chris922
Copy link
Member

chris922 commented Jun 4, 2018

Should every class be detected as a possible mapper? In this case the annotation should be used with caution and thus only if the package just contains "real" mappers.

To prevent issues in case every class will be seen as a possible mapper I would propose to just search in the defined package and not in sub-packages. OR: Allow something like an "ant-path" syntax (org.company.component.mappers.*, org.company.component.mappers.**)

@sjaakd
Copy link
Contributor

sjaakd commented Jun 4, 2018 via email

@ldolbir
Copy link
Author

ldolbir commented Jun 4, 2018

First of all THANK YOU! All of you for coming up with this library. It is easy to use and very intuitive. I use mappers in conjunction with JAXB and we have 4 different enterprise schemas to map (86 mappers). We could use less, but why repeat the code ???
Feature:
The new attribute should work the same way the component-scan attribute works with Spring Beans. if you don't specify the package, the scan is not enabled and the Mapper works the same way it does today (i.e backward compatible). If you do specify a package, it will find all annotated classes recursively starting with the referenced package. Since the matching will be performed dynamically using "To" and "From" types it should warn of duplicates and use the last one it finds (just like Spring).

Benefits:

  1. Prevent direct reference of the Mappers in the definition. The greatest feature of this library is it will adjust the mapping when the mapped Type's implementation is changing. By referencing the re-used Mapper classes the code, the library becomes less flexible/re-usable.

Example: Mapped type class was modified to include new attribute that is already has a mapper. Unless code inspection is performed the possible re-use will not be discovered.

  1. Remove the need to list large number of classes in the Mapper definition.
    Thanks again!

@sjaakd
Copy link
Contributor

sjaakd commented Jun 4, 2018

First of all THANK YOU! All of you for coming up with this library

Thanks for the feedback. Appreciate that.

If you do specify a package, it will find all annotated classes recursively starting with the referenced package.

You mean, classes annotated with @Mapper? We also support hand-written mappers. Those would not be resolved, right?

Since the matching will be performed dynamically using "To" and "From" types it should warn of duplicates and use the last one it finds (just like Spring).

That's tricky, right..? You'll never know if the last method it finds is the correct one. But in essence, in case there are multiple qualifying methods you would issue a warning in stead of an error (as is the case now)? Note we have Qualifier to resolve conflicts currently.

Just out of curiosity: we have the concept of MapperConfig. That's intended to centralise certain settings. 'Uses' is also there.

@ldolbir
Copy link
Author

ldolbir commented Jun 4, 2018

I use MapperConfig. It is very useful (especially for unmappedTargetPolicy). But it doesn't provide support for example scenario I outlined above.
Thanks!

@ldolbir
Copy link
Author

ldolbir commented Jun 4, 2018

Additionally, I would annotate both the Generated and Hand-written mappers if one desire to get them discovered (something like @Mapper and @ManualMapper). In my humble opinion it will give the framework good level of consistency. But I love it already, so you must be doing something right!

@sjaakd
Copy link
Contributor

sjaakd commented Jun 4, 2018

I use MapperConfig. It is very useful (especially for unmappedTargetPolicy). But it doesn't provide support for example scenario I outlined above.
Thanks!

I meant that you only have to specify your list of mappers once.. You could use it as a kind of central mapper repository. I'm not sure though how MapStruct handles a reference to the mapper itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants