Skip to content

Mapping from map to bean#2471

Merged
filiphr merged 1 commit intomapstruct:masterfrom
filiphr:map-to-bean
Jun 27, 2021
Merged

Mapping from map to bean#2471
filiphr merged 1 commit intomapstruct:masterfrom
filiphr:map-to-bean

Conversation

@filiphr
Copy link
Member

@filiphr filiphr commented Jun 6, 2021 •
edited
Loading

This supersedes #2317. @ckosmowski did an amazing job, I only polished it a bit and fixed the merge conflicts.

@ckosmowski let me know if you don't agree with some of my changes.

Fixes #1075

Open points:

  • Add documentation
  • Add tests for using non String map as a source parameter mapped into a bean

@filiphr filiphr mentioned this pull request Jun 6, 2021
Closed
@filiphr
Copy link
Member Author

filiphr commented Jun 6, 2021

One think that I forgot to add is the warning about raw map or a map that does not have String as a key.

Is there something in backwards compatibility way that we need to think about?

@ckosmowski
Copy link
Contributor

ckosmowski commented Jun 6, 2021

Thanks for bringing this feature further!

From what i understand about the polishing, it looks great! So i agree 👍 .
One thing that is very important for us about this feature is that we really need to input untyped maps (trusting the user that it will have only Strings as keys) because of the outputs we get from a third party library.

But if i understand it correct, in this case the message will only be a warning and does not stop the mapper from working, am i correct?

sjaakd
sjaakd approved these changes Jun 8, 2021
View reviewed changes
Copy link
Contributor

@sjaakd sjaakd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few minor's..

@sjaakd
Copy link
Contributor

sjaakd commented Jun 8, 2021

@filiphr .. I guess we do need to update the documentation as well.. right?

@filiphr
Copy link
Member Author

filiphr commented Jun 12, 2021

I guess we do need to update the documentation as well.. right?

You are right @sjaakd. I will add some documentation to this as well.

One thing that is very important for us about this feature is that we really need to input untyped maps (trusting the user that it will have only Strings as keys) because of the outputs we get from a third party library.

But if i understand it correct, in this case the message will only be a warning and does not stop the mapper from working, am i correct?

@ckosmowski yes there will be a warning. However, the mapping will not work in this case because raw maps are skipped. Your implementation was doing that already. Check:

https://github.com/mapstruct/mapstruct/pull/2471/files#diff-0e91bfe179a1bac245627d8be8971044045f147c81b308a3c3d6f822a76b4804R329-R339

I added the assertions, but the was null anyways. Only a Map<String, ???> will be treated as mapping from Map to Bean. You will have to define your mappers that they receive Map<String, ???> and pass those untyped maps to them. Your mappers will be safe and you will trust that passing a raw map into a typed map is OK.

@filiphr
Copy link
Member Author

filiphr commented Jun 12, 2021

@sjaakd, I've added some docs, added some more tests and also addressed your comments about the warning messages.

Please have a look and let me know what you think.

sjaakd
sjaakd requested changes Jun 19, 2021
View reviewed changes
Copy link
Contributor

@sjaakd sjaakd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filiphr . LGTM in general. Though the generated code doesn't look as it should be.

documentation/src/main/asciidoc/chapter-3-defining-a-mapper.asciidoc

[WARNING]
====
When a raw map or a map that does not have a String as a key is used, then a warning will be generated.
Copy link
Contributor

@sjaakd sjaakd Jun 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the key interpreted in such case.. guess we need to add that in the doc.

Copy link
Member Author

@filiphr filiphr Jun 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't, I thought that it is clear at the beginning of the section

There are situations when a mapping from a Map<String, ???> into a specific bean is needed.

@ckosmowski @filiphr
mapstruct#1075 Support for Mapping from Map<String, ???> to Bean
c8bb5c6 
Co-authored-by: Filip Hrisafov <filip.hrisafov@gmail.com>
@filiphr filiphr merged commit 985ca2f into mapstruct:master Jun 27, 2021
@filiphr filiphr deleted the map-to-bean branch June 27, 2021 05:34
@filiphr filiphr mentioned this pull request Oct 24, 2021
Open
@ckosmowski
Copy link
Contributor

ckosmowski commented Feb 23, 2022

Awesome that this change made it to the project! Thanks guys. I would love to be added to the copyright notice of the project like the other contributors if possible as a reference. Thanks in advance :-)

@filiphr
Copy link
Member Author

filiphr commented Feb 23, 2022

Thanks for letting us know @ckosmowski. You've been added to the copyright notice 0b2c7e5. I did a pass of all the commits, but it seems like I missed you. Sorry about that

@ckosmowski
Copy link
Contributor

ckosmowski commented Feb 24, 2022

Thanks, nevermind!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sjaakd sjaakd sjaakd requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support for Mapping from Map<String, ???> to Bean

3 participants

@filiphr @ckosmowski @sjaakd