GeezerVison

Thanks a lot for the PR @ibogdanchikov. On first sight it looks OK.

I saw that you want for the build in method approach. However, I don't think that this is needed. I think that this would be more suited to work with inline conversions. And do things like Optional.ofNullable( ... ) and optional.get() for the mapping for from and to.

In addition to this, for Optional#get to be able to be used I think that we need to add some out of the box conditional method that would do the check if the optional is present.

Basically the code that is generated to look something like:

targetOptionalProperty.setToOptionalProp( Optional.ofNullable( source.getToOptionalProp() ) );
if ( source.getFromOptionalProp().isPresent() ) {
    targetOptionalProperty.setFromOptionalProp( source.getFromOptionalProp().get() );
}

This is my first time ever trying to contribute to an open source project, so I apologize in advance for anything I might have done wrong.

By the way. Congrats on your first open source contribution. We are happy to see that you chose MapStruct 😄

I saw that you want for the build in method approach. However, I don't think that this is needed. I think that this would be more suited to work with inline conversions.

I totally agree. I think this would be a much better solution. May I try and implement inline conversion instead? Would you be OK with that?

Yes of course @ibogdanchikov. Feel free to try the other approach. Let us know if you have any questions

@filiphr I've reimplemented Optional mapping using inline conversion. Could you please take a look?

thank you for taking this on! @filiphr do you mind looking over this?

I've started looking into this and we want and will have this in 1.6. One thing I want to fix is the null checks that are being done on Optional properties. We shouldn't be doing that, and I am not sure how easy it would be to achieve this.

@filiphr Could you please elaborate on how would you like to fix the null checks? I would be very grateful if you could provide trivial examples showing in what ways the generated code would look wrong and how would you like it to look. Thank you.

Sorry for taking so long time @ibogdanchikov.

What I basically would like to see is instead of:

if ( source.getFromOptionalProp() != null ) {
    target.setFromOptionalProp( source.getFromOptionalProp().orElse( null ) );
}

I would like to see

if ( source.getFromOptionalProp().isPresent() ) {
    target.setFromOptionalProp( source.getFromOptionalProp().get() );
}

or if we had some out-of-the-box presence check method for optionals:

if ( isPresent( source.getFromOptionalProp() ) ) {
    target.setFromOptionalProp( source.getFromOptionalProp().get() );
}

@filiphr Thank you very much for the examples, they were very helpful! I adjusted the implementation according to these examples.

Specifically, I added the following condition to CommonMacros.ftl:

<#if getSourceType().isOptionalType()>.isPresent()<#else> != null</#if>

and changed OptionalToObjectConversion.getToExpression to "<SOURCE>.get()", so now it generates the following code:

if ( source.getFromOptionalProp().isPresent() ) {
    target.setFromOptionalProp( source.getFromOptionalProp().get() );
}

Please let me know if this solution is acceptable. Thank you.

This looks better @ibogdanchikov. Thanks a lot. I'll get your branch and play around with it before merging it

Hi @filiphr, did you find something blocking the merge of this feature ?

Hi @filiphr, did you find something blocking the merge of this feature ?

Unfortunately, I haven't had the time I want to look better into this one. Recently there was another PR (#3183) for this functionality. So I'll need to analyze both and see what works best for MapStruct

I had some time to look into this and #3183. The approach which is taken in #3183 is more flexible and allows mapping between different types e.g. Optional<Integer> -> Optional<String>. However, in this PR that doesn't work. Therefore, I am going to close this in favour of #3183. I am really sorry for this @ibogdanchikov, I hope you understand.

Optional selectByEmail(String email) does not support constructor in spring boot myBatis query
How to fixed it>