#1427 Add additional configuration for Jsr330 and Spring component models by chris922 · Pull Request #1437 · mapstruct/mapstruct

chris922 · 2018-04-18T18:45:05Z

Added two new annotations that allows to configure the component models, e. g. the bean name

@MapperSpringConfig
@MapperJsr330Config

This adds the functionality requested in ticket #1427. A few more information from my side are available there.
I added @Since 1.3 to new classes, so in case the PR will be accepted after 1.3 was released I/someone must update this.

sjaakd · 2018-04-21T11:42:28Z

@chris922 First of all, nice idea. But I also need to think about it a bit more..

I think we need to discuss this with the MapStruct team.
Are the configurations mutual exclusive..? What happens if you specify them both?
Is there a need for the component model parameter in @Mapping when we introduce such configuration annotations? Or should we cut the suffix @..Config?
I'm not a big fan of using the 'value' in the annotations.. There's an implicit meaning to be attached.. I'm not sure whether in a @..Config the name is the proper candidate for value.
What about CDI?

Anyway.. If we are going to swap out the componentName for annotations, that should be 2.0.. (which is upcoming 😄. ).

chris922 · 2018-04-21T13:43:22Z

Hi @sjaakd,

Let me know the outcome of your discussion! Would be happy if this PR will be merged and if not then at least I learned more internal stuff from mapstruct (what was my main intention)
Only the configuration that matches the configured componentModel in @Mapper will be used. So in case you define @MapperJsr330Config but set your componentModel to spring it will be ignored. Maybe a warning should be printed in this case?
Right now it is required. Maybe we can make it optional but then we've got issues in case both new annotations will be used (I would propose to stop the processing with an error)
I could remove the value attribute and use componentName or something similar if you prefer
No special configuration for CDI available right now, any ideas what could be configured? I never used CDI myself.. always Spring ;)

To what componentName attribute do you refer? In 1.2.0 I am not aware about something like this. Already something new that I missed?

Thanks for your feedback!

Best regards
Christian

sjaakd · 2018-04-21T15:59:50Z

I learned more internal stuff from mapstruct (what was my main intention)

I really appreciate that. Its good to have more folks on board. 😄.

To what componentName attribute do you refer? In 1.2.0 I am not aware about something like this. Already something new that I missed?

My wrong... Doing too much at the same time.. I meant componentModel. I could envision that you only need to specify (a form of) your annotation in stead of a componentModel in the @Mapper annotation. But that would be downward incompatible and a natural candidate for a 2.0 version.

chris922 · 2018-04-21T16:10:30Z

I could envision that you only need to specify (a form of) your annotation in stead of a componentModel in the @Mapper annotation. But that would be downward incompatible and a natural candidate for a 2.0 version.

I think we could already implement that componentModel is not required without removing it - just mark it as deprecated and don't remove the functionality.
And in 2.0 it could be removed. So that both things are valid.. maybe with the componentModel as the leading value.

filiphr · 2018-04-21T16:54:39Z

This is indeed an interesting PR. Replacing the Mapper#componentModel with a new annotation sounds like a good idea. We would only need to see how the inheritance of the component model would work. As currently one can set it via @MapperConfig as well. Maybe in 2.0 we can even add support for meta annotations, so we might not need the config anymore 😃.

Using a custom annotation would be easier to add injection specific parameters, like the delegateQualifier and componentType in the MapperSpringConfig.

@chris922 I had a quick glance over it and there are some places that I think we can improve one:

Instead of specifying our own enum for SpringComponentType maybe we can have a string or a class that would be the annotation that needs to be used. In that case the users can even pick their own custom meta annotations
The names, one of the most difficult thing in programming 😃. Maybe something like @SpringMapper, @Jsr330Mapper, etc, or add Config in the end.
There are some other general changes in the way some internals are used, but we can go over that later

In any case our plan is to release 1.3.0.Beta1 as soon as possible. If you don't mind I would put this for the release after that 1.3.0.Beta2.

chris922 · 2018-04-22T12:36:43Z

Hi @filiphr ,

I started with the same idea to allow specifying the annotation directly, but I choosed the enum so that it is easier to allow different generations based on the componentType and also to restrict it to the 4 possible component annotations from Spring. Otherwise it could be misused for other stuff. Of course it could be restricted to those annotations and it is still possible to do different things during generation, but I always find enumerations easier. Better overview about possibilities and built-in restriction to those values
"There are only two hard things in computer science: [...] and naming things"... you know ;)
Just @SpringMapper/@Jsr330Mapper sounds fine for me. Short and meaningful.
Okay, just let me know.. e. g. add comments in the code 😃

I try to summarize the current status:

Make componentModel deprecated in @Mapper and make it possible to just use the new annotations
1.1. componentModel is leading, print WARN in case componentModel doesnt match used annotation / multiple annotations used
1.2. WARN as soon as componentModel will be used as it is "deprecated and will be removed in 2.0"
1.3. ERROR in case multiple component model annotations are used in parallel
Add @CdiMapper annotation (without special configuration right now, or do you have any ideas what could be configured?)
No annotation + no componentModel = default mapping (as is)
Rename value in new annotations to componentName
Rename annotations to @SpringMapper and @Jsr330Mapper

If those things fit your expectation I would already start with them. Please let me know.

I will wait for feedback if the componentType should be switched to Class<? extends Annotation> or String.
And of course if you have some other ideas or comments.

EDIT: Ahh and what I missed: What about componentModel within @MapperConfig and mapstruct.defaultComponentModel processor option. Maybe just let it there?
Common configuration for @SpringMapper/@Jsr330Mapper doesn't make sense for most of the possible configuration values that exist right now, maybe just for delegateQualifier.

sjaakd · 2018-04-22T13:33:40Z

Hmm. its a pity java does not allow you to extend annotations. It would make perfect sense to let Jsr330Mapper inherit from Mapper 😞

…mponent models Added two new annotations that allows to configure the component models, e. g. the bean name * MapperSpringConfig * MapperJsr330Config

* Renamed MapperJsr330Config to Jsr330Mapper, attribute value to name * Renamed MapperSpringConfig to SpringMapper, attribute value to name * Added deprecated warning to Mapper#componentModel * Added CdiMapper annotation

@Mapper

…g componentModel attribute in @Mapper Additionally added cdi-api dependency to unit tests (was it the first unit-testcase for cdi component model?)

chris922

I just pushed the changes to add the things I mentioned in my last comment. As I am unsure about a few things I did I added review comments inline.

I am looking forward for your feedback!

chris922 · 2018-04-24T22:20:46Z

+@Documented
+@Target(ElementType.ANNOTATION_TYPE)
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ComponentModelMapper {


I had to add this meta-annotation that allows to recognize the @SpringMapper/.. annotations, otherwise it is not possible to detect that no such annotation exist and the default component model should be used - or if multiple component model annotations exists (-> ERROR).

chris922 · 2018-04-24T22:21:28Z

        </dependency>
+        <dependency>
+            <groupId>javax.enterprise</groupId>
+            <artifactId>cdi-api</artifactId>


Seems that I added the first CDI unit tests ... hopefully in a correct way

chris922 · 2018-04-24T22:23:11Z

+        // this prevents creating many instances of this configuration for the same Element
+        // it will be assumed that this method will never be called with different messagers for the same Element
+        // one reason to cache instances is that the the MapperConfiguration validates the Element and multiple
+        // instantiations would lead to multiple identical warnings/errors


Is this okay? Maybe a WeakHashMap is too much as a processor is only a short-running process. Maybe you've got better ideas

chris922 · 2018-04-24T22:25:01Z

+                    mapperPrism.componentModel()
+                );
+            }
+        }


I added the validation of the mapper here, there is also a TODO a few lines earlier that some more validations are required here. Removing the printMessage calls here would allow to remove the configurations cache.

chris922 · 2018-04-24T22:25:47Z

-    private MapperConfiguration(MapperPrism mapperPrism) {
-        this.mapperPrism = mapperPrism;
+    private MapperConfiguration(Element e, FormattingMessager messager) {
+        this.mapperPrism = MapperPrism.getInstanceOn( e );


Moved the MapperPrism.getInstanceOn to the constructor as the Element is required a few lines later. I also added the FormattingMessager to be able to print the validation messages. I don't know if validation is fine at this point, maybe there is some more general place when the @Mapper annotated classes will be loaded?

filiphr · 2018-05-04T20:38:36Z

@chris922 just to let you know that we haven't forgotten this one. We are focused on the 1.3.0.Beta1 release and we want to get it out as soon as possible (and issues just keep popping up 😟). Once we get that out I am going to have a look at your comments and update in this one. I hope this is fine for you

filiphr · 2022-10-01T20:29:08Z

Closing this since it is now possible to achieve this with the new @AnnotateWith annotation.

This was done in PR #3019

chris922 added 3 commits April 25, 2018 00:17

mapstruct#1427 Add additional configuration for Jsr330 and Spring co…

962d4ec

…mponent models Added two new annotations that allows to configure the component models, e. g. the bean name * MapperSpringConfig * MapperJsr330Config

Further component model enhancements

f3ff115

* Renamed MapperJsr330Config to Jsr330Mapper, attribute value to name * Renamed MapperSpringConfig to SpringMapper, attribute value to name * Added deprecated warning to Mapper#componentModel * Added CdiMapper annotation

Allow usage of @SpringMapper/@Jsr330Mapper/@CdiMapper without definin…

6f04625

…g componentModel attribute in @Mapper Additionally added cdi-api dependency to unit tests (was it the first unit-testcase for cdi component model?)

chris922 force-pushed the 1427_mapper-doesnt-support-custom-name-for-spring-service-annotation branch from 283c3a7 to 6f04625 Compare April 24, 2018 22:18

chris922 commented Apr 24, 2018

View reviewed changes

chris922 mentioned this pull request Oct 17, 2019

Allow custom scope annotations for dagger #1928

Open

filiphr self-assigned this Oct 27, 2021

filiphr closed this Oct 1, 2022

Uh oh!

Conversation

chris922 commented Apr 18, 2018

Uh oh!

sjaakd commented Apr 21, 2018

Uh oh!

chris922 commented Apr 21, 2018

Uh oh!

sjaakd commented Apr 21, 2018

Uh oh!

chris922 commented Apr 21, 2018

Uh oh!

filiphr commented Apr 21, 2018

Uh oh!

chris922 commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaakd commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris922 left a comment

Choose a reason for hiding this comment

Uh oh!

chris922 Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

chris922 Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

chris922 Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

chris922 Apr 24, 2018

Choose a reason for hiding this comment

Uh oh!

chris922 Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr commented May 4, 2018

Uh oh!

filiphr commented Oct 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chris922 commented Apr 22, 2018 •

edited

Loading

sjaakd commented Apr 22, 2018 •

edited

Loading

chris922 Apr 24, 2018 •

edited

Loading

filiphr commented Oct 1, 2022 •

edited

Loading