Skip to content

Allow disabling native type ids in IonMapper #232

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

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

jobarr-amzn
Copy link
Contributor

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

  • Native type ids may be disabled in Ion{Generator,Factory,Mapper}
  • IonParser capability introspection canReadTypeId => true
  • IonGenerator#writeTypePrefix override removed, enabling non-native

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:

  • Are feature flags the preferred way to do this, or booleans like protected boolean _cfgCreateBinaryWriters = false; in IonFactory?
  • If feature flags, then should Feature implement JacksonFeature or FormatFeature?
  • I copied the established pattern for Feature flags, but this seems like a pretty ideal use case for EnumSet. Thoughts?

@cowtowncoder
Copy link
Member

First things first:

  • Features or flags? For format modules, Features for sure.
  • Use FormatFeature for format-specific read/write features -- either ParserType.Feature or separate IonReadFeature / IonWriteFeature. F.ex see JsonParser.Feature and JsonGenerator.Feature)
  • but if there are any per-Factory features, JacksonFeature (see JsonFactory.Feature)

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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.

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

* Native type ids may be disabled in `Ion{Generator,Factory,Mapper}`
* IonParser capability introspection `canReadTypeId => true`
* IonGenerator#writeTypePrefix override removed, enabling non-native

Together these changes allow `AsPropertyDeserializer` with an
`IonParser` backing to deserialize either style while the `IonGenerator`
will honor its constructed feature set.
@cowtowncoder cowtowncoder merged commit fa5450f into FasterXML:2.12 Dec 10, 2020
cowtowncoder added a commit that referenced this pull request Dec 10, 2020
@cowtowncoder
Copy link
Member

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
Copy link
Member

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

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.

2 participants