-
-
Notifications
You must be signed in to change notification settings - Fork 1k
#2688: Access to the target property name #2834
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
Conversation
sjaakd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivlcic I need to review the implementation a bit more thoroughly. But we have a dependsOn in @Mapping.. without digging (yet) to deep into this problem.. Isn't that usable.?
processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/common/Parameter.java
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/PresenceCheckMethodResolver.java
Show resolved
Hide resolved
I don't really see how this is connected. Basically what I needed was to block contained (sub)collection(s) mapping at runtime based on target property name while reusing same mapper. I have a REST client which can filter out returned properties from deep nested structures (GraphML like), and this is done via runtime parameters. That's why I can't foresee every possible combination and use different mapper (and hence different compile time btw: I squashed commits on my branch again after review. I guessed that this would be easier to compare to master. I hope I didn't spoil your workflow. |
|
I'll need to have a better look into this. However, I would like to thank you for the contribution, the examples look really good (although one might be a bit too complex 😄 ).
I think that @sjaakd wrote that because of the "Complex Example" that you have. Which is doing something really complex, and I do understand why you would need it in your code, we would need to see if we want to keep it in our tests (we would need to scratch our head always when we see it)
It is all fine, we are squashing everything when we merge anyways. The diff is also complete towards the merge branch in a PR, so it is all good. Since we are already out with 1.5.0.RC1, I would like to target this towards 1.6 (with hopefully faster release this time) |
I'm sorry for that "Complex example" and similar - derived tests. I'm a complete noob to Mapstruct and I had only two days to implement "runtime object graph path filtering". So I started I was trying to convince (you) the contributors why this would be useful and is unachievable with You can just leave a comment and I'll fix/remove it. |
...ct/ap/test/conditional/basic/ConditionalMethodForCollectionMapperWithTargetPropertyName.java
Show resolved
Hide resolved
sjaakd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small comments after more thorough review.. In general it looks good to me.. Nice job.
processor/src/test/java/org/mapstruct/ap/test/conditional/basic/ConditionalMappingTest.java
Outdated
Show resolved
Hide resolved
processor/src/test/java/org/mapstruct/ap/test/conditional/basic/ConditionalMappingTest.java
Outdated
Show resolved
Hide resolved
processor/src/test/java/org/mapstruct/ap/test/conditional/basic/ConditionalMappingTest.java
Outdated
Show resolved
Hide resolved
processor/src/test/java/org/mapstruct/ap/test/conditional/basic/ConditionalMappingTest.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/source/SourceMethod.java
Outdated
Show resolved
Hide resolved
I'm fine with it. I first focused on what I observed (did not have too much time either yesterday) and at first glance I thought it also concerned the order of property-mapping (which is a kind of strange assumption at hindsight admittedly). I can see - in your example - you got your inspiration from the life-cycle avoidance context testcases. I'm also happy to see that this did not make it to the unit testcases. |
…not detecting PresenceCheck method. Fixed @TargetPropertyName tests with more assertions. Fixed ConditionalMappingTest @TargetPropertyName testing method names.
Zegveld
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels to me that there is a lot more pass-through code present then necessary.
The set of changes that I request all focus on the same topic, using the same style as is done for TargetType.
I haven't placed remarks at each location where adjustments might be required.
The tests look good, which should make it easier to refactor this.
processor/src/main/resources/org/mapstruct/ap/internal/model/MethodReference.ftl
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/common/ParameterBinding.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/common/ParameterBinding.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/source/SelectionParameters.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/TypeSelector.java
Outdated
Show resolved
Hide resolved
…freemarker tree instead of passing it through java code.
Zegveld
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starts to look great, just one issue remaining concerning the way things are handled at the ParameterBindings.
I think it is also good to add a compiler error in case the @TargetPropertyName annotation is used in a method other than one annotated with @Condition. Because that currently is not supported. What do you think, @filiphr?
Keep up the good work, @ivlcic.
processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/TypeSelector.java
Outdated
Show resolved
Hide resolved
…e manner as @TargetType.
processor/src/main/java/org/mapstruct/ap/internal/model/source/selector/TypeSelector.java
Outdated
Show resolved
Hide resolved
sjaakd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice piece of work @ivlcic .. thanks!
I do not think that we are doing that for |
|
In any case if we need to add custom checks for some annotations we can always add a custom |
filiphr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally to the time to have a proper look into this.
Thanks @sjaakd and @Zegveld for the reviews.
@ivlcic, I've pushed some small cleanup changes. Hope you are OK with them.
This is almost ready to be merged. I have 2 things to ask:
- Can we please add some documentation for this?
- Can we please move the tests to their own package and dedicated test. e.g.
org.mapstruct.ap.test.conditional.targetpropertyname. Would also be good to extract the different implementation of theTargetPropertyNameModelin their own classes. I think that it will make the tests a bit more readable.
Moved test domain model classes out of the hiding TargetPropertyNameModel interface. Renamed TargetPropertyNameModel to DomainModel since it is still used in tests.
|
|
@Zegveld, @filiphr, @sjaakd ... Thanks for all your help with this! One small side note (ticket #2882, commit #2883): Correction for Regards! |
|
I there anything more to be done from my side? |
|
Thanks for your work on this @ivlcic. I think that the only think that is reaming is for us to integrate this into main. I hope to do this as soon as possible. |
filiphr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a final pass of this and started to integrate it into main. I have only one question @ivlcic.
Can you please let me know what you think about that?
I'll merge and fix the conflcits afterwards (almost done with that locally)
Implemented
Access to the target property name #2688
As agreed in discussion: #2831
Rationale
Currently there is no option to do run-time filtering based on target property name.
This comes very handy when mapping larger object graphs from ORM frameworks, to block
unnecessary loading from database and reuse mapping code.
With this feature you can for instance block further mapping in runtime based on some
@Contextvariable state and reuse mapper code without having to foresee every possible property mapping combination.Implementation
Implementation introduces support for new presence check method parameter annotated with
@TargetPropertyName.It is implemented with minimum impact to current code base, so it might be in architecture/elegance sense problematic.
It introduces no changes any current features; it's only an addition.
Unit tests are provided in
org/mapstruct/ap/test/conditional.Example 1
Example 2
Complex Example