-
-
Notifications
You must be signed in to change notification settings - Fork 1k
#3652 Inverse Inheritance possible for ignore-mappings without source #3675
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
Conversation
|
|
||
| /** | ||
| * mapping can only be inversed if the source was not a constant nor an expression nor a nested property | ||
| * and the mapping is not a 'target-source-ignore' mapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still curious why this was explicitly stated here. I tried looking at the history of this method, but did not find a hint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was curious too @zyberzebra. This looks like an ancient thing. The MappingOptions was called Mapping at some point. The rename was done in 7bee121.
The line isIgnored && sourceName == null was added first in 4d8bc29. And looking at that commit it seems like the logic was not ported properly, because prior to that we had
// mapping can only be reversed if the source was not a constant nor an expression nor a nested property
if ( constant != null || javaExpression != null ) {
return null;
}
// should only ignore a property when 1) there is a sourceName defined or 2) there's a name match
if ( isIgnored ) {
if ( sourceName == null && !hasPropertyInReverseMethod( targetName, method ) ) {
return null;
}
}and after that we had
// mapping can only be reversed if the source was not a constant nor an expression nor a nested property
// and the mapping is not a 'target-source-ignore' mapping
if ( constant != null || javaExpression != null || ( isIgnored && sourceName == null ) ) {
return null;
}Things were quite different back then. We are now handling the target later and differently to back then, so the fix done by @Hypnagokali looks like spot on.
|
Thanks a lot for the quick fix @Hypnagokali. Nice spotting this.
Good catch @Hypnagokali. I think that we can remove the nor a nested property statement from the Javadoc. We are clearly not doing that there 😄 |
|
All right, I have adjusted the comment 😊 |
|
Thanks @Hypnagokali |
Fixes #3652
I have added a test case where the target has a property that does not exist on the source type.
The inverse inheritation still works, because this property is then simply ignored in
BeanMappingMethod#handleDefinedMapping(line 1240):I have another question:
The javadoc of
MappingOptions#canInversesays:But there is no check for a nested property in this method and it seems to work: Some tests uses
@InheritInverseConfigurationwith nested properties:e.g.:
ReversingNestedSourcePropertiesConstructorTest#shouldGenerateNestedWithConfigReverseShould I remove this statement from the javadoc comment?