Handle no suitable empty constructor during BeanMappingMethod creation#1287
Conversation
…gMethod creation
agudian
left a comment
There was a problem hiding this comment.
Looks good! I just have a minor comment on the wording and I wonder what happens if there is only an inaccessible non-private constructor available (default visibility or package visibility)...
| for ( ExecutableElement constructor : constructors ) { | ||
| if ( (constructor.getModifiers().contains( Modifier.PUBLIC ) | ||
| || constructor.getModifiers().contains( Modifier.PROTECTED ) ) | ||
| if ( !constructor.getModifiers().contains( Modifier.PRIVATE ) |
There was a problem hiding this comment.
How do we know here that the constructor is actually accessible from the package of the mapper?
There was a problem hiding this comment.
That is the case, we don't really know. And I think it is difficult to check this because the test is later on. Actually one of our test have package protected classes and package protected static classes. The modifiers are then empty (and that is why it started failing).
I am not sure if it is worth it to add more complex checks? It will create a compile problem in the generated file in any case.
| GENERAL_VALID_DATE( "Given date format \"%s\" is valid.", Diagnostic.Kind.NOTE ), | ||
| GENERAL_INVALID_DATE( "Given date format \"%s\" is invalid. Message: \"%s\"." ), | ||
| GENERAL_NOT_ALL_FORGED_CREATED( "Internal Error in creation of Forged Methods, it was expected all Forged Methods to finished with creation, but %s did not" ), | ||
| GENERAL_NO_SUITABLE_CONSTRUCTOR( "%s does not have a suitable empty constructor." ), |
There was a problem hiding this comment.
I think we can omit the “suitable”, as you already mention “empty”. Maybe add “visible” or “accessible”?
There was a problem hiding this comment.
I agree "accessible" sounds better than "suitable". I'll change it. Do you think that we need to give a suggestion what they should do? To me it is pretty clear, but I don't know 😄
There was a problem hiding this comment.
Yes, I think it’s pretty obvious... 🙂
…nMappingMethod creation
|
Looks good! Feel free to squash and merge! |
|
Thanks a lot for the review @agudian |
Fixes #1283.
This PR fixes #1283 by moving the no suitable empty constructor check during the
BeanMappingMethodcreation (only if there is no factory method). With this we additionally get the same check for non-reverse methods andBeanMapping#resultType