#3902: validate unknown properties in @Ignored#3908
Conversation
thunderhook
left a comment
thunderhook
left a comment
There was a problem hiding this comment.
Hi @znight1020!
Thanks a lot for your PR! I have two minor comments, but other than that, great work!
| additionalExcludes.add( "org/mapstruct/ap/test/bugs/_1596/*.java" ); | ||
| additionalExcludes.add( "org/mapstruct/ap/test/bugs/_1801/*.java" ); | ||
| additionalExcludes.add( "org/mapstruct/ap/test/bugs/_3089/*.java" ); | ||
| additionalExcludes.add( "org/mapstruct/ap/test/bugs/_3902/*.java" ); |
There was a problem hiding this comment.
I don't think that this is needed. I uncommented that line and my maven build was successful. Could you please take another look at it?
There was a problem hiding this comment.
When I run a mvn clean install locally, I'm seeing a CompilationFailureException during MavenIntegrationTest.fullFeatureTest() with the following error:
[ERROR] .../mapstruct/processor/src/test/java/org/mapstruct/ap/test/bugs/_3902/Issue3902Mapper.java:[23,12] No property named "foo" exists in @Ignored for target type "Issue3902Mapper.ZooDto"
I suspect this is because I'm running the build which includes the integrationtest module.
I noticed the comment in MavenIntegrationTest that mentions automatic exclusion for files named with *Erroneous*.
/**
* ... excluding all classes that contain ... in their path/file name:
* ...
* <li>{@code *Erroneous*}</li>
* ...
*/
Would the correct approach here be to rename Issue3902Mapper.java to ErroneousIssue3902Mapper.java and remove the explicit exclusion I added?
There was a problem hiding this comment.
Good catch!
Yes that would indeed be better, the FullFeatureCompilationExclusionCliEnhancer should be used to exclude tests for specific compilers (and it seems for some edge cases using SPIs or accessor naming strategies).
(and my comment "I uncommented that line and my maven build was successful" was a lie, I did not run the integration test module, sorry for that 😅)
There was a problem hiding this comment.
That's totally fine! Your comment was the perfect hint that led me to discover the Erroneous naming convention, so it was very helpful 😄
I've applied the requested changes. Thank you again for the detailed review and great guidance!
filiphr
left a comment
filiphr
left a comment
There was a problem hiding this comment.
Thanks @znight1020 for the fix.
What was the reason for not using " Did you mean "%s"?" in the BEANMAPPING_UNKNOWN_PROPERTY_IN_IGNORED error message?
| args = new String[] { | ||
| targetPropertyName, | ||
| resultTypeToMap.describe(), | ||
| mostSimilarProperty |
There was a problem hiding this comment.
I don't understand why we removed the mostSimilarProperty from this error message?
There was a problem hiding this comment.
To be honest, my main focus was on fixing the NullPointerException and ensuring a clear, specific error message for @Ignored was reported. I completely overlooked adding the "Did you mean" suggestion, my apologies for that.
Adding it would be much more consistent with the other 'Unknown property' errors. Should I create a new PR for this enhancement?
There was a problem hiding this comment.
It would be great if you could create another PR and add that as well
3 participants
Fixes #3902
Solution
The root cause of the
NullPointerExceptionwas identified within theBeanMappingMethod.handleDefinedMappingmethod's error reporting logic.Root Cause Analysis
@Ignored(targets = {"foo"}), MapStruct internally creates a MappingOptions object. When processing an unknown property like "foo", the call tomapping.getElement()returnsnull.Messager.printMessage(), passing the null element. This results in an NPE, as the messager requires a valid source element to determine the error location.Implementation Details
A new, more specific error message
BEANMAPPING_UNKNOWN_PROPERTY_IN_IGNOREDwas added toMessage.java.(mapping.isIgnored() && mapping.getElement() == null)to detect properties that originate from an@Ignoredannotation and do not exist. This correctly distinguishes the case from a standard@Mapping(target="...", ignore=true)on an unknown property.With these changes, all mvn clean install builds now pass successfully.