Handle duplicate fields from the type and interface #128 #129
Conversation
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
=========================================
Coverage 90.83% 90.83%
- Complexity 404 413 +9
=========================================
Files 49 49
Lines 993 993
Branches 130 131 +1
=========================================
Hits 902 902
- Misses 60 62 +2
+ Partials 31 29 -2
Continue to review full report at Codecov.
|
| FieldDefinitionToParameterMapper.mapFields(mappingConfig, typeDefinition.getFieldDefinitions(), typeDefinition.getName()) | ||
| .forEach(p -> allParameters.put(p.getName(), p)); | ||
| // includes parameters from the interface | ||
| getInterfacesOfType(typeDefinition, document).stream() |
There was a problem hiding this comment.
Why do we need to get fields from the interface in the first place?
By definition, for any implemented interface, all fields should be declared in the type itself as well.
Is it to get metadata like Javadoc?
In this case I think we should "merge" the interface field with the corresponding type's field, because I imagine the doc or directives can also come from the type's field.
Maybe we should take everything from the type's field in priority, and if that field doesn't have a metadata element (like a doc) we fallback to the metadata from the interface's field.
There was a problem hiding this comment.
From the example that is shown in the bug, javadoc was present in the interface and not the type.
Another example: you can require custom serialization for a data type that is defined in the interface. And you don’t want to duplicate directives and javadoc in all types for this field. So you will define it in the interface. The type will have just the field.
There was a problem hiding this comment.
Yes, that's why we need to read the interface's fields as well, indeed.
However, we could imagine a situation where the doc is overridden in the type (or not even present in the interface), and in that case we need to take the doc from the type's field, not the interface's one.
But with the code as it is here, we would simply override the type's fields data with the interface's one, and lose anything declared directly in the implementing type.
There was a problem hiding this comment.
I've modified the logic to take JavaDoc and annotations from the type.
And if something is absent then it will be taken from the interface field.
There was a problem hiding this comment.
I'm so sorry to bring this up only now, but now I'm wondering whether we need to look at interface fields at all.
I mean, to generate the Java code for the interface, we use the interface fields and docs. And to generate the Java code for the implementing class, we don't need to look at the interface fields at all, do we?
Javadoc would naturally be inherited by overriding methods (we don't even need to do this at codegen level). And annotations are not inherited, but do we want that?
There was a problem hiding this comment.
Could you please create a separate issue with your suggestion?
We can discuss it there.
Thanks!
There was a problem hiding this comment.
Sure I can do that, I just felt like it was another way to fix the issue at hand, not a separate issue.
But you're right, we can merge this and fix the bug right away.
| allParameters.put(paramDef.getName(), merge(existingParamDef, paramDef)); | ||
| } else { | ||
| allParameters.put(paramDef.getName(), paramDef); | ||
| } |
There was a problem hiding this comment.
This is a minor comment, but the whole if block can be replaced by:
allParameters.merge(paramDef.getName(), paramDef, ::merge)Or something along those lines.
There was a problem hiding this comment.
Thank you so much. I never used this method before.
Fixes #128