Adding cond mapping #2148

Desislav-Petrov · 2020-07-11T19:30:17Z

Related to: #2051

* mapstruct#2139 reproducer * mapstruct#2139 solution * mapstruct#2139 license

…hod (mapstruct#2141) * mapstruct#2135 improved messages for not able to select qualified method

Desislav-Petrov · 2020-07-11T19:56:22Z

I'm a bit surprised by the JDK 13 fail - I don't get these locally. Has anybody seen a similar error?

filiphr · 2020-07-17T18:55:52Z

Thanks for the PR @Desislav-Petrov, we are a bit busy with the 1.4 release currently. We will try to find some time to do a proper review on this PR.

Don't worry about the JDK 13 failure, for some reason there are issues like that sporadically on GitHub actions.

Desislav-Petrov · 2020-09-17T13:31:50Z

Hi,

Did you manage to take a look?

Thanks,
Des

filiphr

I finally found the time to do this review.

I left some comments, but those are minor.

I have something more general to say. Thank you for your time working on this PR, it is always nice to get such contributions. However, you'll need to iterate on it again. Currently MapStruct will use what you write in condition without any checks whether the method exists, the source type matches, the method returns boolean etc. There is no discovery of condition methods, which means that it is not possible to define conditions in a util class and use it via Mapper#uses (which is what I assume most people will use). The reason why we do those things is because we want to provide the experience to our users. Do not compile the code if there are certain errors that we can detect and also allow them to use @Condition in case they define it without them needing to write anything in condition.

I hope that you will understand our position on this and the reason why we can't merge this PR in this state.

filiphr · 2020-10-04T08:06:31Z

core/src/main/java/org/mapstruct/Mapping.java

+     *
+     * @return The name of the condition method to be evaluated when trting to do the given target mapping
+     */
+    String condition() default "";


The example that you are using from the defaultExpression is not something valid for condition.

I would suggest the following:

Example:

@Mapping( target = "name", condition = "isNotBlank" )

generates:

if ( isNotBlank( source.getName() ) ) { target.setName( source.getName() ); }

And the condition method should be defined as:

boolean isNotBlank(String value);

I'm not sure that this makes it really flexible:
target="name", condition="isNotBlack" always assumes that the condition will be based on the source's corresponding field but this might not be the case.

It would be much more flexible if the condition method is of the form:
boolean someName(SourceObject type)

Let me know what do you think?

I can follow both ideas. I like having the flexibility @Desislav-Petrov proposes with getting the whole source object to do some checks on this one.
But keeping it strict to the value to which the check is assigned as @filiphr proposes makes it easier to reuse the condition methods. Sticking to the isNotBlank example: This could be used for all String source values, getting the whole source type will make it more complicated to reuse it for other source values.

One idea to support both: Default to @filiphr proposals and if there is an (not annotated) argument for the method this will be the source value.
And also support if an argument was annotated with @MappingSource the source object will be used (do we already have an annotation like this? tbh I dont remember, just can find @MappingTarget that maybe could also be supported?!)

filiphr · 2020-10-04T08:07:38Z

processor/src/main/java/org/mapstruct/ap/internal/model/source/MappingOptions.java


    private static final Pattern JAVA_EXPRESSION = Pattern.compile( "^java\\((.*)\\)$" );

+    public String getCondition() {


Can you please put this getter next to the other getters in this class.

Desislav-Petrov · 2020-10-11T18:58:44Z

Hi Filip,

Thanks for the review I did expect I'd have to iterate over. My intention was to put something as a starting point as it's my first commit to the project, get your feedback and move on from there.

Will try to address your comments shortly.

Thanks

filiphr · 2020-11-21T07:40:22Z

@Desislav-Petrov are you still interested and have time to work and finish this feature? Please let us know if that is not the case in order for us to take over and drive it home.

Desislav-Petrov · 2020-11-21T09:33:47Z

Sorry for the delay - I'm still interested, will try to put updated pr this week.

� Conflicts: � processor/src/main/java/org/mapstruct/ap/internal/processor/creation/MappingResolverImpl.java � processor/src/main/java/org/mapstruct/ap/internal/util/Message.java � processor/src/main/java/org/mapstruct/ap/internal/util/MessageConstants.java

Desislav-Petrov · 2020-11-28T23:18:35Z

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

            }
+
+            //check if there's a condition and if the method exists
+            if ( mapping.getCondition() != null ) {


Tried to do this check in a few different places but this one seems to be the most appropriate one. Could somebody please confirm? If so, I'll add a check for the conditional method param type.

Desislav-Petrov · 2020-11-28T23:24:07Z

Hi @filiphr ,

I've done another pass and would appreciate feedback on a few things:

How would you recommend to do the method discovery in case of using a static? Are there similar examples in the codebase?
Also, I replied on your suggestion for the usage of the conditional from 4th of Oct so waiting on feedback
I've asked a question on my review about the right place to do the type checks

It will take me a few iterations to get there, but I'm sure with a bit of help we will have this feature closed shortly.

Thanks

filiphr · 2021-01-15T20:42:44Z

Hey @Desislav-Petrov,

What I had in mind is to do something similar like we've done with the following:

ObjectFactoryMethodResolver#getFactoryMethod - which is responsible for selecting ObjectFactory methods
LifecycleMethodResolver#beforeMappingMethods - for selecting @BeforeMapping methods
LifecycleMethodResolver#afterMappingMethods - for selecting @AfterMapping methods.

All those go through MethodSelectors which are responsible for selecting the appropriate methods. I would suggest having a look at the different MethodSelector implementations.

The examples above should be able to answer your question 1 and question 3.

OK I read #2148 (comment) and I understand your point. You would like to pass the entire object to determine if something is there or not. To be honest I didn't really think about that possible use case. I was mostly thinking in the direction of checking if a String is not empty / blank, checking that a number is different than something, etc. Basically making this not aware about the source of the location.

At this moment I am not sure whether this makes sense or not. I would advise tackling the first part and then we can iterate again and think about this one.

filiphr · 2021-02-21T20:40:31Z

Hey @Desislav-Petrov, we are planning our 1.5 release and we would like to include this feature as well. Do you think that you will have time to drive this to the end or do you want some of us to take it over and finish it?

Desislav-Petrov · 2021-02-21T21:25:03Z

Hey @filiphr, sorry about the delay, been quite tied up at work. I'm still willing to get this done so I should have another PR the coming week.
Thanks

Desislav-Petrov · 2021-02-28T23:52:36Z

Hi @filiphr,

I've been playing around and managed to get the MethodSelectors working for the conditions that you declare as a part of the interface/abstract class itself. However, it gets more complicated when you consider adding conditions from external static utilities.

First of all, what would you do with the syntax.. would it be condition = fully.qualified.ClassName.method ?

Then, when it comes to the resolution, now you are working with classes that aren't define anywhere in the mapper (not even imported). How would you go about verifying if this exists and has correct target/source params - I'm not sure the examples for lifecycle/object factory cover this case? We can potentially introduce something like @condition, then pull all methods that have this annotation and do the method selection on them?

filiphr · 2021-03-01T20:07:59Z

However, it gets more complicated when you consider adding conditions from external static utilities.

We currently have similar constraints for other types of methods for MapStruct. Therefore, I would say that ideally you will need to create your own utilities and annotate them. Keep in mind that the method selectors use all classes that are available via Mapper#uses.

First of all, what would you do with the syntax.. would it be condition = fully.qualified.ClassName.method ?

I would try to entirely base the condition on the same way qualifiedBy and qualifiedByName works currently.

Perhaps for the future we can add conditionExpression where we would support java(...) where we use it one to one as with the other expressions. On top of that we can add a special expression language where we would be able to resolve some classes, e.g. method(StringUtils#isNotEmpty) and we will look into the available classes passed in Mapper#uses or or method(org.apache.commons.lang3.StringUtils#isNotEmpty) or some other syntax that we would like to use.

However, for this PR I would like to focus on treating condition the same as qualifiedBy

Desislav-Petrov · 2021-03-03T15:55:25Z

Hi,

I've done another pass now, main points:

We support method verification via MethodSelectors now; as you recommended I utilise "uses" to get access to the class where the static utility lies
I've seen how the qualifyBy works but I think what I've done makes it easier to deal with the template generation because you still need to do
if (conditionMethod(source)) {
this.target = source.source;
}
There's a test I've put @ignore on which seems to be failing on my box with & without the changes, not sure if this is a known issue?

If you're happy with that I can do another small polishing pass in terms of any additional exception handling, namings etc.

Regards,
Des

core/src/main/java/org/mapstruct/Mapping.java

chris922 · 2021-03-06T11:40:04Z

core/src/main/java/org/mapstruct/Mapping.java

+     *
+     * @return The name of the condition method to be evaluated when trting to do the given target mapping
+     */
+    String condition() default "";


I can follow both ideas. I like having the flexibility @Desislav-Petrov proposes with getting the whole source object to do some checks on this one.
But keeping it strict to the value to which the check is assigned as @filiphr proposes makes it easier to reuse the condition methods. Sticking to the isNotBlank example: This could be used for all String source values, getting the whole source type will make it more complicated to reuse it for other source values.

One idea to support both: Default to @filiphr proposals and if there is an (not annotated) argument for the method this will be the source value.
And also support if an argument was annotated with @MappingSource the source object will be used (do we already have an annotation like this? tbh I dont remember, just can find @MappingTarget that maybe could also be supported?!)

Co-authored-by: Christian Bandowski <[email protected]>

filiphr

Thanks for your changes @Desislav-Petrov.

I think that I found the the reason why you need @Ignore. Have a look at my comment.

In any case, I still stand by my statement that we should only check based on the mapping type and treat this like a presence check. I don't like the fact that you need to pass the entire object. I would prefer something like isNotBlank and the likes.

We can iterate on it and add support for passing the source object as well. However, the biggest functionality for me currently is to make this work like an external presence check without knowing the source type.

Not that I don't see the benefit of the examples in this PR, but in my opinion these are not covering the biggest number of use cases that I see for this type of feature.

filiphr · 2021-03-07T11:43:28Z

processor/src/main/java/org/mapstruct/ap/internal/util/Message.java

    VALUEMAPPING_ANY_REMAINING_FOR_NON_ENUM( "Source = \"<ANY_REMAINING>\" can only be used on targets of type enum and not for %s." ),
    VALUEMAPPING_ANY_REMAINING_OR_UNMAPPED_MISSING( "Source = \"<ANY_REMAINING>\" or \"<ANY_UNMAPPED>\" is advisable for mapping of type String to an enum type.", Diagnostic.Kind.WARNING  ),
-    VALUEMAPPING_NON_EXISTING_CONSTANT_FROM_SPI( "Constant %s doesn't exist in enum type %s. Constant was returned from EnumMappingStrategy: %s"),
+    VALUEMAPPING_NON_EXISTING_CONSTANT_FROM_SPI( "Constant %s doesn't exist in enum type %s. Constant was returned from EnumNamingStrategy: %s"),


Why is this change needed? This is the reason for the test which is failing and you disabled it with @Ignore.

Sorry - my bad, think it went wrong when I was using IntelliJ renaming refactor, sorted out now.

So based on your previous comment, shall I take that you aren't willing to include this PR?

I see the reasoning for the presence checks approach but I think I see it as complimentary to what I've tried to achieve here, which is based on a real usecase I had for mapstruct.

filiphr · 2021-03-07T12:36:10Z

So based on your previous comment, shall I take that you aren't willing to include this PR?

This PR will not go to waste. I will get it locally and try it out. Maybe I can do some polishing and make sure that both use cases are covered.

Desislav-Petrov · 2021-03-07T12:43:26Z

Sure, thanks. What I suggest we can do is to try to get it to an MVP state for the 1.5 release.

After 1.5 is out, I'm happy to continue enhancing the 'conditional mapping' feature if we have a good idea what we want the end state to look like.

filiphr · 2021-04-25T10:14:30Z

Closing this since the reworked PR #2403 has been integreated.

Thanks a lot for your contribution @Desislav-Petrov. Please give the SNAPSHOT a go and let us know whether it works for you or not.

Desislav-Petrov · 2021-04-25T17:33:55Z

@filiphr I've seen the polished PR, and yes, makes sense to me and covers the usecase.

Thanks for the continuous guidance during the work on this issue, I'm a lot more familiar with the codebase now so hope to be back soon with more PRs :)

sjaakd and others added 3 commits July 5, 2020 14:41

mapstruct#2139 reproducer (mapstruct#2140)

638e132

* mapstruct#2139 reproducer * mapstruct#2139 solution * mapstruct#2139 license

mapstruct#2135 improved messages for not able to select qualified met…

b323c36

…hod (mapstruct#2141) * mapstruct#2135 improved messages for not able to select qualified method

Adding cond mapping

05977a0

Desislav-Petrov mentioned this pull request Jul 11, 2020

[Feature] Conditional mapping #2051

Closed

filiphr force-pushed the master branch from 082db73 to 2a849dc Compare July 17, 2020 23:16

filiphr requested changes Oct 4, 2020

View reviewed changes

Desislav-Petrov added 2 commits November 22, 2020 17:02

Adding checks for return type

fbc1dab

Desislav-Petrov commented Nov 28, 2020

View reviewed changes

Desislav-Petrov added 2 commits November 28, 2020 23:45

Add missing headers

861943d

Style fixes

ec5339d

Merge branch 'master' of https://github.com/mapstruct/mapstruct

3b0e722

Desislav-Petrov added 4 commits March 3, 2021 11:22

Using MethodSelectors

f609bec

Merge branch 'master' of https://github.com/mapstruct/mapstruct

9eee14e

remove prev ignore that was needed to make the build pass

151b8b2

Add ignore

1670ca5

Update copyright list

4ac8c74

chris922 reviewed Mar 6, 2021

View reviewed changes

Update core/src/main/java/org/mapstruct/Mapping.java

e6a90e6

Co-authored-by: Christian Bandowski <[email protected]>

filiphr requested changes Mar 7, 2021

View reviewed changes

Revert ignore

1dd10cb

filiphr mentioned this pull request Apr 4, 2021

Conditional mapping #2403

Merged

1 task

filiphr closed this Apr 25, 2021


		private static final Pattern JAVA_EXPRESSION = Pattern.compile( "^java\\((.*)\\)$" );

		public String getCondition() {

Uh oh!

Adding cond mapping #2148

Adding cond mapping #2148

Uh oh!

Conversation

Desislav-Petrov commented Jul 11, 2020 • edited by filiphr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Desislav-Petrov commented Jul 11, 2020

Uh oh!

filiphr commented Jul 17, 2020

Uh oh!

Desislav-Petrov commented Sep 17, 2020

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Desislav-Petrov commented Oct 11, 2020

Uh oh!

filiphr commented Nov 21, 2020

Uh oh!

Desislav-Petrov commented Nov 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Desislav-Petrov commented Nov 28, 2020

Uh oh!

filiphr commented Jan 15, 2021

Uh oh!

filiphr commented Feb 21, 2021

Uh oh!

Desislav-Petrov commented Feb 21, 2021

Uh oh!

Desislav-Petrov commented Feb 28, 2021

Uh oh!

filiphr commented Mar 1, 2021

Uh oh!

Desislav-Petrov commented Mar 3, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filiphr commented Mar 7, 2021

Uh oh!

Desislav-Petrov commented Mar 7, 2021

Uh oh!

filiphr commented Apr 25, 2021

Uh oh!

Desislav-Petrov commented Apr 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Desislav-Petrov commented Jul 11, 2020 •

edited by filiphr

Loading