[Avro] Add support for @AvroDefault and @Nullable annotations #49
Conversation
|
@baharclerode On NPE, it is fairly safe to say it is not intentional. I can't think of a reason to do so. Patch makes sense on its own, although I think reordering wrt default value is something that would actually be useful in at least one other place, where converting "resolving" deserializer (that is, one where separate reader/writer schemas are used). One note on parsing default value: this will be bit of a pain to merge into |
|
I think my final usecase requires avro 1.8.1 compatibility anyways (and jackson-dataformat-avro 2.8 is not compatible with 1.8.1), so I can just rebase this onto master and reopen a new pull request if you'd like. |
|
I think it'b great if you could add other pieces for Jackson 2.8, and I'll merge those, and then default handling for |
|
Just realized that this will implement/fix #13. |
| // Appears to be a bug in POJOPropertyBuilder.getMetadata() | ||
| // Can't specify a default unless property is known to be required or not, or we get an NPE | ||
| // If we have a default but no annotations indicating required or not, assume required. | ||
| if (_hasAnnotation(m, AvroDefault.class) && !_hasAnnotation(m, JsonProperty.class)) { |
There was a problem hiding this comment.
I don't think check for @JsonProperty should be needed here.
Is that only used to avoid NPE or for some other need?
There was a problem hiding this comment.
This is only to work around the NPE. I just saw your message about fixing it, so I'll test everything with that and see if I can remove this workaround!
|
@baharclerode I just fixed what was causing |
|
Btw, not related to this patch but I did some butchering for tests in |
|
Building is good! The test changes looked alright at a quick glance. I'm closing out this PR so that I can reopen it against master. |
This adds support for embedding the default values (Provided by
@AvroDefaultor@JsonProperty) into the generated avro schemas. This also adds support for@Nullablein case there are multiple annotation introspectors in play and the annotations need to be recognized despite normally being the default.I seem to have found a weird bug where if a property has a default value or a description, but is neither explicitly marked as required nor not required,
POJOPropertyBuilder.getMetadata():503will throw an NPE when it tries to coerce therequiredmarker into a boolean primitive. I wasn't sure if this was intentional or something I don't fully understand. I worked around it by providing explicitrequiredmarkers if@AvroDefaultis present, but there's probably a better way to handle this that I'm not aware of. Normally this isn't a problem because@JsonPropertydoesn't allow you to specify a default value or description without also setting the required marker totrueorfalse.Additional PRs for
@AvroSchema,@AvroMeta, and@Unionwill probably be filed tomorrow sometime. The PR for adding polymorphic deserialization backported to the 2.8 branch will follow those.