Conditional mapping #2403

filiphr · 2021-04-04T17:49:52Z

This is my iteration of the conditional mapping.

There are 2 commits, one which refactors the presence checks into an interface and using Freemarker templates, and the second one that adds the functionality.

There is a new annotation @Condition which has to be on a method in order for such a method to be considered as a custom presence check method.

There are new attributes in @Mapping:

conditionBy - similar to qualifiedBy but only for @Condition methods
conditionByName - similar to qualifiedByName but only for @Condition methods
conditionExpression - which allows to define the expression in Java format.

This PR uses the same concept for discovering custom presence check methods through the MethodSelectors.

Fixes #2051
Fixes #2084

This PR should make PR #2148 and PR #2097 obsolete.

@ivangsa can you please check if this satisfies your requirement? The approach here is slightly different than the one proposed by you in PR #2097. Check ConditionalMappingTest#optionalLikeConditional and OptionalLikeConditionalMapper.

@Desislav-Petrov can you also have a look at this? This makes it possible to use the source parameters, @Context parameters and the source right hand side.

@sjaakd can you also please have a look at this. Check the 2 commits separate, since they are entirely independent, and the first one is there to make it simpler for the conditional mapping.

Todos before merge:

Add documentation section

sjaakd · 2021-04-05T07:41:39Z

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

+                throw new IllegalStateException( "Method " + method + " has no source parameter." );
+            }
+
+            String previousPropertyName = sourceParameter.getName();


is it previousPropertyName or originalSourceParameterName

It is actually both. The idea is to replace

<#macro localVarName index><#if index == 0>${sourceParameter.name}<#else>${propertyEntries[index-1].name}</#if></#macro>

In the NestedPropertyMappingMethod.ftl. It is the previous defined variable that should be used. In the first property it is the source parameter, but then it is the previous property

sjaakd · 2021-04-05T07:46:06Z

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

                        if (entry.getPresenceChecker() != null && entry.getReadAccessor() != null) {
-                            sourcePresenceChecker += " && " + variableName + " != null && "
-                                + variableName + "." + entry.getPresenceChecker().getSimpleName() + "()";
+                            presenceChecks.add( new NullPresenceCheck( variableName ) );


I see that we add a NullPresenceCheck and a SourceReferenceMethodPresenceCheck .. Why not assume delegation of the null check to a default SourceReferenceMethodPresenceCheck? Wouldn't that give

More room to play for the implementor of SourceReferenceMethodPresenceCheck? (null is a value, right)

Give more readable code?

Or perhaps I don't understand it sufficient yet at this point in time (first review pass).

This is a special one which is only did for #1456. My idea was to make the presence checks independent of each other. The SourceReferenceMethodPresenceCheck is also used in the case when we need to call hasProperty() on a source bean. In this case we shouldn't do a null check for the source since it has already been done at the beginning of the method. We want to avoid the following scenario

public Target map(Source source) { if ( source == null ) { return null; } Target target = new Target(); if ( source != null && source.hasName() ) { target.setName( source.getName() ); } }

The SourceReferenceMethodPresenceCheck is also used in the case when we need to call hasProperty() on a source bean

So, the hasXYZ is one of the 'default' presence checkers? Is this overridden as well as you define a conditionBy? (so if you've defined has methods.. what will happen in relation to the new stuff)?

Yes hasXYZ is one of the 'default' presence checkers. If you define a custom @Condition method that is applicable for that type then, this will be used, i.e. your presence checker on the bean won't be used. Perhaps, those should have highest precedence instead?

sjaakd · 2021-04-05T07:50:09Z

...r/src/main/java/org/mapstruct/ap/internal/model/presence/AllPresenceChecksPresenceCheck.java

+/**
+ * @author Filip Hrisafov
+ */
+public class AllPresenceChecksPresenceCheck extends ModelElement implements PresenceCheck {


From this I assume you want to concatenate multiple presence checks.. But as mentioned earlier, there could be a dependency: a presence checker not being executed while a previous one already fired.

I'm not sure if we need that.. E.g.: can you skip the null presence check? Or couldn't that one be the default to override (instead of add a check after that). ?

How much is this all steered by the strategy enums on method-class-mapper level?

Should we deprecate the strategy enums.. now we have more fine grained control.. ? E.g. should this be 2.0 functionality?

I don't quite follow with what you mean by "a presence checked not being executed while a previous one already fired". This AllPresenceChecksPresenceCheck is meant mostly to cover the use case in #1456.

How much is this all steered by the strategy enums on method-class-mapper level?

This is entirely steared by the strategy enums. All the behaviour stays the same, basically NullValueCheckStrategy can be though as PresenceValueCheckStrategy.

Should we deprecate the strategy enums.. now we have more fine grained control.. ? E.g. should this be 2.0 functionality?

I don't think that we should deprecate them. I think that it should behave the same way as it did before. With this we are giving more options for the users to write presence checks without changing their beans.

I don't quite follow with what you mean by "a presence checked not being executed while a previous one already fired". This AllPresenceChecksPresenceCheck is meant mostly to cover the use case in #1456.

I suspected that the following code is generated:

if ( source != null && pc1( source ) && pc2( source) { target.setName( source.getName() ); }

The value null will never be offered to the pc1. So pc1 cannot make choices anymore on null.. but reading your answer that does not happen right? The same is valid for pc1 and pc2 by the way, but at least that is something that the user has under control.

This is entirely steared by the strategy enums. All the behaviour stays the same, basically NullValueCheckStrategy can be though as PresenceValueCheckStrategy.

What about NullValuePropertyMappingStrategy..?

The point is that this interface is getting too complex. You need to define a strategy and then you need to do something in your @Mapping to customise this.

I would plea for a solution that we

make behaviour consistent between:. `NullValuePropertyMappingStrategy' and 'NullValueCheckStrategy'

then drop the 2 `NullValuePropertyMappingStrategy' and 'NullValueCheckStrategy'

extend @Condition to be used on @MapperConfig and @Mapper

have some default @Conditions for reuse. Like the null check and the has check.

have a script to apply to source code migrate the current two strategies. I'm thinking about the way that assertj was done from its predecessor.

Rethink the remaining strategy NullValueMappingStrategy and at least call it something different: SourceBeanNullCheck or apply a special case of the @Condition to this scenario as well.

Don't get me wrong.. I really like the approach. But I think the API is becoming too complex and too dependent.

So perhaps we should call the next release 2.0 alpha or something like that.

I see what you are saying. In this situation we are not looking for custom presence checkers here, this will work exactly like it did before. The code you think that is generated with pc1 and pc2 it won't be generated.

I am not following your plea, perhaps we need to do a sync meeting about it 😉

sjaakd

You are right about collections I spotted a time ago that since we support automatic mapping we don't need in-situ collection mapping anymore. Perhaps also something we could drop for 2.0 to make it more consistent.

About de strategies.. I still think this way of working could at least offer a viable approach to deprecate the nullvaluecheck strategy. After all, you could provide a more generic option in your code:

@Condition
boolean ispresent( Object o ) {
   return o != null;
}

Thinking whether we should do this already for 1.5 and mention how to do this (in order to prepare users for 2.0)?

…al mapping

…stom presence check methods

filiphr · 2021-04-24T20:21:20Z

About de strategies.. I still think this way of working could at least offer a viable approach to deprecate the nullvaluecheck strategy. After all, you could provide a more generic option in your code:

You are right the NullValueCheckStrategy indeed has no impact once you add @Condition.

We need to investigate this, but I think such a condition will lead to an "ambiguous error" like I have created in #1216

filiphr · 2021-04-24T20:22:03Z

Thanks for the review btw. I have rebased this on top of the main branch and got rid of the used NullValueCheckStrategy

filiphr requested a review from sjaakd April 4, 2021 17:49

filiphr force-pushed the conditional-mapping branch from ff857d3 to d1de9a8 Compare April 4, 2021 17:50

sjaakd reviewed Apr 5, 2021

View reviewed changes

sjaakd approved these changes Apr 24, 2021

View reviewed changes

filiphr added 2 commits April 24, 2021 22:10

Refactor presence checks to object in order to simplify the condition…

484d846

…al mapping

mapstruct#2051, mapstruct#2084 Add new @Condition annotation for cu…

b2e620e

…stom presence check methods

filiphr force-pushed the conditional-mapping branch from ce61a5a to b2e620e Compare April 24, 2021 20:18

filiphr merged commit 51cdbd6 into mapstruct:master Apr 25, 2021

filiphr deleted the conditional-mapping branch April 25, 2021 10:09

This was referenced Apr 25, 2021

Adding cond mapping #2148

Closed

adding custom "nested presence checkers" support #674 and #2084 #2097

Closed

Uh oh!

Conditional mapping #2403

Conditional mapping #2403

Uh oh!

Conversation

filiphr commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjaakd Apr 5, 2021 • 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 Apr 5, 2021 • 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!

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!

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!

sjaakd Apr 10, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

sjaakd left a comment

Choose a reason for hiding this comment

Uh oh!

filiphr commented Apr 24, 2021

Uh oh!

filiphr commented Apr 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

filiphr commented Apr 4, 2021 •

edited

Loading

sjaakd Apr 5, 2021 •

edited

Loading

filiphr Apr 5, 2021 •

edited

Loading

sjaakd Apr 10, 2021 •

edited

Loading