GeezerVison

Hi @ro0sterjam
it would be very nice to add an option to configure the target null transformation
from this

to this

It would be useful for cases there Optional is a container for additional state (e.g. if an attribute was present at all in the patch request. so it maps like Optional.present -> value, Optional.empty -> null set, null -> undefined/not set in a request, this is supported OOB in jackson)

Hi @ro0sterjam it would be very nice to add an option to configure the target null transformation from this

Source Type Target Type Source Value Target Value
Object Optional null Empty
Optional Optional null Empty
to this

Source Type Target Type Source Value Target Value
Object Optional null null
Optional Optional null null
It would be useful for cases there Optional is a container for additional state (e.g. if an attribute was present at all in the patch request. so it maps like Optional.present -> value, Optional.empty -> null set, null -> undefined/not set in a request, this is supported OOB in jackson)

Honestly that seems reasonable to me and was originally going to support this case if not for this comment here #674 (comment)

That said, I do agree and think we should allow the user to decide what they want, similar with collections.

If we did do this though, should we conflate it with NullValueMappingStrategy.RETURN_DEFAULT and NullValuePropertyMappingStrategy.SET_TO_DEFAULT? Perhaps not, considering that:

For all other objects an new instance is created, requiring an empty constructor.

Which I would argue is less common of a desired outcome than always returning a non null for an Optional is. So I would probably argue that it's worth introducing a new flag here. Perhaps something like DefaultOptionalValueStrategy.USE_NULL|USE_EMPTY?

Incidentally, similar to Optional, I believe never returning a null in a Collection is also best practice. I sort of feel like these should also not be conflated with String, Boxed Types, and simple Objects, but rather fall under the same configuration as Optional (if you're following best practices with Optional, you're probably following best practices with Collections as well); hoping to strike a healthy balance of flexibility and complexity.

With that, I might propose that we introduce instead a config such as: DefaultCollectionAndOptionalValueStrategy.USE_NULL|USE_EMPTY which only ensures that Optionals, Collections, and Maps are never null if USE_EMPTY is configured; otherwise it respects the mapping table that you suggest.

Edit:
Looks like I might have misunderstood what NullValuePropertyMappingStrategy is for... https://stackoverflow.com/questions/66776058/mapstruct-1-4-2-final-nullvaluepropertymappingstrategy-set-to-default-not-work

@ro0sterjam just out of curiosity: is this going to change how we currently handle Optional. There might be may users out there which already have their own way of handling this. In other words: should we consider a switch so we switch it default off?

BTW: one of the things that I consider up for change in a 2.0 version is the way we've done all these XYZStrategies. Especially the null check stuff. They kind of emerged over time. I don't think they are very intuitive. @filiphr : WDYT?

One use case for Optional mapping is for Patch request body:
For controller what you want is:

• if null then skip mapping (not present in input Json)
• if empty then set target to empty value.
• else unwrat value

When we want to use for input to call an external RestApi:

• if I want to set a value then wrap the value
• if I want to delete a value then set to empty (this will set Json property to null)
• if I don't want the patch to ignore a property set it to null (Json property absent in resulting Json)

It's possible to obtain this?

BTW: one of the things that I consider up for change in a 2.0 version is the way we've done all these XYZStrategies. Especially the null check stuff. They kind of emerged over time. I don't think they are very intuitive. @filiphr : WDYT?

We've been talking about these strategies for a while. I do agree with you that it might be a good idea to reiterate on them for 2.0.

@sjaakd regarding the affect on current code. Won't the rule of MapStruct of using something which already exists be applied as well? I haven't reviewed the PR, so I am not sure to what you are referring to.

@lysz210 I am really against in MapStruct treating null optional as a value. The entire idea of the optional is not to have null. For what you are asking I think that you need some other ValueHolder mechanism, which can hold a value of null.

@filiphr the main reason is the a representation of Json UNDEFINED fields that in Java it's not possible without Optional or something similar (https://www.baeldung.com/jackson-optional) to implement a patch in RestApi.
I understand the core concept behind Optional in Java.
Is Guava Optional a better fit?

@lysz210 I know about Json UNDEFINED fields. You can use a null value for treating those fields as not being present. However, that in my opinion is a big anti pattern of using Optional. The blog you are referencing also mentions that:

Keep in mind that Optionals should not be used as fields and we are doing this to illustrate the problem.

A better solution for this is something like the JsonNullable from https://github.com/OpenAPITools/jackson-databind-nullable

@ro0sterjam just out of curiosity: is this going to change how we currently handle Optional. There might be may users out there which already have their own way of handling this. In other words: should we consider a switch so we switch it default off?

I could be mistaken, but from my understanding/experience with MapStruct, custom mappings should supersede any default mapping behaviour (which this is). But I can verify this claim myself. Would you be able to provide an example of how you are currently mapping Optionals (as well as expected behaviour) and I can verify if this PR changes that behaviour?

Re: null mapping

I think it's worth looking at how Jackson approaches Optional mapping, which actually seems to be configurable. This configuration configureAbsentsAsNulls by default is false.

The difference in behaviour are, if false:

While if true:

I do agree with you @filiphr that setting Optional to a null is an anti-pattern, but it still doesn't change the fact that some users do use it in this anti-pattern way, so perhaps we should just give it to them in a configurable manner as to not make life too difficult for them?

Maybe a simple NullOptionalValueMappingStrategy with the default value of MAP_TO_EMPTY and an option to set as KEEP_AS_NULL would suffice.

Where MAP_TO_EMPTY maps as the following:

And KEEP_AS_NULL does the following:

The only difference being mapping from a null Optional to another Optional. In this case, if the input is already a null Optional, then it means the developer already is using Optional in an anti-pattern way, so we might as well give them a way to keep doing what they are doing.

Anyway, just my 2 cents on the matter.

There are other Wrapper class and probably some custom classes too.
It will be very useful if the implementation is done with a StrategyPattern so that anybody can implement one without changing types.
With this we can also write one for Guava's Optional and JsonNullable.

There are other Wrapper class and probably some custom classes too.
It will be very useful if the implementation is done with a StrategyPattern so that anybody can implement one without changing types.
With this we can also write one for Guava's Optional and JsonNullable.

And JAXBElement perhaps. I've been writing custom code for years to map that one 😄 .. And here it has direct meaning. I guess that's a very interesting idea.

I finally managed to find the time to review this in detail. I really like the direction where this is going. Thanks for your work on it @ro0sterjam.

As a first step for how we treat Optional I would say that we always assume that they are not null. I would wait for initial feedback before trying to allow null for them. I've heard of the Jackson strategies. I think we can look into this as a step 2 of this functionality. Perhaps users will need to opt in into telling us to do null checks on optionals, or they would need to provide their own mapping methods for this.

Having said that, it means that we don't really need the changes in the CommonMacros.ftl. We can achieve the check by adapting PropertyMapping.PropertyMappingBuilder#getSourcePresenceCheckerRef. We can provide our own custom PresenceCheck for the optionals.

Apart from that, I would also not yet do a null check for the source parameter in the OptionalMappingMethod.ftl if the source parameter is an optional. This also makes the question you had about handling the mapNullToDefault obsolete, since we can never be in a situation like that. Although, this might mean that instead of doing optional.map we need to do optional.orElse( null ) and then do the mapping. Otherwise, empty optional won't be affected by the RETURN_DEFAULT strategy and I think it should.

Hey @filiphr thanks for the suggestions. I'm trying to incorporate them into my changes, but hitting a bit of a road block in trying to comprehend what getSourcePresenceCheckerRef is doing.

From what I can tell, it is first giving priority to any custom presence check provided from the @Mapping config for the given source property.

But then after that, I'm not entirely following what the following snippet is doing?

           SelectionParameters selectionParameters = this.selectionParameters != null ?
                this.selectionParameters.withSourceRHS( sourceRHS ) :
                SelectionParameters.forSourceRHS( sourceRHS );
            PresenceCheck presenceCheck = PresenceCheckMethodResolver.getPresenceCheck(
                method,
                selectionParameters,
                ctx
            );

Update: I think this is the, @Condition presence check, is that correct?

Finally we got the block:

            PresenceCheck sourcePresenceChecker = null;
            if ( !sourceReference.getPropertyEntries().isEmpty() ) {
                Parameter sourceParam = sourceReference.getParameter();
                // TODO is first correct here?? shouldn't it be last since the remainer is checked
                // in the forged method?
                PropertyEntry propertyEntry = sourceReference.getShallowestProperty();
                if ( propertyEntry.getPresenceChecker() != null ) {
                    List<PresenceCheck> presenceChecks = new ArrayList<>();
                    presenceChecks.add( new SuffixPresenceCheck(
                        sourceParam.getName(),
                        propertyEntry.getPresenceChecker().getPresenceCheckSuffix()
                    ) );

                    String variableName = sourceParam.getName() + "."
                        + propertyEntry.getReadAccessor().getReadValueSource();
                    for (int i = 1; i < sourceReference.getPropertyEntries().size(); i++) {
                        PropertyEntry entry = sourceReference.getPropertyEntries().get( i );
                        if (entry.getPresenceChecker() != null && entry.getReadAccessor() != null) {
                            presenceChecks.add( new NullPresenceCheck( variableName ) );
                            presenceChecks.add( new SuffixPresenceCheck(
                                variableName,
                                entry.getPresenceChecker().getPresenceCheckSuffix()
                            ) );
                            variableName = variableName + "." + entry.getReadAccessor().getSimpleName() + "()";
                        }
                        else {
                            break;
                        }
                    }

                    if ( presenceChecks.size() == 1 ) {
                        sourcePresenceChecker = presenceChecks.get( 0 );
                    }
                    else {
                        sourcePresenceChecker = new AllPresenceChecksPresenceCheck( presenceChecks );
                    }
                }
            }

From what I'm understanding, this is helping us build a nested presence checker. Which makes sense to me, but why is it only utilized when propertyEntry.getPresenceChecker() != null? I assume that if there is no explicit presence checker on the property, than the nested presence check is delegated to CommonMacros.ftl?

I am going about an approach that utilizes a new OptionalPresenceCheck (similar to NullPresenceCheck) but it's not clear where the best place to place this would be (whilst also supporting nested property checks for Optional properties in the nested chain).

Any guidance (or really just clarification on what the code snippets above are doing) would be greatly appreciated!

I checked out your branch a bit to test this out @ro0sterjam and I stumbled around the same thing. Yes the use of PresenceCheckMethodResolver is for the custom @Condition. Perhaps we would need to do the optional check in the big block you mentioned after that instead of the early placed I mentioned, the initial place I mentioned does not have the read accessor on which we need to do the isPresent().

For the null check on the source properties, I am working on some refactoring to support #2610, and I believe that we would be able to benefit from it in this PR as well.

@ro0sterjam I finally managed to do the refactoring that I promised. Was done in PR #3348 and it is now on main. Can you perhaps rebase on top of main and see if it fits better for you?

@ro0sterjam I finally managed to do the refactoring that I promised. Was done in PR #3348 and it is now on main. Can you perhaps rebase on top of main and see if it fits better for you?

Will rebase and take a look in the upcoming days, thanks!

Is there anything blocking this PR from merging? Can we help on some additional tasks, like docs?

Hey @lostiniceland sorry, work and life got the better of me and I stepped away from this project for a while. I'll pick this back up sometime this week.

There is still some code clean up in the manner that @filiphr suggested.

I would also like to support the following:

Support mapping sub properties of Optional by mapping/flat-mapping all the way down (e.g. source = "optional1.optional2.value", target = "flattenedValue")

The flattened-feature would be really nice. Take your time.
I was just wondering because this PR looked pretty complete.
And thanks for your efforts. If I can help let me know.

I'm taking a step back and going to clarify desired behaviour first, via the following tests:
https://github.com/mapstruct/mapstruct/tree/0a51de390c336b0781b758ab894506442ea4a4b7/processor/src/test/java/org/mapstruct/ap/test/optionalmapping

Specifically, some contentious behaviour that I want to finalize before continuing development.

1. https://github.com/mapstruct/mapstruct/pull/3183/files#diff-86e60d92bad08bb1f1b01e1b154fb343072292be3beab3bb635366b97c52507bR37
   This test is for Optional<SubTypeA> -> Optional<SubTypeB>.
   Currently I have the test verify that null -> Optional.empty(), which based on the conversations above is contentious as some would prefer that null -> null

2. https://github.com/mapstruct/mapstruct/pull/3183/files#diff-09991bb929eddfa1e4220121358b971bc9763ffc405973da59128062dc0534d2R37
   This test is for Optional<String> -> Optional<String>.
   Currently I have the test verify that null -> null. This is slightly different from the case above because the source and target subtypes are identical (both are String). The default behaviour with MapStruct is to map as-is without any transformation if the source type matches the target type (no sub-mapping is required in this case).

3. https://github.com/mapstruct/mapstruct/pull/3183/files#diff-7f941ebedc6115b18fc20f5c3bed432a082f54579338acc371bb283103acb88fR10
   Currently I have the test verify that when nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS, and we encounter an Optional, that we also perform an Optional.isPresent() check in addition to the default null check. Not sure if this will break existing behaviour for some users, so perhaps we should either leave this out completely (as the same thing can be achieved with @Condition today) or create a new OptionalCheckStrategy.ON/OFF config?

@filiphr re: moving the Optional check away from CommonMacros.ftl to getSourcePresenceCheckerRef.

I think this depends on the question: when do we want to perform an Optional check?

Three options and my thoughts:

1. Always

• Easiest to do; simply add to getSourcePresenceCheckerRef
• Likely quite dangerous in regards to backwards compatibility

2. Whenever setterWrapperNeedsSourceNullCheck returns true (e.g. when NullValueCheckStrategy is ALWAYS)

• Need a bit of refactoring; this check lives in CommonMacros.ftl currently, not getSourcePresenceCheckerRef
• Also possibly dangerous in regards to backwards compatibility

3. Whenever some new OptionalValueCheckStrategy is set to ALWAYS (or similar)

• Need to surface from config to PropertyEntry
• Likely the safest option

Pushed some new functionality:

Implement default return type for non optional types when RETURN_DEFAULT configured (e.g. collection types)

Achieved this by introduction of a new model InitDefaultValue which takes logic from @libs.initTargetObject (should consider consolidating and redirect usage of @libs.initTargetObject to InitDefaultValue).

Support mapping sub properties of Optional by mapping/flat-mapping all the way down

Supported by use of special ? character which signals that a orElse( null ) should be called on an Optional.
For Example: source = someObject.someOptional?.someString.
It can also be called at the end of a source: source = someObject.someOptional?
Finally, we can also map the Optional as-is without calling orElse( null ): source = someObject.someOptional

Other changes and outstanding items:

Implement Optional.isPresent() check in expected pre-mapping null checks

Removed this functionality; I believe this will be too radical a change from current behaviour, and this is already achievable with a custom @Condition for the Optional type.

Implement default Optional.empty() when NullValueCheckStrategy.ALWAYS set

Still unsure how we want to approach this; From the comments in this thread I can see that some would prefer not to have Optional.empty() forced onto them. NullValueCheckStrategy.ALWAYS would theoretically be a manner in which one could implement a "patch" request.

e.g.

if (source.getPossiblyMissingOptionalValue() != null) {
  target.setPossiblyMissingOptionalValue( apiOptionalToInternalOptional( source.getPossiblyMissingOptionalValue() ) );
}

In this case, we only set target.possiblyMissingOptionalValue if source.possiblyMissingOptionalValue != null.

That all said, the remaining "inner" mapping functions will always return Optional.empty(). I believe this is a decent compromise between the desire to always return Optional.empty() over null, as well as support for nullable Optional for the "patch" use case.

One remaining caveat is when mapping from Optional.<A>empty() to Optional.<A>empty() when the source property is null... this is because mapstruct does a direct mapping without a "submapper" (e.g. target.setOptional(source.getOptional())).

Thanks for your work on this @ro0sterjam. I've been a bit busy. I'll try to carve out some time to look into this in detail.

We're still enthusiastically waiting for this to make the cut, or NOT?... could you please give an update on what's the verdict?

We're still enthusiastically waiting for this to make the cut, or NOT?... could you please give an update on what's the verdict?

Unfortunately life has gotten in the way. I think there has been a lot of conversation here that is leading us in the right direction, but I am not able to actively work on this right now. So if anyone wants to pick this up, please be my guest. I can try to come back to it in about a month's time but I can't make any promises.

Long time overdue, I have taken this PR and reworked it a bit in PR #3971. Should be part of 1.7

@filiphr Many thanks for picking up on this issue! It was huge blocker for me.