Automatically create mapping methods to map nested properties

[Dmytro Polovinkin]

I used MapStruct for mapping two classes hierarchies, which looked the same, just were generated from different schemas and was really missing the feature described here:
https://github.com/mapstruct/mapstruct/issues/60

I tried to implement the feature myself and I'd like to discuss some items.

First of all - should it be the default behavior? I think - no, and the user should implicitly set some field in Mapper annotation to true, such as autoMapNestedClasses.

What I did in my implementation:
PropertyMappingBuilder does not print the error message when mapping method is not found, but indicates which method is missing in the field of the PropertyMapping (the only thing it needs is the target type and source type).

Then in the MapperCreationProcessor in the getMappingMethods() I check whether the BeanMappingMethod has any properties which have the missing target method and create a mock SourceMethod (not the ForgeMethod which already exists) for a missing mapping.

After that I'm adding all forged methods into the collection of SourceMethods, also add them into the sourceModel of a MappingBuilderContext and rerun the getMappingMethods().

I don't have it in my github yet (I'll deliver it soon) but I just wanted to know I was heading in the right direction. Am I? Are there any things I should be particularly aware of?

[Andreas Gudian]

Hi Dmytro,

first of all, that's great to hear!

And you already raise the most imporant of the questions first, which is the user-side of the feature. Should it be Opt-in or opt-out?

As a new user, I would expect that MapStruct generates mapping methods for nested beans automatically. I don't have to activate the automatic creation of forged iterable mapping methods either, so why should it be different with simple bean properties?

As a user migrating from a previous version of MapStruct, I would expect that the generated mapping code doesn't do anything dramatically different. In this case, users wouldn't get a different behaviour - as until now they get the error message and either add the proposed mapping method or ignore the property (which is supported by the MapStruct Eclipse plugin).

So I'd say we don't have to add a switch to enable/disable it explicitly, as all that the feature does for the user is avoid an error message (or create a different one).

But error handling is then the next point where it gets interesting. What do we do if the automatically added method can't be created without errors or warnings (e.g. unmapped target properties)? I'd say we need to report those errors exactly as they are now reported, but at the originating existing method and with more back-trace information for the path that led to the creation (e.g. customer.address.country).

On your implementation approach: I must admit that I don't have the model in my head right now, so this is more a feeling than actually knowing what I'm talking about. My guess whould have been that the implementation can be heavily oriented along the path of the forged iterable methods. The whole concept of "forged methods" should cover this use case well and we should end up with another variants of forged methods to be created.

Anyway, this sounds exciting and we're eager to see your pull request! Would be great to see this in a version 1.2 or so.

Andreas

--
You received this message because you are subscribed to the Google Groups "mapstruct-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mapstruct-users+unsubscribe@googlegroups.com.
To post to this group, send email to mapstruct-users@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Dmytro Polovinkin]

I have already pushed my changes into https://github.com/navpil/mapstruct/tree/60-nested-props-automapping

I tried to use the ForgedMethod and it seems to work. However lots of tests are broken now.

[Dmytro Polovinkin]

Please help me with the next problem.

I create ForgeMethods for all undeclared nested mappings, therefore there are some places when some method is generated even though it does no real conversion.

For example Issue590Test is now generating the mapper with my changes, though it clearly fails to convert String to XMLFormatter. And since String and XMLFormatter aren't beans, there are no error messages, such as "Unmapped target property".

How can we avoid such problems? Do we need to strictly identify which types are beans and thus conversion between them should be possible with a forged method? Maybe we should check that beans have exactly the same getters and setters to generate forged method for them? Is the warning message similar to "method mapSomethingToSomething was autogenerated" sufficient? 

On Tuesday, September 27, 2016 at 9:19:16 PM UTC+3, Dmytro Polovinkin wrote:

[Andreas Gudian]

Well, that's a tough spot you hit there.

How about this: if a target property is not assignable from the source property, we try to generate the forged method (if no other built-in or defined method exists). But for a forged method, we check that the target type has at least one property that can be mapped. Unmapped target properties will be reported as configured, but if there isn't a single property that's being mapped, then we also report an error in any case, just to ensure that it's being handled explicitly by the user.

Not sure if that's really really the right way to do it, though. You probably saw some examples now, so what do you think? Does it make any sense or is it too fuzzy?
 

[Dmytro Polovinkin]

Let's imagine that we have a switch to change strategy how the automappings are generated. Values could be:

1) EXACT_MATCH - that is all source and target properties should match. No unmapped targets, no unmapped sources. The most strict one.

2) TARGET_FILLED - we care that all target properties are mapped and we don't care that some of the source properties are left out. 

3) AT_LEAST_ONE_MATCH - the one you propose, if at least one property is mapped, then we can generate the forged methods. Unmapped properties reported as configured.

4) DONT_CARE - just create forged methods even if they do no conversions, maximum what we do - give a warning if some methods do nothing.

There may be some others. For example GRAB_ALL_FROM_SOURCE: no source properties are left unmapped to target, but target can have unmapped properties.

Now, we don't have such switch, though it can be implemented in the future if needed. If I were to choose, I'd pick one, which would be default one among all mentioned above. And the most strict one is the one users would usually want. This is also in line with the "no magic in MapStruct" philosophy.

I didn't see many real world examples, but in my case beans matched exactly and I was annoyed with the fact that MapStruct didn't generate the obvious mappings for me. This is also the case you mention in the issue#60. I think that the non-obvious mappings should be specified by the user directly.

What do you think about it?

To unsubscribe from this group and stop receiving emails from it, send an email to mapstruct-use...@googlegroups.com.
To post to this group, send email to mapstru...@googlegroups.com.

[Andreas Gudian]

Thinking about it by day, what you describe with TARGET_FILLED with handling of unmaapped target properties as configured feels more like what we should aim for. The EXACT_MATCH is not something we check for in norml bean mappings, I'd leave that out here.

But I guess in that case we're back at the problem with String vs XMLFormatter, right?

To unsubscribe from this group and stop receiving emails from it, send an email to mapstruct-users+unsubscribe@googlegroups.com.
To post to this group, send email to mapstruct-users@googlegroups.com.

[Dmytro Polovinkin]

We can agree on TARGET_FILLED but in order to avoid the String vs XMLFormatter problem we may add the check that there's at least one property in the target. So the complete rules are:

1) If there are no properties in target and no build-in conversion methods can be used, report an error.

2) If there are some properties in target, then all of them should be mapped, otherwise report an error.

This means however that we handle ForgeMethod and SourceMethod differently when some properties are unmapped. And the error/warning switch for unmapped properties (I guess there is such switch, right?) would have no effect.

I can try to implement it in the way I just mentioned - I have no checks for properties now, and anyway they need to be there at least in some form.

[Andreas Gudian]

Sounds good!

Just keep some headroom for some @Mapping(unmappedTargetPolicy=...) handling. We should discuss that in more detail on GitHub in the pull request.

[Dmytro Polovinkin]

Hello Andreas,

I'm close to be done, there are only few tests left to fix. I had to change the way some tests are written because with the new functionality in place sometimes mapper methods are generated and compilation succeeds, when it was expecting to fail.

Probably the only change from the user perspective was the change from error message 

"No implementation can be generated for this method. Found no method nor implicit conversion for mapping source element type into target element type." 

to something like 

"Can't map AttributedString to String. Consider to implement a mapping method: String map(AttributedString value)"

I hope this is ok. I tried to retain original error messages as much as possible otherwise.

I have only two tests left, where one of them is eclipse-specific as it passes for jdk. Eclipse generates classes which do not compile, whereas jdk does not generate them at all. 

The test in question is org.mapstruct.ap.test.selection.generics.ConversionTest#shouldFailOnNonMatchingWildCards(). It has two problems: first, the WildCardSuperWrapper does not follow beans convention and does not have a default constructor. If it had, the WildCardSuperWrapper<String> would be easily converted to WildCarSuperMapper<Integer> by using automap. Therefore I changed one of the mappers to WildCardSuperWrapper<TypeA> to retain the original error. The second problem is that eclipse-based mapper tries to convert the WildCardSuperWrapper<String> to the WildCardSuperWrapper<TypeA> by using the method 

private XMLGregorianCalendar stringToXmlGregorianCalendar( String date, String dateFormat )

and I'm currently looking at it.

There are two other problems I'm facing:

1) Just an annoyance, but the license check often does not pass even if it is there. I guess it is because I'm on Windows and the licence check is very strict about newlines. This makes the build process be more trivial then the simple mvn clean install

2) Integration tests fail on FullFeatureCompilationTest. The compilation fails with 

[ERROR] The import org.junit cannot be resolved

and many more for my newly created tests.

Can you give me a hint what can be the problem with the second issue?

And just in case - I will not switch to Linux (I have practical reasons for that).

[Dmytro Polovinkin]

I have already finished the code.

There are still two issues remaining and I don't know whether and how I should handle them:

1) Eclipse specific test (ConversionTest.shouldFailOnNonMatchingWildCards) is failing because for some reason the executableElement.getParameters() returns T instead of TypeA. This causes a wrong mapping to be generated. I believe this could not have been caused by my changes.

2) Integration tests are still failing on errors like 

[ERROR] import org.junit.Test;

[ERROR] ^^^^^^^^^

[ERROR] The import org.junit cannot be resolved

I really don't understand what is going on.

Should I do the pull request even having the above mentioned problems?

On Tuesday, September 27, 2016 at 9:19:16 PM UTC+3, Dmytro Polovinkin wrote:

[Andreas Gudian]

Hi,

Sorry for not responding, I've been on vacation - without any internet :).

Please go ahead and open up a PR so we can discuss it there further.

Regarding your Line-Ending issue: I'm on Windows as well, and you just need to tell your IDE to create new files with unix-line endings. It should be fine then.

Andreas