#674 Support for native Java 8 Optional mapping #3183

ro0sterjam · 2023-03-02T06:23:50Z

First draft at native mapping support for Java 8 Optional types. I intend to clean up some logic, especially around the template once provided some preliminary feedback.

Models Optional as a unique case of ContainerMappingMethod.

Source Type	Target Type	Source Value	Target Value
Optional	Object	Present	Value
Optional	Object	Empty	null
Optional	Object	null	null
Object	Optional	Value	Present
Object	Optional	null	Empty
Optional	Optional	Present	Present
Optional	Optional	Empty	Empty
Optional	Optional	null	Empty

Features (WIP):

Implement base mappings as per table above
Support mapping of contained element (i.e. (A) Optional<TypeA> -> Optional<TypeB>, (B) Optional<TypeA> -> TypeB, and (C) TypeA -> Optional<TypeB>)
Implement support for @BeforeMapping and @AfterMapping
~~[ ] Implement Optional.isPresent() check in expected pre-mapping null checks~~
Implement default return type for non optional types when RETURN_DEFAULT configured (e.g. collection types)
~~[ ] Implement default Optional.empty() when NullValueCheckStrategy.ALWAYS set~~
Implement default Optional.empty() when NullValuePropertyMappingStrategy.SET_TO_DEFAULT set
Javadoc
Updating documentation
Support mapping sub properties of Optional by mapping/flat-mapping all the way down (e.g. source = "optional1.optional2.value", target = "flattenedValue")

ro0sterjam · 2023-03-02T06:24:26Z

processor/src/test/java/org/mapstruct/ap/testutil/assertions/JavaFileAssert.java

@@ -33,7 +33,7 @@ public class JavaFileAssert extends FileAssert {

    private static final String FIRST_LINE_LICENSE_REGEX = ".*Copyright MapStruct Authors.*";
    private static final String GENERATED_DATE_REGEX = "\\s+date = " +
-        "\"\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\+\\d{4}\",";
+        "\"\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}[+-]\\d{4}\",";


Fixing up some timezone bias :P

ro0sterjam · 2023-03-02T06:29:47Z

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

+        private Assignment forgeOptionalMapping(Type sourceType, Type targetType, SourceRHS source) {
+
+            targetType = targetType.withoutBounds();
+            ForgedMethod methodRef = prepareForgedMethod( sourceType, targetType, source, "?" );


Using ? here to represent an Optional type

I need to see how this will look like in the error. I think that it would be something like customer?.address?.value if customer and address are optional. I think that looks nice. Perhaps an example as a comment here to make it easy to understand 😄

I have to admit, I'm not certain how to trigger an error that will print out this forge history. Would you have any guidance on the simplest way to do so? Once I can see what it looks like, I can add the comment.

ro0sterjam · 2023-03-02T06:33:18Z

processor/src/main/resources/org/mapstruct/ap/internal/model/OptionalMappingMethod.ftl

+                <#-- TODO: this should return the default value for whatever the returnType is -->
+                return null;


Not sure how to return the "default" type for when the resultType is not Optional (e.g. when it is a collection). Preferably mapping Optional<Collection> == null => Empty Collection (though I do recognize that no one should be passing a null to an Optional, but we should prob still handle this case regardless.

Also Optional.<Collection>empty() should probably also map to Empty Collection when mapNullToDefault is true, but that logic too is missing below.

ro0sterjam · 2023-03-02T06:33:56Z

processor/src/main/resources/org/mapstruct/ap/internal/model/OptionalMappingMethod.ftl

+    }
+
+    <#if sourceParameter.type.optionalType>
+        return ${sourceParameter.name}.map( ${loopVariableName} -> <@includeModel object=elementAssignment/> )<#if !returnType.optionalType>.orElse( null )</#if>;


Permission to rename loopVariableName to containedElementName?

robotmrv · 2023-03-02T09:19:41Z

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)

ro0sterjam · 2023-03-02T14:43:29Z

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

sjaakd · 2023-04-23T08:58:03Z

@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?

lysz210 · 2023-04-23T09:27:41Z

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?

filiphr · 2023-04-23T16:44:55Z

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.

lysz210 · 2023-04-23T17:02:55Z

@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?

filiphr · 2023-04-23T18:09:27Z

@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 · 2023-04-25T13:40:58Z

@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?

ro0sterjam · 2023-04-25T14:08:26Z

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:

Source Value	Target Value
Value	Present
null	Empty
Absent	Empty

While if true:

Source Value	Target Value
Value	Present
null	Empty
Absent	null

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:

Source Type	Target Type	Source Value	Target Value
Object	Optional	Value	Present
Object	Optional	null	Empty
Optional	Optional	Present	Present
Optional	Optional	Empty	Empty
Optional	Optional	null	Empty

And KEEP_AS_NULL does the following:

Source Type	Target Type	Source Value	Target Value
Object	Optional	Value	Present
Object	Optional	null	Empty
Optional	Optional	Present	Present
Optional	Optional	Empty	Empty
Optional	Optional	null	null

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.

lysz210 · 2023-04-28T06:10:39Z

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.

sjaakd · 2023-04-28T07:30:43Z

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.

filiphr

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.

filiphr · 2023-05-01T10:03:03Z

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

+        private Assignment forgeOptionalMapping(Type sourceType, Type targetType, SourceRHS source) {
+
+            targetType = targetType.withoutBounds();
+            ForgedMethod methodRef = prepareForgedMethod( sourceType, targetType, source, "?" );


I need to see how this will look like in the error. I think that it would be something like customer?.address?.value if customer and address are optional. I think that looks nice. Perhaps an example as a comment here to make it easy to understand 😄

ro0sterjam · 2023-07-09T17:43:18Z

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!

filiphr · 2023-07-10T05:44:32Z

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.

filiphr · 2023-08-01T13:42:29Z

@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 · 2023-08-01T17:57:07Z

@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!

lostiniceland · 2023-10-16T09:44:55Z

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

ro0sterjam · 2023-10-16T14:14:16Z

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")

lostiniceland · 2023-10-16T14:36:38Z

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.

ro0sterjam · 2023-10-18T03:37:08Z

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.

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
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).
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?

ro0sterjam · 2023-10-20T02:18:25Z

@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:

Always

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

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

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

Need to surface from config to PropertyEntry
Likely the safest option

ro0sterjam · 2023-11-21T04:02:47Z

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())).

ro0sterjam · 2023-11-21T04:17:41Z

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java

@@ -680,6 +690,15 @@ public ReadAccessor getReadAccessor(String propertyName, boolean allowedMapToBea
                .orElse( null );
            return new MapValueAccessor( getMethod, typeParameters.get( 1 ).getTypeMirror(), propertyName );
        }
+        else if ( propertyName.equals( "?" ) && isOptionalType() ) {


I might consider just throwing a error here if we encounter a ? but it is not an Optional type.

ro0sterjam · 2023-11-21T04:24:10Z

processor/src/main/java/org/mapstruct/ap/internal/model/common/Type.java

+    public boolean hasAccessibleDefaultConstructor() {
+        if ( hasAccessibleDefaultConstructor == null ) {
+            hasAccessibleDefaultConstructor = false;
+            List<ExecutableElement> constructors = ElementFilter.constructorsIn( typeElement.getEnclosedElements() );
+            for ( ExecutableElement constructor : constructors ) {
+                if ( constructor.isDefault() && !constructor.getModifiers().contains( Modifier.PRIVATE ) ) {
+                    hasAccessibleDefaultConstructor = true;
+                    break;
+                }
+            }
+        }
+        return hasAccessibleDefaultConstructor;
+    }
+


This is related to an existing strange behaviour with CommonMacros#initTargetObject

mapstruct/processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl

Line 173 in c9b551a

new <@includeModel object=ext.targetType/>()

In that code, it is blindly calling new X() even if X does not have a default constructor. This is even called out in NullValuePropertyMappingStrategy, but in this case since this is a nested object, we cannot work around it with a defaultValue().

Thus this method was added so that null can be returned when a "default value" cannot be constructed.

I'm considering just removing this logic and keeping it simple (i.e. let the user fix this issue by writing a custom mapper or create a default constructor).

ro0sterjam · 2023-11-21T04:30:13Z

processor/src/main/resources/org/mapstruct/ap/internal/model/NestedPropertyMappingMethod.ftl

@@ -21,7 +21,11 @@
    <#if !entry.presenceChecker?? >
    <#if !entry.type.primitive>
    if ( ${entry.name} == null ) {
-        return ${returnType.null};
+        <#if returnType.isOptionalType()>
+            return Optional.empty();


Should probably use InitDefaultValue here instead of hardcoding Optional.empty(), but this ModelElement is missing the required factoryMethod.

filiphr · 2023-11-26T21:06:40Z

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.

olonge-coursera · 2024-05-03T21:59:07Z

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

ro0sterjam · 2024-05-04T15:14:25Z

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.

ro0sterjam commented Mar 2, 2023

View reviewed changes

ro0sterjam mentioned this pull request Mar 2, 2023

Support Java 8 java.util.Optional type #674

Open

ro0sterjam commented Mar 2, 2023

View reviewed changes

ro0sterjam force-pushed the rj/support-for-optionals branch from 0e3a5f4 to 0d2f599 Compare March 2, 2023 06:48

filiphr mentioned this pull request Apr 22, 2023

#674 Support Java 8 java.util.Optional type #2874

Closed

filiphr reviewed May 1, 2023

View reviewed changes

filiphr mentioned this pull request Aug 1, 2023

Refactor to use presence check for mapping methods #3347

Closed

ro0sterjam force-pushed the rj/support-for-optionals branch from 0a51de3 to 290023d Compare October 20, 2023 00:41

ro0sterjam added 15 commits November 19, 2023 16:59

mapstruct#674 Support for native Java 8 Optional mapping

9cf813b

fix formatting

71c87b3

Default to `Optional.empty() when null checking

2dd531b

generated tests

966c4ad

optional check alongside null check

9309c38

public field tests

35fd4aa

Before/After mapping

2f1cb00

Tests to clarify behaviour

49cd50c

negate support

129b07d

map to default for all types

c9b551a

more test

0e28324

nested optional support

262b608

formatting

9e969e7

generated sources

c7e7ed9

formatting

56cf074

ro0sterjam force-pushed the rj/support-for-optionals branch from 290023d to 56cf074 Compare November 21, 2023 03:29

always return Optional.empty() for inner maps

563afa3

ro0sterjam commented Nov 21, 2023

View reviewed changes

javadoc

087ddc8

filiphr mentioned this pull request Nov 26, 2023

map generics #2377

Open

		<#-- TODO: this should return the default value for whatever the returnType is -->
		return null;

#674 Support for native Java 8 Optional mapping #3183

Are you sure you want to change the base?

#674 Support for native Java 8 Optional mapping #3183

Conversation

ro0sterjam commented Mar 2, 2023 • edited

ro0sterjam Mar 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robotmrv commented Mar 2, 2023

ro0sterjam commented Mar 2, 2023 • edited

sjaakd commented Apr 23, 2023

lysz210 commented Apr 23, 2023

filiphr commented Apr 23, 2023 • edited

lysz210 commented Apr 23, 2023

filiphr commented Apr 23, 2023

ro0sterjam commented Apr 25, 2023

ro0sterjam commented Apr 25, 2023

lysz210 commented Apr 28, 2023

sjaakd commented Apr 28, 2023

filiphr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0sterjam commented Jul 9, 2023 • edited

filiphr commented Jul 10, 2023

filiphr commented Aug 1, 2023

ro0sterjam commented Aug 1, 2023

lostiniceland commented Oct 16, 2023

ro0sterjam commented Oct 16, 2023

lostiniceland commented Oct 16, 2023

ro0sterjam commented Oct 18, 2023 • edited

ro0sterjam commented Oct 20, 2023 • edited

ro0sterjam commented Nov 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filiphr commented Nov 26, 2023

olonge-coursera commented May 3, 2024 • edited

ro0sterjam commented May 4, 2024

ro0sterjam commented Mar 2, 2023 •

edited

ro0sterjam Mar 2, 2023 •

edited

ro0sterjam commented Mar 2, 2023 •

edited

filiphr commented Apr 23, 2023 •

edited

ro0sterjam commented Jul 9, 2023 •

edited

ro0sterjam commented Oct 18, 2023 •

edited

ro0sterjam commented Oct 20, 2023 •

edited

ro0sterjam commented Nov 21, 2023 •

edited

olonge-coursera commented May 3, 2024 •

edited