FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
316 stars 136 forks source link

Allow disabling native type ids in IonMapper #232

Closed jobarr-amzn closed 3 years ago

jobarr-amzn commented 3 years ago

This change makes the behavior of the Ion format more similar to YAMLMapper in jackson-dataformat-yaml.

Together these changes allow AsPropertyDeserializer with an IonParser backing to deserialize either style while the IonGenerator will honor its constructed feature set.

This should address #149 and #225.

Open questions:

cowtowncoder commented 3 years ago

First things first:

As to EnumSet -- it'd be almost nice for EnumSet; if EnumSet was immutable I'd probably have used it originally. Using an int is bit old school, but at this point it is also the way it must be used for parser/generator features (ObjectReader and ObjectWriter pass them when constructing new parser/generator) to work.

cowtowncoder commented 3 years ago

One note: timing-wise this is pretty unfortunate: semantically, this would have to wait until 2.13 as it adds new API feature.

But depending on how safe the change is, I think I could perhaps consider adding it as "dark launch" in 2.12.x; would remain undocumented until 2.13 but available for those who know about it (and do not mind the technical slight incompatibility between patch versions).

jobarr-amzn commented 3 years ago

but if there are any per-Factory features, JacksonFeature (see JsonFactory.Feature)

Okay, my understanding is that that is not the case here. I think the pattern I'm following now is what is wanted, except my Feature enums should extend FormatFeature. I'll keep the same set of convenience plumbing that I see in jackson-dataformat-yaml.

But depending on how safe the change is, I think I could perhaps consider adding it as "dark launch" in 2.12.x; would remain undocumented until 2.13 but available for those who know about it (and do not mind the technical slight incompatibility between patch versions).

Thanks! 👍 For safety all the existing tests continue to pass unaltered and the new ones pass too ;)

Prior to this patch trying to round-trip Subclass like the new unit tests do results in:

com.fasterxml.jackson.databind.exc.InvalidTypeIdException: Missing type id when trying to resolve subtype of [simple type, class com.fasterxml.jackson.dataformat.ion.polymorphism.PolymorphicRoundTripWithSerializationAnnotations$BaseClass]: missing type id property '@class'
 at [Source: UNKNOWN; line: -1, column: -1]

    at com.fasterxml.jackson.databind.exc.InvalidTypeIdException.from(InvalidTypeIdException.java:43)
    at com.fasterxml.jackson.databind.DeserializationContext.missingTypeIdException(DeserializationContext.java:1945)
    at com.fasterxml.jackson.databind.DeserializationContext.handleMissingTypeId(DeserializationContext.java:1458)
    at com.fasterxml.jackson.databind.jsontype.impl.TypeDeserializerBase._handleMissingTypeId(TypeDeserializerBase.java:307)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer._deserializeTypedUsingDefaultImpl(AsPropertyTypeDeserializer.java:174)
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromObject(AsPropertyTypeDeserializer.java:113)
    at com.fasterxml.jackson.databind.deser.AbstractDeserializer.deserializeWithType(AbstractDeserializer.java:263)
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74)
    at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:322)
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4591)
    at com.fasterxml.jackson.dataformat.ion.IonObjectMapper.readValue(IonObjectMapper.java:224)
    at com.fasterxml.jackson.dataformat.ion.polymorphism.PolymorphicRoundTripWithSerializationAnnotations.testNativeTypeIdsEnabledByDefault(PolymorphicRoundTripWithSerializationAnnotations.java:42)
    <SNIP>...

I don't imagine that anyone would be sad by a successful deserialization instead though.

jobarr-amzn commented 3 years ago

I've updated this PR to use make IonGenerator.Feature implement FormatFeature, and also removed a couple static assertion imports to better match surrounding code.

How's it looking from your perspective? Does it look mergeable? Is there anything else I can do that would be helpful or customary?

cowtowncoder commented 3 years ago

Looks good and I trust you know what you are doing here, esp. wrt Ion knowledge. Should be able to merge soon; added just couple of tiny notes.

cowtowncoder commented 3 years ago

Merged: had fun time with 3.0 merge, got it done but... somehow looks like 2 of new tests fail, so could probably need help there.

cowtowncoder commented 3 years ago

Never mind, I had managed to accidentally remove a capability introspection method from IonParser; now works.