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

Gracefully handle Deserialization of Joda values from JSON Objects #115

Closed

Conversation

asaph
Copy link

@asaph asaph commented Jun 24, 2020

Gracefully handle deserialization of Joda objects that were serialized without first registering the JodaModule. Under such a situation, objects containing Joda types would be serialized with Jackson's default serialization behavior which is to use all the object's public getters to inform serialization. This results in rather verbose output, but no errors or warnings logged.
More critically, the resulting json cannot be deserialized even after registering the JodaModule. Neglecting to register the JodaModule is an easy trap to fall into because doing so was not required in Jackson 1.x, which was a monolith that supported Joda types out of the box. Jackson 2.x was broken out into modules. Anyone serializing objects with Joda types who upgraded to Jackson 2.x without realizing this might have inadvertently serialized and persisted objects which are undeserializable. This fix adds support for deserializing all Joda object types which have been serialized with Jackson's default all-public-getters serializer.

@cowtowncoder
Copy link
Member

I appreciate you contributing this. My first thought is that I am not sure I would want to add support for such handling.

You may want to bring this up on

https://groups.google.com/forum/#!forum/jackson-dev

and we can see how others feel about it.

@asaph
Copy link
Author

asaph commented Jun 25, 2020

Thanks for your consideration. To add a little more context, an example of an Exception error message people see when failing to deserialize these objects is:

Can not deserialize instance of org.joda.time.DateTime out of START_OBJECT token

FWIW I believe people have been running into this gotcha for years and generally not getting down to the root of the problem. One example (posted in 2 places):

I can appreciate your reservations around adding this mitigation to the library because it can potentially mask a misuse of the library (i.e. failing to register the JodaModule with your ObjectMapper prior to serialization). IMO including this mitigation is a justifiable tradeoff as the the consequence is having undeserializable data. I'm definitely open to other opinions. If you could kindly post it on the mailing list you referenced above, we'll see what the consensus is and take it from there.

@cowtowncoder
Copy link
Member

Sounds reasonable. Another possibility could be to create separate module, but I can see why that might get tricky since this would need to add support in addition to canonical representations. I will send a question on dev list and see how others feel about this.

Also: now that I think about this more, I should mention this:

FasterXML/jackson-databind#2683

which would try to address similar question, on blocking serialization if no module registered. I think something similar could be done for Joda types -- regardless of whether support was also added for deserializing from JSON Objects.
I will file issue as well.

@cowtowncoder cowtowncoder changed the title Gracefully handle Deserialization of Joda Objects Gracefully handle Deserialization of Joda values from JSON Objects Jun 25, 2020
@asaph
Copy link
Author

asaph commented Jun 27, 2020

FasterXML/jackson-databind#2683 and FasterXML/jackson-databind#2776 are welcome enhancements because they would help developers catch mis-serialization issues at the development stage in the future. But given that Jackson 2.x is already widely deployed without these mitigations, I still think it's important for deserializers to make a best effort to recover gracefully, as undeserializable data is a most unwelcome problem.

I like the idea of addressing the problem at both the serialization and the deserialization ends. It's also worth noting that serialization and deserialization don't always occur within the same system. Consider the case of System A and System B with different codebases, library versions, owners and runtime environments. System A serializes and persists a payload, and then some time later, System B tries to deserialize the payload. It would be beneficial for System B, if it could gracefully recover from System A's mis-serialization issues. Postel's Law comes to mind.

@cowtowncoder
Copy link
Member

Yes, valid points; I esp. agree that source/destination systems can be very different, and many developers do not quite realize this (and suggest use of local timezone as default for various things).

@asaph asaph force-pushed the graceful_object_deserialization branch from 84b2da3 to d432fd2 Compare July 2, 2020 05:40
that were serialized without first registering
the JodaModule. Under such a situation, objects
containing Joda types would be serialized with
Jackson's default serialization behavior which
is to use all the object's public getters to
inform serialization. This results in rather
verbose output, but no errors or warnings logged.
More critically, the resulting json cannot be
deserialized even after registering the JodaModule.
Neglecting to register the JodaModule is an easy trap
to fall into because doing so was not required in
Jackson 1.x, which was a monolith that supported
Joda types out of the box. Jackson 2.x was broken
out into modules. Anyone serializing objects with
Joda types who upgraded to Jackson 2.x without
realizing this might have inadvertently serialized
and persisted objects which are undeserializable.
This fix adds support for deserializing all
Joda types which have been serialized with
Jackson's default all-public-getters serializer.
@asaph asaph force-pushed the graceful_object_deserialization branch from d432fd2 to 1bb5292 Compare July 2, 2020 05:43
@cowtowncoder
Copy link
Member

At this point I don't think I will merge this in, closing.

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