graphhopper / geocoder-converter

Converts arbitrary geocoding responses to a GraphHopper response
https://graphhopper.com/api/1/docs/geocoding/#external-providers
Apache License 2.0
9 stars 11 forks source link

IllegalArgumentException should get BAD_REQUEST code #17

Closed karussell closed 8 years ago

karussell commented 8 years ago

Strange, I thought dropwizard handles IllegalArgumentException already as bad requests (400) but if e.g. the query is empty I get "java.lang.IllegalArgumentException: q cannot be empty" resulting in http code 500

See this docs: http://www.dropwizard.io/0.9.1/docs/manual/core.html#parameters and the "400 Bad Request" comment

karussell commented 8 years ago

Not very high priority of course, just if your "web search foo" is a bit better than mine ;)

boldtrn commented 8 years ago

Ok, I changed it to BadRequestException, which throws a 400, should fix the issue?

karussell commented 8 years ago

I found some custom exception mapper thing: http://stackoverflow.com/q/21482030/194609

karussell commented 8 years ago

Here is an example: https://github.com/dropwizard/dropwizard/pull/1128/files#diff-7f3579c0d8c8333ddb86abd71486b338R42

karussell commented 8 years ago

But probably easier like you solved it.

karussell commented 8 years ago

The only minor advantage of an exception mapper would be that code upstream with IllegalArgExceptions would also properly fail. But I do not see this yet.

karussell commented 8 years ago

Here is a blogpost about it: http://thoughtspark.org/2013/02/25/dropwizard-and-jersey-exceptionmappers/

boldtrn commented 8 years ago

The only minor advantage of an exception mapper would be that code upstream with IllegalArgExceptions would also properly fail.

When thinking about this, I would not want a user to see upstream error messages, due to security reasons. But in general I am fine with creating an Exceptionmanager.

Should we reopen this issue?

karussell commented 8 years ago

Ah, yes. You are right, this is indeed a security thing and we should keep it as you did it.