AVRO-3649: reorder union types to match default #1919
Conversation
Why: We want to use @AvroDefault with @nullable. How does it help with resolving the issue: The schema generation automaticly picks the right order of the union type. Side effects: This change has no side effects, because: - The order of union types is irrelevant for schema compability. - This change enables definitions wich has been invalid.
| */ | ||
| public boolean isValidValue(JsonNode jsonValue) { | ||
| return isValidDefault(this, jsonValue); | ||
| } |
There was a problem hiding this comment.
👍 use standard method instead of static (it may replace private static boolean isValidDefault in near future), and avoid this long switch/case on schema/type.
About default value on union type, I wonder if it couldn't be even better to change logic and get the first compatible Schema of union with the value (And throw exception if none) ?
I did this code in local (and partially copy yours), and it seems to work fine (at least for unit test). I will put more details in linked JIRA Ticket in few minutes.
There was a problem hiding this comment.
thx for the feedback, can u give me some points what standard method you are talking about?
There was a problem hiding this comment.
Just was able to take a look at your PR. It looks like a even better solution, because it "fixes" the problem on a lower level.
Would you say that this PR is now obsolete?
There was a problem hiding this comment.
standard method => i would say "non static overridable method" (better for object programming).
For 2 PR of 2 solution, I think we should discuss about that on the JIRA issue, i may missed some point (i want advice of other developer).
There was a problem hiding this comment.
Is there any possible way we can rewrite this without adding or exposing the isValidValue(JsonNode ...) method?
There was a reason the isValidDefault method is private: we really don't want to be exposing the jackson internals in public methods and (if I remember correctly). There was a fair amount of work deleting these methods in the past, in order to one day replace the jackson internals.
If anyone has a better memory than me, please feel to chime in!
| */ | ||
| public boolean isValidValue(JsonNode jsonValue) { | ||
| return isValidDefault(this, jsonValue); | ||
| } |
There was a problem hiding this comment.
Is there any possible way we can rewrite this without adding or exposing the isValidValue(JsonNode ...) method?
There was a reason the isValidDefault method is private: we really don't want to be exposing the jackson internals in public methods and (if I remember correctly). There was a fair amount of work deleting these methods in the past, in order to one day replace the jackson internals.
If anyone has a better memory than me, please feel to chime in!
The best documentation for the reflection annotations is currently the Javadoc. Hopefully the new website will draw contributors to improve this situation... |
3 participants
Why:
We want to use @AvroDefault with @nullable.
How does it help with resolving the issue:
The schema generation automaticly picks the right order of the union type.
Side effects:
This change has no side effects, because:
What is the purpose of the change
Implementing AVRO-3649.
Verifying this change
Documentation