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

`IonObjectMapper` does not throw JacksonException for some invalid Ion #311

Closed popematt closed 2 years ago

popematt commented 2 years ago

In some cases, the IonObjectMapper is not correctly catching and wrapping IonExceptions. For example, given this class:

class FooBar {
    int foo;
    List<Object> bar;
}

When attempting to deserialize invalid Ion, sometimes we get a JsonMappingException, and other times an IonException.

IonObjectMapper mapper = IonObjectMapper.builder().build();

// throws a JsonMappingException
FooBar fb1 = mapper.readValue("{ foo: 1, bar: () ] }", FooBar.class); 
// throws an IonException
FooBar fb1 = mapper.readValue("{ foo: 1, bar: ( ] )  }", FooBar.class); 

Why does this matter? Some other libraries (eg. Ktor, in the JacksonConverter) expect particular exceptions (eg. JsonParseException or JsonMappingException) from an ObjectMapper, and behave in unexpected ways when the IonException leaks through.

I believe the fix for this is to catch and wrap IonException as a JsonParseException in the IonParser.nextToken() method, and I intend to work on a PR for it.

cowtowncoder commented 2 years ago

@popematt Ok, looks like there is some issue with Jackson 3.0 and these changes -- looks like there is somehow no failure for unknown symbols? I don't know how to look into possible problem so pushed changes to master. But would appreciate help in figuring out what is going on, if possible.

Note that changes between 2.x and 3.0 wrt renamed methods should not change semantics. But I guess something is changed...

Reopening the issue; leaving the patch in for now.

popematt commented 2 years ago

I think now that #314 is merged, we can close this, right @cowtowncoder ?

cowtowncoder commented 2 years ago

Yes.