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

[Avro] Add support for @AvroEncode annotation #69

Merged
merged 2 commits into from
Mar 31, 2017

Conversation

baharclerode
Copy link
Contributor

@baharclerode baharclerode commented Mar 28, 2017

This enables support for the @AvroEncode annotation, which is the avro equivalent of a JsonSerializer/JsonDeserializer.

The serialization side of things was fairly straight-forward, since we can write directly to the underlying datum without worrying about parser state; Deserialization is a bit more tricky, since the parser inserts virtual tokens (start/end object, field names on records, etc.) that we need to consume those and otherwise ensure the parser state is updated so that deserialization control can be handed back to Jackson once the CustomEncoding has done its job.

This is the last major piece of the puzzle that brings compatibility with the apache implementation up to an acceptable level for my use cases, so hoping it'll make the cut for 2.9 😃


I also re-enabled the tests that were ignored in 2.8 due to namespace changes, since the underlying code changes were themselves reverted when merged into master a while back.

public Object findDeserializer(Annotated am) {
AvroEncode ann = _findAnnotation(am, AvroEncode.class);
if (ann != null) {
return CustomEncodingDeserializer.class;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AnnotationIntrospectorPair does not support returning JsonDeserializer instances, so using a HandlerInstantiator to instantiate the deserializer with the correct context information.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmh. What do you mean by not supporting that? Since its Object it should, and if not, that should be fixed...

@@ -113,6 +124,10 @@ public Object findSerializer(Annotated a) {
if (a.hasAnnotation(Stringable.class)) {
return ToStringSerializer.class;
}
AvroEncode ann = _findAnnotation(a, AvroEncode.class);
if (ann != null) {
return CustomEncodingSerializer.class;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AnnotationIntrospectorPair does not support returning JsonSerializer instances, so using a HandlerInstantiator to instantiate the serializer with the correct context information.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto here, my reading would suggest instances or classes both should work?
(but I guess if there's problem with one there's with the other too)

Copy link
Member

Choose a reason for hiding this comment

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

Or do you mean that since there isn't enough information, it may not be done? Contextualization will be done, however, so as long as serializer/deserializer implement Contextual(De)Serializer, matching createContextual() method will be called to give access to annotations, active configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _isExplicitClassOrOb check here requires that the primary return a Class object - If you return an instance of a JsonSerializer, _isExplicitClassOrOb returns false and the secondary is used. If that's unintended behavior, then yeah, that should be fixed, and this code could be simplified to just return a JsonSerializer instance with the context information already bound instead of deferring that to a custom HandlerInstantiator (and the same for JsonDeserializer above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think changing this to

return true;

would fix the issue, and would actually make the OrOb part of the method name make more sense, now that I think about it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok intention was to be reflected by method name: "Is (explicit) Class or Object". The only (?) kink there was simply that there are placeholder values to indicate "This is not a real value" -- just because Annotations themselves can NOT have value of null, ever, so if a Class-value annotation optional property is desired, it MUST have value of some bogus class.

So, long way of saying: yes, returning a (de)serializer absolutely should be supported and if not, there's a bug. I'll file a bug on jackson-databind and test this, for 2.9.

Copy link
Member

Choose a reason for hiding this comment

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

@baharclerode Ok, filed & fixed FasterXML/jackson-databind#1581 -- it was regression from 2.8, but wasn't covered by unit tests (ironically enough I had added some tests for AnnotationIntrospectorPair in 2.9... but not for this method).

@cowtowncoder
Copy link
Member

Ok hmmh. This is quite elaborate, and I'll have to think carefully... since the problem is that I would ideally want to reduce or eventually even eliminate dependencies to the standard avro lib, wrt encoding/decoding. If this is only sort of isolated area and functionality only affects annotated types (which I think is the case), it may still be ok. And I am +1 on increased interoperability per se.
I just don't want to add coupling to implementation; for example, to improve serialization speed I would eventually want to implement encoding directly.

I'll have to read over the code a bit more; I like many of the aspects (and we can probalby simplify some parts as per comments I added).

@baharclerode
Copy link
Contributor Author

baharclerode commented Mar 29, 2017

You won't be able to completely remove the encoding dependencies from the avro lib and support this annotation - The annotation specifically requires a subtype of CustomEncoding, which references both the Decoder and Encoder abstract classes; They are, however, effectively interfaces with a few default methods, which means that it will still be completely decoupled from the implementation. (CustomEncoding also references Schema, which can't be easily replaced though)

For example, the deserialization should already be decoupled, since that is handled by a wrapper that extends Decoder and wraps the AvroParserImpl using the existing methods to ensure that the parser state is advanced correctly. I don't think it even calls any of the decoder primitives that AvroParserImpl exposes.

Serialization would probably require a similar pattern, but as long as the underlying implementation supports the same Encoder primitives, the NonBSGenericDatumWriter can be swapped out for any other (faster) implementation that knows how to pass itself or a wrapper into EncodedDatum.write() method.

@cowtowncoder
Copy link
Member

Agreed: I am ok with using annotations as well as simple interfaces (Decoder, Encoder) for this part.
I am only worried about keeping those isolated, so that not everything has to implement these abstractions.
And if at some future point it seemed useful we could further modularize things to isolate even these aspects -- but that might never happen.

But note that my explicit goal (hoping to still get done for 2.9) is to remove actual usage of BinaryDecoder for standard decoding. I don't think this would interfere with that goal, I just want to mention it just in case I am overlooking something.

@baharclerode
Copy link
Contributor Author

Your fix for _isExplicitClassOrOb worked perfectly, and I've updated the PR to remove the AvroHandlerInstantiator.

@cowtowncoder
Copy link
Member

@baharclerode Looks like there was yet another smaller issue, now fixed too:

FasterXML/jackson-databind#1584

but I don't think it'd affect your changes here.

@cowtowncoder cowtowncoder added this to the 2.9.0.pr3 milestone Mar 31, 2017
@cowtowncoder cowtowncoder merged commit 364edeb into FasterXML:master Mar 31, 2017
cowtowncoder added a commit that referenced this pull request Mar 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants