#1269 use update methods for different sources for nested targets #1272
Conversation
|
@sjaakd I've added you to this review as we have been working together on this part 😄 |
I'll do a review asap. 😄 |
| @@ -180,9 +180,15 @@ public NestedTargetPropertyMappingHolder build() { | |||
| .groupedBySourceReferences | |||
| .entrySet() ) { | |||
| PropertyEntry sourceEntry = entryBySP.getKey(); | |||
| boolean forceUpdateMethodOrNonNestedReferencesPresent = | |||
| forceUpdateMethod || !groupedSourceReferences.nonNested.isEmpty(); | |||
| // if an update method is forced or there are non nested source references then | |||
There was a problem hiding this comment.
Q1. The only places were we currently fall back on update method is when we have 2 source arguments in the mapping method right?
Q2. This is a new situation. I think we need to explain what actually triggers this update methods. There have been many discussions in the past why we use them in a chain that only uses create methods.
I'm not sure.. In general. were would this lead is in the future? Should we actually allow this situation?
There was a problem hiding this comment.
Q1: Yes until now the only place where we fall back to update method was having 2 source methods.
Q2: Exactly this is another corner case. All these cases are quite complex and are forcing us to do some complex logic in there.
I agree with you, this can be more problematic in the future.
What we could do is not to allow mappings that are not consistent mappings to different sources should not be allowed maybe. This means that we would have to do checks at the beginning and if we see something like this we error out. So if we have not symmetric mappings to sources (mappings to different sources within the levels)
There was a problem hiding this comment.
What we could do is not to allow mappings that are not consistent mappings to different sources should not be allowed maybe.
I'm just thinking: what are frameworks like dozer doing in these cases? Are we missing something?
Anyway.. If we can support a case in a simple manner (like above) we probably should.
There was a problem hiding this comment.
I am not entirely sure what the other frameworks do in such a case. Maybe we need to test it 😄
| forceUpdateMethod || !groupedSourceReferences.nonNested.isEmpty(); | ||
| // if an update method is forced or there are non nested source references then | ||
| // we need to create mapping options for forged methods (i.e. Ignore unmapped target properties | ||
| // because it the nonNested properties might contain the target properties) |
There was a problem hiding this comment.
I think we need to put examples in the comments. I'm kind of lost what happens here.
There was a problem hiding this comment.
Again, I completely agree with you. It is complex (maybe even too much). I can put some examples here, or even mention the tests that are testing this
| MappingOptions sourceMappingOptions = MappingOptions.forMappingsOnly( | ||
| groupByTargetName( entryBySP.getValue() ), | ||
| forceUpdateMethod | ||
| forceUpdateMethod, |
There was a problem hiding this comment.
the parameter name is restrictToDefinedMappings however we put inforceUpdateMethod how do they relate?. Same goes for the next arg.
There was a problem hiding this comment.
It was like that before. Maybe we need to rename it to restrictToDefinedMappings instead.
|
@filiphr : apart from the remarks I head about naming and putting in some references to the test cases, I don't have anything to add. Please feel free to go ahead. I do think we need to refactor this stuff one day. There's already a PR for removing the mappings originating from the old enum mapping solution. Whence that is done, we could see how understandable the code is and add references to test cases where applicable. |
|
I'll apply those remarks and push the changes. I agree we need to refactor this and simplify it if possible. We have enough test cases I think to be able to do it correctly 😄 |
|
@sjaakd I update the comments can you have a look again if it is better? |
|
@filiphr go ahead.. |
Fixes #1269
We are basically resorting to creating update methods in case a case like in the issue occurs. The generated mapper will look like the one in this gist
I have added a note in the documentation that it can happen that an ignoring unmapped target policy is used.