Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Java 8 java.util.Optional type #674

Open
agudian opened this issue Oct 26, 2015 · 25 comments
Open

Support Java 8 java.util.Optional type #674

agudian opened this issue Oct 26, 2015 · 25 comments
Labels
Milestone

Comments

@agudian
Copy link
Member

agudian commented Oct 26, 2015

It's a little bit more than adding a built-in conversion:

  • For Optional<T> prop source property:
    • add check for prop.isPresent() in addition to the null-check (in every spot where we currently use our null-check assignment wrapper)
    • using prop.get() to fetch the value
  • For Optional<T> prop target property:
    • use Optional.empty() instead of null for null-assignments, or Optional.ofNullable(..) for other assignments.

Optionally (pun intended) also support the equivalent in Guavas com.google.common.base.Optional, which uses different names for the factory methods.

@gunnarmorling gunnarmorling added this to the 1.1-next milestone Oct 29, 2015
@gunnarmorling
Copy link
Member

add check for prop.isPresent() in addition to the null-check

Do you really think in addition, not instead? Anyone setting an Optional seems to strongly dislike their co-workers ;)

In that light, related to the discussion around the "source value presence checking" strategy, I think there "IS_NULL" should be renamed to something like "DEFAULT" to abstract from the fact whether "is null" or "isPresent()" is invoked by the default means of value presence checking.

@remmeier
Copy link
Contributor

it would be great if the request here is implemented in a more generic fashion. Meaning that the developer has to possiblity to provide an own isPresent(sourceObj, sourcePropertyName, targetObj, targetPropertyName), ignore(...) or skip(...) method to skip the mapping process for some of the properties. Some additional use cases:

  • JPA/Hibernate proxies: ignore proxies if not loaded (can kill the performance)
  • security: filter properties the user is not privileged to see

@leimer
Copy link

leimer commented Mar 15, 2018

Is there a way to enable mapstruct to handle java.util.Optionals? I'd love to provide a basic mapper config which is able to unwrap an optional and reuse it in my concrete mappers.

I have an object which contains an optional address, which contains an optional contact, which contains an emailaddress. I would like to map this object into an object which only holds the emailaddress.
In a naive way this would look like:
@Mapping(source = "address.contact.email", target = "email")
Of course this won't work :-(

Can you help me?

@leimer
Copy link

leimer commented Mar 15, 2018

I found a way by adding special getters for fields with optionals:

    private Optional<Contact> contact;

    public Contact getNullableContact() {
        return contact.orElse(null);
    }

the mapping configuration is now:
@Mapping(source = "nullableAddress.nullableContact.email", target = "email")

any better ideas?

@filiphr
Copy link
Member

filiphr commented Mar 31, 2018

@leimer the idea would be to check getContact().isPresent() for Optional or even do getContact().orElse(null) in our code when performing the mapping.

@leimer
Copy link

leimer commented Apr 3, 2018

@filiphr yes, that sounds reasonable.
What do you think of these mappings?

When source is Optional and target is not:
null -> null
empty -> null
present -> object

Both are Optional:
null -> null or empty?
empty -> empty
present -> present

Only target is Optional:
null -> null
empty -> null
present -> object

And how is this affected by the null-mapping options. Is this an easy one?

@leimer
Copy link

leimer commented Apr 3, 2018

There is a more generic workaround to handle Optional with the current implementation:

    @Mapping(source = "child", target = "kid", qualifiedByName = "unwrap")
    Target map(Source source);

    @Named("unwrap")
    default <T> T unwrap(Optional<T> optional) {
        return optional.orElse(null);
    }

The resulting mapping code is something like:
target.setKid(mapChildToKid(unwrap(source.getChild())));

@filiphr
Copy link
Member

filiphr commented Apr 3, 2018

@leimer I would say that Optional should never be null (I think it really is a code smell and you must hate your users if thats the case 😉).

So I say we need something like:

When source is Optional and target is not:
empty -> null
present -> object

Both are Optional:
empty -> empty
present -> present

Only target is Optional:
null -> empty
empty -> null - What do you actually mean by this? How can source be empty?
present -> object

And how is this affected by the null-mapping options. Is this an easy one?

For NullValueMappingStrategy I would say that when a target is Optional we always return Optional.empty(), never null. Something similar as with primitives.
As for NullValueCheckStrategy Optional should always checked by using getOptional().isPresent()

And whether it is easy, I am not sure. We already have the mapNullToDefault in a lot of places. There is also Type#getNull() which handles primitives. Do you want to give it a try?

@franzvezuli
Copy link

@leimer I like the "qualifiedByName" idea. For my needs I was curious about deep mapping. For example I need a field of an Optional object. So something like this with "qualifiedByName" I don't know if it will work:

@Mapping(source = "child.name", target = "kid", qualifiedByName = "unwrap")

@leimer
Copy link

leimer commented Apr 4, 2018

@leimer I would say that Optional should never be null (I think it really is a code smell and you must hate your users if thats the case 😉).

@filiphr I am totally with you. This is bad style. But I think the generated mapper should react properly on this case. The question is: how? I haven't used MapStruct that much right now, so I don't know how normal mappings react in that situation. But to amplify "Optional over null" practices, I think we should handle nulled Optional fields and map them to empty instead of null.

Only target is Optional:
null -> empty
empty -> null - What do you actually mean by this? How can source be empty?
present -> object

oops, I did a copy&paste mistake. Of course there is no empty when the source is no Optional. Sorry for the confusion. And present -> object is wrong too. The mapping should be like
Only target is Optional:
null -> empty
object -> present

Do you want to give it a try?

Mhm, not sure if I have the time in the next weeks (in two days I will start to renovate my house). But despite that: do you think this is a "good-first-ticket"? I think another ticket would be to adapt the IDE plugins to take Optional fields into account.

@leimer
Copy link

leimer commented Apr 4, 2018

@franzvezuli I don't think this gonna work.

    @Mapping(source = "child", target = "kid", qualifiedByName = "unwrap")
    @Mapping(source = "child.name", target = "nameOfKid", qualifiedByName = "unwrap")
    Target map(Source source);

    @Mapping(source = "name", target = "nameOfKid")
    Kid mapChild(Child child);

When I try to compile this, I get the following error:
Error:(15, 12) java: No property named "child.name" exists in source parameter(s). Did you mean "child.present"?

I think, if you would like to do this type of mapping you would have to use my first workaround, by defining extra Java-Bean-Properties using additional getters/setters

@filiphr
Copy link
Member

filiphr commented Apr 4, 2018

Handling Optional null fields would certainly make it more complex, as it won't be trivial like at this moment. We could give it a go, but I really thing it would be really complex to implement it (both null and not null Optional.

Thanks for the clarification about only target being Optional. And yes your last example is great.

Only target is Optional:
null -> empty
object -> present

But despite that: do you think this is a "good-first-ticket"?

It really is up to you. I think it is not that difficult to add this. We can always guide you along the way. You can start small first by creating some tests for the feature and then go from it. In any case it is completely up to you. We are always willing to accept new contributors and help them in the project 😄

@jengelhart0
Copy link

Any plan to push this forward? Having support for Optionals would be really useful

@filiphr
Copy link
Member

filiphr commented Nov 17, 2019

@cookieMr what are you trying to imply with your comment? This is an Open Source Project and anyone can start working on this issue and help us out in implementing this feature.

We have already given some pointers and we are always ready to help out. The entire team works on this project in their spare time and currently we are focused on some other issues.

@cookieMr
Copy link

@filiphr you are right, i am sorry, i removed my comment, keep up the good work and ignore me

@lt-schmidt-jr
Copy link

I would like to contribute this feature, looking for some guidance

@phmaes
Copy link

phmaes commented Dec 3, 2020

Hi,
today I had to switch to 1.4.1 due to a bug in intellij https://youtrack.jetbrains.com/issue/IDEA-200481 .
I'm now facing another problem with the workaround concerning the way Optional is handled in mapstruct.
This
@Named("unwrap") public <T> T unwrap(Optional<T> optional) { return optional.orElse(null); }
doesn't work anymore...

It gives me this error
error: Qualifier error. No method found annotated with @nAmed#value: [ unwrap ]. See https://mapstruct.org/faq/#qualifier for more info.

Is it a regression? or intended? is there another workaround as it seems this feature isn't available.

@filiphr
Copy link
Member

filiphr commented Dec 3, 2020

@phmaes the error you are getting has nothing to do with this issue here. Please do not post the same comments twice. Either create a new issue or leave a comment

@michalkowol
Copy link

@filiphr I would love to contribute to this project 🙂 Could you please let me know if Optional should be done similar to Iterable?

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

@filiphr
Copy link
Member

filiphr commented Dec 11, 2020

@michalkowol glad that you would like to contribute. I would say that the optional should be done more like the JAXBElement. Have a look at JaxbElemToValue

benblasberg added a commit to benblasberg/mapstruct that referenced this issue Feb 21, 2021
benblasberg added a commit to benblasberg/mapstruct that referenced this issue Feb 21, 2021
benblasberg added a commit to benblasberg/mapstruct that referenced this issue Feb 21, 2021
benblasberg added a commit to benblasberg/mapstruct that referenced this issue Feb 21, 2021
benblasberg added a commit to benblasberg/mapstruct that referenced this issue Feb 21, 2021
@filiphr filiphr modified the milestones: Future planning, 1.6.0 Jan 25, 2022
ibogdanchikov added a commit to ibogdanchikov/mapstruct that referenced this issue Jun 6, 2022
ibogdanchikov added a commit to ibogdanchikov/mapstruct that referenced this issue Jun 6, 2022
ibogdanchikov added a commit to ibogdanchikov/mapstruct that referenced this issue Jun 6, 2022
ibogdanchikov added a commit to ibogdanchikov/mapstruct that referenced this issue Jun 7, 2022
ibogdanchikov added a commit to ibogdanchikov/mapstruct that referenced this issue Aug 27, 2022
ro0sterjam added a commit to ro0sterjam/mapstruct that referenced this issue Mar 2, 2023
ro0sterjam added a commit to ro0sterjam/mapstruct that referenced this issue Mar 2, 2023
@ro0sterjam
Copy link
Contributor

Hey @filiphr, throwing in my contender for this feature into the ring #3183

My fix takes the approach that @michalkowol mentioned, as it personally felt like the more natural place for it to go. I didn't really get too far when looking into JAXBElement, but can take a second shot at that too if you prefer.

@filiphr
Copy link
Member

filiphr commented Apr 15, 2023

Thanks for your contender @ro0sterjam. I'll need some more time to see the differences between yours (PR #3183) and the one from @ibogdanchikov (PR #2874).

I appologize in advance that this will take a bit more time

ro0sterjam added a commit to ro0sterjam/mapstruct that referenced this issue Oct 20, 2023
@filiphr filiphr modified the milestones: 1.6.0.Beta1, 1.6.0.Beta2 Nov 4, 2023
ro0sterjam added a commit to ro0sterjam/mapstruct that referenced this issue Nov 21, 2023
@caleb-cushing0-kr
Copy link

and then 20 years went by...

@mvsoder
Copy link

mvsoder commented Jan 3, 2024

The Java Optional class also has a map() method that is useful for this purpose. The map method takes a "functional interface" Function method and the StructMap mapper method can be used. Example:

Optional<UserEntity> optional = repo.findById( id );
Optional<User> user = optional.map( UserMapperImpl::map );

It takes the burden of unpacking and repacking the optional away from MapStruct.

@filiphr filiphr modified the milestones: 1.6.0.Beta2, 1.6.0.RC1 May 1, 2024
@chebureki
Copy link

and then 20 years went by...

still waiting :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests