Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support enum avro serialization with default value #391

Open
wants to merge 4 commits into
base: 2.16
Choose a base branch
from

Conversation

Sonic-Rage
Copy link

@Sonic-Rage Sonic-Rage commented Sep 8, 2023

Fixes #388.

avro/pom.xml Outdated
@@ -47,7 +47,7 @@ abstractions.
<dependency>
<groupId>org.apache.avro</groupId>
<artifactId>avro</artifactId>
<version>1.8.2</version>
<version>1.9.2</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under impression that this will break about everything... did you run unit test suite?

But even if not, we need this to be in separate PR, with separate issue.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wary of that upgrade as well, but doing that version bump and running the avro test suite resulted in passing tests.

We're a gradle shop so running stuff with maven was a bit hokey with all our firewall rules. Readying through #167 looks like some stuff has been upgraded but I imagine there must be missing unit tests for some of the underlying avro lib changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's odd wrt tests: for me code did not compile with missing imports.
And compilation on CI and locally did not work.

I wish I was more up to date with Avro lib changes since there are newer minor versions, some of which might resolve issues -- although from what I remember, things were quite messy wrt backwards-compatibility (lack thereof) for some specific usage.

*
* @since 2.16
*/
private static final JacksonAnnotationIntrospector JACKSON_ANNOTATION_INTROSPECTOR = new JacksonAnnotationIntrospector();
Copy link
Member

@cowtowncoder cowtowncoder Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, AnnotationIntrospector should be passed from outside; it may be reconfigured by caller.
We should not rely on default instance.

This just means some more plumbing of things to get AnnotationIntrospector passed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. thanks

@cowtowncoder
Copy link
Member

Ok, I realized that the avro core dependency upgrade to 1.9.x would be needed to access "enum default" information (both create and get). But this upgrade is not possible since jackson-dataformat-avro code itself is incompatible with 1.9.x (see #167 f.ex).

So I am not 100% sure this is actually achievable at this point.

I did make couple of changes to PR, but it is unfortunately not in a compilable state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JsonEnumDefaultValue not supported when using AvroMapper to generate schema from JAVA class
2 participants