#1011 updates are not handled correctly for nested target mappings #1046
Conversation
|
@filiphr I added you as initial reviewer since it reflects your idea. @agudian , @gunnarmorling : by all means, if you want to review as well go ahead. |
|
One thing I realized myself: the Forge method should delegate its equals to its include |
There was a problem hiding this comment.
Overall it looks really great.
Some general stylistic comments:
- I would also add some Javadoc to the different
popXmethods. They are well explained in theMappingOptions, but it would be nice if we have it per class where it is used. - The order of some imports is strange and some formatting issues.
This only works for nested target properties, and not source properties right? I think that it should work for mappers like: org.mapstruct.ap.test.nestedproperties.simple.SimpleMapper, org.mapstruct.ap.test.nestedsourceproperties.ArtistToChartEntry, org.mapstruct.ap.test.nestedsourceproperties.ArtistToChartEntryComposedReverse. Maybe this needs another PR, I don't know?
The reason why I did "Request changes" is because I think that there is a problem when we want to do normal mapping with nested target properties like in org.mapstruct.ap.test.nestedtargetproperties.ChartEntryToArtist#map(ChartEntry chartEntry). In the implementation method chartEntryToStudio(ChartEntry chartEntry, Studio mappingTarget) we have some null checks. I think that those checks there are wrong. Maybe we need to have one type of generated methods for update methods (with @MappingTarget) and one for normal mappings (with a return type).
| */ | ||
| public Map<PropertyEntry, MappingOptions> popTargetReferences() { | ||
|
|
||
| // sort all mappings based on the top level name before popping |
There was a problem hiding this comment.
Don't you mean group here? Also for the other sort comments.
There was a problem hiding this comment.
yep.. I'll change this into group.
There was a problem hiding this comment.
well, its actually also a kind of pop (like pop from a stack). groupByPoppedPropertyEntry good enough?
There was a problem hiding this comment.
The name of the method is completely fine. What I meant are the comments sort all mappings based on the top level name before popping. What you mean (I think) is group all mappings by the top level name before popping
| public boolean hasNestedTargetReferences() { | ||
| boolean found = false; | ||
| Iterator<List<Mapping>> mapIterator = mappings.values().iterator(); | ||
| while ( mapIterator.hasNext() && !found ) { |
There was a problem hiding this comment.
Why don't you use 2 for iterators like in the other methods?
There was a problem hiding this comment.
I'm not so fond of breaking a loop. The found condition breaks the loop. Now it is in the loop check.
There was a problem hiding this comment.
Fine by me. I just needed some more time to understand the loops, nothing else 😄
There was a problem hiding this comment.
I have to say, I had to read that method for a while as well to understand that it's just a two step a iteration...
public boolean hasNestedTargetReferences() {
for (List<Mapping> mappings : mappings.values()) {
for (Mapping mapping : mappings) {
TargetReference targetReference = mapping.getTargetReference();
if (targetReference.isValid() && targetReference.getPropertyEntries().size() > 1) {
return true;
}
}
}
return false;
}
... we could then much easier transform it into a chain of stream / lambdas when we're full on Java 8 at some point... 😁
| return result; | ||
| } | ||
|
|
||
| private Map<String, List<Mapping>> sortByTargetName( List<Mapping> mappingList ) { |
There was a problem hiding this comment.
I think it should be groupByTargetName. At least that is what I get from the method implementation 😄
There was a problem hiding this comment.
that's right.. I'll change.
| return result; | ||
| } | ||
|
|
||
| public void reInit( Parameter sourceParameter ) { |
There was a problem hiding this comment.
Maybe a small Javadoc what this exactly means, also for the Mapping#reInit
There was a problem hiding this comment.
we place the Mappings on top of a new method. So we need to associate those with the new source parameter in this method. There can be only one source parameter in generated forged methods... Something along those lines?
There was a problem hiding this comment.
Yes something along those lines
There was a problem hiding this comment.
Without having read all the code, it sounds like we could also just create fresh instance for the new usage, eg with a properly named factory method? Then we wouldn't have to make sure that we don't use the instance for the previous method after it has been reInit-ed for the next method?
There was a problem hiding this comment.
@agudian: it originates form init which happens after the construction of the Mapping as well. Point is: I already have a MappingOption. Just freshly created (but an another spot). I don't want to drag around the mappings themselves (I see a need for having the other mappings as well). So what about plain init?
There was a problem hiding this comment.
Sounds good, init wouldn't confuse me here... 🙂
| @@ -115,4 +107,39 @@ public String getFullName() { | |||
| return Strings.join( Arrays.asList( fullName ), "." ); | |||
| } | |||
|
|
|||
| public PropertyEntry pop() { | |||
There was a problem hiding this comment.
Maybe a Javadoc to explain that a new PropertyEntry is created and how.
| return errorOccured; | ||
| } | ||
|
|
||
| private boolean reportErrorOnTargetObject(Mapping mapping) { |
There was a problem hiding this comment.
This won't show the entire nested path if some of the middle nested properties is abstract class / interface, right? Maybe we need to add a test for this situation 😄
There was a problem hiding this comment.
Lets keep this separated. I'll add an issue.
|
|
||
| ForgedMethod forgedMethod = (ForgedMethod) this.method; | ||
| ForgedMethod forgedMethod = (ForgedMethod) this.method; | ||
| if ( forgedMethod.isAutoMapping() ) { |
There was a problem hiding this comment.
Why don't you add this if to the one bellow and have if ( forgedMethod.isAutoMapping() && ( targetProperties.isEmpty() || !unprocessedTargetProperties.isEmpty() ) ) instead?
| @@ -37,6 +38,7 @@ | |||
| @RunWith(AnnotationProcessorTestRunner.class) | |||
| @WithClasses(ErroneousIssue1029Mapper.class) | |||
| @IssueKey("1029") | |||
| @WithSingleCompiler(org.mapstruct.ap.testutil.runner.Compiler.JDK) | |||
There was a problem hiding this comment.
What is the problem with the Eclipse compiler? On my computer there was no problem when running for both compilers.
There was a problem hiding this comment.
none.. its a left over of testing.. I'll remove that.. Good point.
|
I know we mentioned it once, but a lot of the new implementation is begging for Java 8 😄 |
you mean the combination: in one mapping nested source / nested target..? I've got no reason to believe it does not work.. I don't touch the |
I know.. But I know that a lot of companies are more conservative in their java update plan. Its only 5 years ago a rolled out a platform on Java5.. |
I think they originate from: @Mapper( nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS )
public abstract class ChartEntryToArtist {
....That is what I meant with I probably have to update the documentation 😄. I added those to make the composite test case work out of the box (it assumes that the first argument changes are not override by the second argument changes). Makes sense? |
I know that we forge methods, I just thought that we are going to group those methods as well. Now for the
Yes it makes sense if all the fields are |
Ok.. So what you mean is that there are too many source mappings generated than is strictly needed. I think this is indeed a new issue. But we should be careful here. Introduction of grouping here could lead to a can of worms (think about dependsOn stuff) etc. So I'm actually quite happy with the current implementation. |
Indeed it is a new issue. I created #1048 for this reason. |
Hmm. I agreed it an issue for the migration notes. But I do think we did not think on this composite scenario properly in the past: Having said this: which arg So, perhaps I should add a testcase to check whether the possibility to control the order is indeed kept in-tact by my scribbling and adding a |
|
I think that the test
The thing is that there is no question @Mapper( nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS )
public abstract class ChartEntryToArtist {
@Mappings({
@Mapping(target = "type", ignore = true),
@Mapping(target = "name", source = "chartEntry2.chartName"),
@Mapping(target = "song.title", source = "chartEntry1.songTitle" ),
@Mapping(target = "song.artist.name", source = "chartEntry1.artistName" ),
@Mapping(target = "song.artist.label.studio.name", source = "chartEntry1.recordedAt"),
@Mapping(target = "song.artist.label.studio.city", source = "chartEntry1.city" ),
@Mapping(target = "song.positions", source = "chartEntry2.position" )
})
public abstract Chart map(ChartEntry chartEntry1, ChartEntry chartEntry2);
//There are other mappings as well
}As you can see there is clear distinction which property is taken from which argument in the Adding a testcase with
I couldn't find anything about nested properties being experimental in our documentation. If they are I agree with you we can keep them, until we gain more experience. However, I don't think that we need to use |
| public Map<Parameter, MappingOptions> splitOnSourceParameterType() { | ||
|
|
||
| Set<Parameter> parameters = new HashSet<Parameter>(); | ||
| Map<Type, List<Mapping>> mappingsKeyedByParameterType = new HashMap<Type, List<Mapping>>(); |
There was a problem hiding this comment.
If we do this, then if 2 parameters have the same type like in the ChartEntryToArtist then we are going to return the same MappingOptions for chartEntry1 and chartEntry2. Is this expected?
Of course.. you are right.. I need to rethink. Perhaps we should generate also methods having more source parameters (as long as that is relevant). WDYT? |
I thought I noticed it somewhere.. |
That's an interesting idea. However, I don't think that that is the problem. Have a look at Additionally see my comment on Edit: The line for the property mapping was wrong. |
I initially did so (had |
The |
If we want to achieve re-use (and avoid conflicts) we need to. Just as implementing the equals and hashcode operations to make a proper distinction. And they are for forged Map, Iterable and Stream mappings, right? I'll limit myself to my case.. |
I just had some idea 😄. Maybe the name
@sjaakd if you want I can create a new issue for this and do it in a separate PR, so we keep things more contained 😄 |
Using wildcards like this is something interesting. I only think that @gunnarmorling is not a big fan of wildcards 😄 |
I know.. But we need to away to stop auto-mapping, but also name-based mapping. I now write entire mappers with |
No problem at all.. |
|
Okay, I think I get the gist of what the first and second commit are about to do and I'd only have some minor comments there (I'm going over it later). The last commit "exclude other mappings (name and parameter base) from nesting" puzzles me a bit more... 😉. The test yields the same result with and without the code changes in the processor of that commit: in both cases, the test is green, but it shows that something is not right here, as you've written in the comment. Without knowing too much about MapStruct, I would probably expect that More explicitly, I would expect that the following two declarations would create the same results: @Mapping(target = "fish.kind", source = "fish.type")
FishTankDto toFishTankDto(FishTank source);is basically just a short-cut to this: FishTankDto toFishTankDto(FishTank source);
@Mapping(target = "kind", source = "type")
FishDto toFishDto(Fish fish);In the first example, don't we pass on the mapping options from Without thinking too much about it, I would guess that it's what a user would expect as well. We usually require users to only describe the exceptions and let the rest be handled implicitly. With the auto-mapping out, I actually expect that users will use the path expressions in The behaviour of the previous combination of nested source and nested target is a bit odd and I don't think that users really have a need for exactly that....Because what if I want to stop mapping at a certain branch? I'd just ignore that one, e.g. That's mostly the reason why I finally worked on the nested-property code completion for |
| public boolean hasNestedTargetReferences() { | ||
| boolean found = false; | ||
| Iterator<List<Mapping>> mapIterator = mappings.values().iterator(); | ||
| while ( mapIterator.hasNext() && !found ) { |
There was a problem hiding this comment.
I have to say, I had to read that method for a while as well to understand that it's just a two step a iteration...
public boolean hasNestedTargetReferences() {
for (List<Mapping> mappings : mappings.values()) {
for (Mapping mapping : mappings) {
TargetReference targetReference = mapping.getTargetReference();
if (targetReference.isValid() && targetReference.getPropertyEntries().size() > 1) {
return true;
}
}
}
return false;
}
... we could then much easier transform it into a chain of stream / lambdas when we're full on Java 8 at some point... 😁
| return nestedDependsOn; | ||
| } | ||
|
|
||
|
|
| @@ -115,4 +107,39 @@ public String getFullName() { | |||
| return Strings.join( Arrays.asList( fullName ), "." ); | |||
| } | |||
|
|
|||
| public PropertyEntry pop() { | |||
| if ( fullName.length > 1 ) { | |||
| String[] newFullName = Arrays.copyOfRange( fullName, 1, fullName.length ); | |||
There was a problem hiding this comment.
Why didn't we make fullName a List<String>? We could have used List.subList(...) here instead then, avoiding the array-copy,
| } | ||
|
|
||
| @Test | ||
| @WithClasses({FishTankMapper.class}) |
There was a problem hiding this comment.
You know my CPU isn't that powerful, so moving all mappers to the top of the class would really help... For debugging purposes, I usually remove those mappers that I'm not interested in from the WithClasses... 😄
There was a problem hiding this comment.
I know.. But I'm not doing it to tease you 😄. The reason is indeed debugging (and my sloppiness: I forget to move stuff back).. But I can of course do that.
|
@agudian thanks for your comments. I'll fix those (won't change the array at this point in time though, but we can do this later.
I can agree with that. But stil, we introduce a backward incompatibility. And we should be cautious with those. At lest the (co)issuer of #1011 had an issue here, hence my latest commit. My biggest concern is that we end up in ignoring a lot of nested target properties. That's why I propose to have a shorthand: @Mapper
public interface FishTankMapper {
FishTankMapper INSTANCE = Mappers.getMapper( FishTankMapper.class );
@Mapping(target = "fish.kind", source = "fish.type")
@Mapping(target = "fish.*", ignore=true) // will ignore all others
FishTankDto map( FishTank source );
}So, what I can do is remove the latest commit from this PR, extend on it by the above wild card idea and move it to a new PR for stopping auto-mapping. Just as we discussed in #1001. Would this PR than be fit to be merged? We can continue the discussion on
Ok.. I'm not so familiar with Eclipse here, hence I did not look at it, I can give it a try. But Not everyone is relying on Eclipse for solving the problem above. So I'm still in favor of some kind of shorthand. |
And add 'normal' (non wildcard) ignore on nesting levels to this PR (reuse the fishtanks test for that). |
|
Agreed, let's leave out the last part and discuss the potential need for a wildcard ignore or anything like that in a separate issue. |
|
@agudian .. Solved most of your comments (not the array though, I've got a stash for that, but it still fails and I need to investigate why). I myself did not see the problem correctly with was found in #1011 by @snowe2010 after giving him a solution only containing the first 2 commits. The problem is in the code that is generated: protected void fishTankToFishDto(FishTank fishTank, FishDto mappingTarget) {
if ( fishTank == null ) {
return;
}
String type = fishTankFishType( fishTank );
if ( type != null ) {
mappingTarget.setKind( type );
}
}the current solution stops mapping a property The composite test shows that there are 2 mapping methods generated for each source property. @filiphr and myself discussed this above. However, auto-mapping currently is not applied to already handled targets on root nesting level. I reckon we need to address that as well. It should also be handled in another separate mapping method ignoring the properties already mapped. I need to think about this some more. I'll open up a separate issue / PR for that one as well. This works as it used to, solving the issue described in #1011. |
|
The reproducer in 23fd42a actually does its job now. |
|
Wow, what a debate :) Where are we with it, any further discussion needed? I guess something for tonight :) |
Not particularly on this one.. But this one got me thinking about how to handle ignore's in automapping. I send 2 mails on that one (off line). But I think we can also put that in a separate issue. The only think stopping me from merging is @agudian 's verdict on the commit: 23fd42a and 3f84707 and the merge of #1053 . I've got a rebase locally standing by. |
|
I can also add a fixture for this so we can immediately pick up when the generated source changes 😄 |
guess I was watching a very intelligent program 😄 with an half eye called "Tanked" on the Discovery Channel |
|
@gunnarmorling : same here.. If you're ok I'll continue and merge. |
@agudian : Let me know were you think the problems are. So we can take that as input for the discussion. |
|
I'll go ahead and merge. |
Indeed something looks weird there. Why does it generate a
|
|
I've opened #1060 for this. It's definitely something we'll need to look into. |
Nested mappings were never limited to a symmetrical tree. It works exactly as before.
Checkout: The generated code works exactly as before (even before the auto-mapping). I only added the For more info, see #1057 for more info. |
This PR makes use of the new forged bean mapping (part of automapping) functionality. The commits need to be squashed but I left it intentionally for the time being. The idea is to make mapping options and in particular the mappings themselves, also be used by a forged method.
Its a bit rough around the edges but I wanted to share the solution for initial review.
As a reminder to myself: the documentation needs to be update to reflect the best approach for update method (nullvaluecheck strategy: always). Furthermore, I need to add some notes to the release, there is some (logical) changed behavior.