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

IonObjectMapper does not throw JacksonException for some invalid Ion #311

Closed
popematt opened this issue Jan 31, 2022 · 3 comments
Closed
Milestone

Comments

@popematt
Copy link
Contributor

popematt commented Jan 31, 2022

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.

@popematt popematt changed the title IonObjectMapper does not throw JsonParseException IonObjectMapper does not throw JacksonException for some invalid Ion Feb 3, 2022
popematt added a commit to popematt/jackson-dataformats-binary that referenced this issue Mar 4, 2022
popematt added a commit to popematt/jackson-dataformats-binary that referenced this issue Mar 4, 2022
popematt added a commit to popematt/jackson-dataformats-binary that referenced this issue Mar 4, 2022
popematt added a commit to popematt/jackson-dataformats-binary that referenced this issue Mar 4, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Mar 4, 2022
@cowtowncoder cowtowncoder changed the title IonObjectMapper does not throw JacksonException for some invalid Ion IonObjectMapper does not throw JacksonException for some invalid Ion Mar 4, 2022
cowtowncoder added a commit that referenced this issue Mar 4, 2022
@cowtowncoder cowtowncoder reopened this Mar 4, 2022
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 4, 2022

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

popematt commented Mar 9, 2022

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

@cowtowncoder
Copy link
Member

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants