Skip to content
This repository was archived by the owner on Nov 7, 2019. It is now read-only.

Use InvalidFormatException for deserialization parse failures #76

Closed

Conversation

mikewatt
Copy link

This alters the behaviour of the deserializers to throw more specific InvalidFormatExceptions rather than JsonMappingExceptions, initially discussed in dropwizard/dropwizard#1527.

Since this does change behaviour -- exceptions will no longer have a cause of type DateTimeException (or any nested cause at all) -- I'm not sure if it is acceptable for a patch release.

@cowtowncoder
Copy link
Member

Good question, wrt backwards compatibility. One thing to consider is that in most cases caller up the stack (BeanDeserializer, most likely) is likely to catch end re-package exception anyway, if it's not a JsonMappingException. So results may not change as much as it might first appear.

Another question is whether it would make sense to also re-throw DateTimeException; it has bit less information, but it is very likely that it is result of a similar problem. In fact, I'd suggest just wrapping both, and seeing if there are any cases where distinction is meaningful from user perspective.

One suggestion for code, as is, is to actually create a helper method in super-type (or in couple of them since inheritance hierarchies differ). Similar to rethrowDateTimeException, but taking one more argument (String value used? Or maybe another overload for number) to call. That could hide some of logic, if different handling is needed.

@cowtowncoder
Copy link
Member

@mikewatt I actually reworked this idea slightly, to reuse existing method (but pass more information needed), and it will go in 2.7.4. I do add cause both for backwards compatibility as well.

@cowtowncoder cowtowncoder added this to the 2.7.4 milestone Apr 21, 2016
@mikewatt
Copy link
Author

Great, thanks @cowtowncoder.

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

Successfully merging this pull request may close these issues.

2 participants