[Avro] Fix custom deserializers not being used #71

baharclerode · 2017-04-04T13:37:45Z

Found an issue that wasn't caught by one of the test suites.

Turns out because I'm always returning a TypeDeserializer for every property, Jackson doesn't use any custom deserializers that are registered for that property. I have a workaround here that checks for one and uses it instead, but I think there's a better way whereby I don't return a TypeResolverBuilder for properties that have custom deserializers, but I couldn't get that working. What's the proper way to fix this?

baharclerode · 2017-04-04T13:39:04Z

avro/src/main/java/com/fasterxml/jackson/dataformat/avro/AvroTypeDeserializer.java

@@ -57,6 +54,13 @@ public Object deserializeTypedFromScalar(JsonParser p, DeserializationContext ct

    @Override
    public Object deserializeTypedFromAny(JsonParser p, DeserializationContext ctxt) throws IOException {
+        AnnotationIntrospector ai = ctxt.getAnnotationIntrospector();


I feel like instead of having the type deserializer look for custom deserializers, there should be some way to avoid using the type deserializer at all for properties that have custom deserializers - but I don't know where that check needs to be made.

The whole idea of type (de)serializers is that they can be composed with content (de)serializers and work independently. But use of type (de)serializers is inferred from polymorphic handling, so perhaps that should not be enabled if no type handling is desired?

baharclerode · 2017-04-04T13:39:48Z

avro/src/main/java/com/fasterxml/jackson/dataformat/avro/deser/AvroParserImplDecoder.java

@@ -138,6 +138,13 @@ public void skipString() throws IOException {
    @Override
    public ByteBuffer readBytes(ByteBuffer old) throws IOException {
        consumeToken(JsonToken.VALUE_EMBEDDED_OBJECT);
+        byte[] value = _parser.getBinaryValue();


Bugfix: Caller expecting old to be populated with data and not actually using the return value needs to be supported.

baharclerode · 2017-04-04T13:42:32Z

.../src/test/java/com/fasterxml/jackson/dataformat/avro/interop/annotations/AvroEncodeTest.java

@@ -74,8 +76,67 @@ protected CustomComponent read(Object reuse, Decoder in) throws IOException {

    }

-    protected Wrapper wrapper;
+    public static class UuidAsBytesAvroEncoding extends CustomEncoding<UUID> {


The original format of this test meant that if the custom deserializer wasn't picked up, Jackson was still flexible enough that it would pass the test because it could read what it wrote. This UuidAsBytesAvroEncoding actually changes the serialized format, which means this will now break if the custom serializers/deserializers are ignored.

cowtowncoder · 2017-04-04T20:24:36Z

Ouch. Type (de)serializers absolute and positively should never be used for non-polymorphic properties, so that should not be done. I guess I did not read the patch carefully enough.
I am also bit worried about performance aspects, but first problem is of course correctness.

cowtowncoder · 2017-04-04T20:25:48Z

avro/src/main/java/com/fasterxml/jackson/dataformat/avro/AvroTypeDeserializer.java

@@ -57,6 +54,13 @@ public Object deserializeTypedFromScalar(JsonParser p, DeserializationContext ct

    @Override
    public Object deserializeTypedFromAny(JsonParser p, DeserializationContext ctxt) throws IOException {
+        AnnotationIntrospector ai = ctxt.getAnnotationIntrospector();
+        if (ai != null && _property != null) {
+            JsonDeserializer<?> deser = ctxt.deserializerInstance(_property.getMember(), ai.findDeserializer(_property.getMember()));


This is extremely inefficient as well; usually deserializers are cached, and if this is not done much (most?) of the time is spent on fetching and contextualizing deserializer...

Yeah, this was more in the "it worked" category rather than the "this is how to fix it" category.

I think I found a better solution, about to push up some changes.

cowtowncoder · 2017-04-04T20:26:40Z

avro/src/main/java/com/fasterxml/jackson/dataformat/avro/deser/AvroParserImplDecoder.java

+            old.put(value);
+            old.flip();
+            return old;
+        }
        return ByteBuffer.wrap(_parser.getBinaryValue());


This should pass value, and not call accessor again.

baharclerode · 2017-04-04T20:49:49Z

avro/src/main/java/com/fasterxml/jackson/dataformat/avro/AvroAnnotationIntrospector.java

@@ -161,6 +162,10 @@ public Object findSerializer(Annotated a) {
    }

    protected TypeResolverBuilder<?> _findTypeResolver(MapperConfig<?> config, Annotated ann, JavaType baseType) {
+        // If there's a custom deserializer, then it's responsible for handling any type information
+        if (config.getAnnotationIntrospector().findDeserializer(ann) != null) {


Is this the best way to do this check?

I don' t think this approach makes sense: annotations are just one way to register deserializers; modules register others. But beyond this, as I said, type handlers and content handlers are not meant to be aware of each other.

cowtowncoder · 2017-04-05T06:22:21Z

Ok, so, yeah. I don't think AnnotationIntrospector path works: there should not be TypeDeserializer for things that do not need polymorphic handling.
What might work for some cases is to look for @Union, since its existence does signal need for unions. But since schema information isn't otherwise available, I am not sure this part of Avro's type system can actually be support by Jackson currently.

cowtowncoder · 2017-04-14T18:53:38Z

avro/src/main/java/com/fasterxml/jackson/dataformat/avro/deser/AvroParserImplDecoder.java

@@ -138,7 +138,14 @@ public void skipString() throws IOException {
    @Override
    public ByteBuffer readBytes(ByteBuffer old) throws IOException {
        consumeToken(JsonToken.VALUE_EMBEDDED_OBJECT);
-        return ByteBuffer.wrap(_parser.getBinaryValue());
+        byte[] value = _parser.getBinaryValue();


How about I merge this separate from PR as it seems legit separate from other question.

baharclerode · 2017-04-14T21:07:31Z

Following the discussion on #60, the recent changes to master have fixed this problem. I have updated this PR to include just the tests to catch the issue if it appears again.

cowtowncoder · 2017-04-14T22:50:06Z

Makes sense, merged.

baharclerode commented Apr 4, 2017

View reviewed changes

cowtowncoder reviewed Apr 4, 2017

View reviewed changes

baharclerode force-pushed the bah.CustomEncoder_Fix branch from ce90424 to c4cf826 Compare April 4, 2017 20:45

baharclerode commented Apr 4, 2017

View reviewed changes

cowtowncoder reviewed Apr 14, 2017

View reviewed changes

baharclerode mentioned this pull request Apr 14, 2017

[Avro] Add support for @Union and polymorphic types #60

Merged

baharclerode force-pushed the bah.CustomEncoder_Fix branch from c4cf826 to eb812cf Compare April 14, 2017 21:03

[Avro] Fix custom deserializers not being used

c2c2040

baharclerode force-pushed the bah.CustomEncoder_Fix branch from eb812cf to c2c2040 Compare April 14, 2017 21:07

cowtowncoder closed this Apr 14, 2017

cowtowncoder reopened this Apr 14, 2017

cowtowncoder merged commit 7210c75 into FasterXML:master Apr 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Avro] Fix custom deserializers not being used #71

[Avro] Fix custom deserializers not being used #71

baharclerode commented Apr 4, 2017

baharclerode Apr 4, 2017

cowtowncoder Apr 4, 2017

baharclerode Apr 4, 2017 •

edited

Loading

baharclerode Apr 4, 2017

cowtowncoder commented Apr 4, 2017

cowtowncoder Apr 4, 2017

baharclerode Apr 4, 2017

cowtowncoder Apr 4, 2017

baharclerode Apr 4, 2017

cowtowncoder Apr 5, 2017

cowtowncoder commented Apr 5, 2017

cowtowncoder Apr 14, 2017

baharclerode commented Apr 14, 2017 •

edited

Loading

cowtowncoder commented Apr 14, 2017

[Avro] Fix custom deserializers not being used #71

[Avro] Fix custom deserializers not being used #71

Conversation

baharclerode commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baharclerode Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Apr 5, 2017

Choose a reason for hiding this comment

baharclerode commented Apr 14, 2017 • edited Loading

cowtowncoder commented Apr 14, 2017

baharclerode Apr 4, 2017 •

edited

Loading

baharclerode commented Apr 14, 2017 •

edited

Loading