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

Add new mapping exception type for enums and UUIDs (and maybe in future for subtypes) #1000

Closed
natnan opened this issue Nov 10, 2015 · 8 comments
Milestone

Comments

@natnan
Copy link

natnan commented Nov 10, 2015

Extension request to #32.

I'm using dropwizard and I'd like to be able to return more friendly messages to the user when they enter wrong String for enum (which I also use for subtypes) and format for UUIDs. Could we add new exception(s) for these cases? Or use InvalidFormatExcetion for all or some of them?

@natnan
Copy link
Author

natnan commented Nov 10, 2015

Congratulations on issue #1000 by the way :).

@cowtowncoder
Copy link
Member

That is possible, yes, and sounds like a potentially good idea. I'll have to check what exact type to use, and try to make sure all codepaths produce the same thing. Sub-type should probably be JsonMappingException; this would also retain path information and be quite backwards-compatible.

And yes, congratulations -- you just won a FREE copy of the Jackson library! Downloadable via central Maven repository.

@natnan
Copy link
Author

natnan commented Nov 10, 2015

Sub-type should probably be JsonMappingException;

I'm fine with that. JsonMappingException is fine for all of them actually, as long as I can distinguish them. And that's my problem here. Subtype and enum failures construct JsonMappingException with String msg constructor. And String isn't the best way to distinguish errors.

UUID failure on the other hand, has a cause field that is NumberFormatException, so that's actually something I can work with.

If you decide which way to go, I might be able to send a PR.

@cowtowncoder
Copy link
Member

Ah actually you are right: InvalidFormatException is existing Jackson exception type, extending JsonMappingException, so that would work better than basic JME. So that would make lots of sense. Somehow I was assuming use of JDK exception like IllegalArgumentException.

As to UUIDDeserializer, it actually looks like it should also use InvalidFormatException; I could change this as well. One potential concern is cause, which existing code might want to look at. So maybe that should be retained.

@cowtowncoder cowtowncoder changed the title Add new mapping exception type for enums, subtypes and UUIDs Add new mapping exception type for enums and UUIDs (and maybe in future for subtypes) Nov 20, 2015
@cowtowncoder
Copy link
Member

Edited title a bit since I think I will first tackle enum and UUID case: improvements to indicating subtype may well make sense but will probably require bit different approach, unless I misunderstood the request. Problem being not just that there are many other edge cases (wrt default type to use, or not), but also that it's more of a mismatch than invalid format problem (although there are similarities as well).

@natnan
Copy link
Author

natnan commented Nov 20, 2015

👍
Yeah, I agree subtype is a different case than formatting from "object" perspective. From user (i.e. REST client) and JSON perspective though, it's quite similar I'd say.

cowtowncoder added a commit that referenced this issue Nov 21, 2015
@cowtowncoder cowtowncoder added this to the 2.7.0 milestone Nov 21, 2015
@cowtowncoder
Copy link
Member

Improved UUIDDeserializer, EnumDeserializer the way I think they should work. Feel free to open a separate issue for subtype resolution as necessary, ideally showing case(s) where it should be changed wrt format problems, or perhaps by introducing a new sibling exception type. In fact, might make sense to have separate ones for type id resolution problems (unknown type ids) and object id resolution (unknown object id).

@natnan
Copy link
Author

natnan commented Nov 22, 2015

Ah, great! Thank you!. I'll create the issue now for the sake of backlog.

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

No branches or pull requests

2 participants