Do not trigger exceptions / error 500s when endpoints are not found

[abyrd]

These are not really exceptional conditions or errors. They should not be logged as such. For example org.opentripplanner.routing.error.VertexNotFoundException: vertices not found: [to]

Let @trevorgerhardt know when this is fixed so he can adapt his client code.

[JordenVerwer]

What exactly is the issue here? Is it just in the logging, or is it in the API response being returned? Right now the response returned for a VertexNotFoundException is a HTTP status code of 200, with an error id of 400 being included in the body, so I don't see a 500 anywhere. What would have to be adapted in the client code?

Speaking of which, it isn't really clear to me why we have our own set of error codes similar to but distinct from the HTTP status code set. It's pretty confusing to me and I may have made incorrect assumptions about it in the past.

[JordenVerwer]

Also, the whole PlannerError class looks pretty bizarre to me now that I've taken a good look at it.

[abyrd]

Hmm so this is functioning differently than I thought. Due to another conversation I was under the impression that missing endpoints were being reported as HTTP 500 "internal server errors" when they are actually problems with the request. I do see what you mean about simply using the HTTP status codes. Such a change should probably be part of the 1.0 API. Let's draft up a description of how this should look (as a simple comment here) before we implement it.

[JordenVerwer]

I'm not sure if we can use HTTP status codes directly to communicate much, but some codes can be used. Frankly, I don't even know why all these error condition need a code in the first place, given that they already have a name. But let's first have a look at how well the current codes translate to HTTP. Looking at the codes in org.opentripplanner.api.common I'd say there are four categories.

These can just be returned as HTTP status codes as-is:
PLAN_OK (200)*
REQUEST_TIMEOUT (408)*
SYSTEM_ERROR (500)

These contradict the meaning of the equivalent HTTP status code:
PATH_NOT_FOUND (404)
NO_TRANSIT_TIMES (406)
TOO_CLOSE (409)
BOGUS_PARAMETER (413)

These don't even exist as (standard) HTTP status codes:
GEOCODE_FROM_AMBIGUOUS (340)*
GEOCODE_TO_AMBIGUOUS (350)*
GEOCODE_FROM_TO_AMBIGUOUS (360)*
UNDERSPECIFIED_TRIANGLE (370)
TRIANGLE_NOT_AFFINE (371)
TRIANGLE_OPTIMIZE_TYPE_NOT_SET (372)
TRIANGLE_VALUES_NOT_SET (373)
GEOCODE_FROM_NOT_FOUND (440)*
GEOCODE_TO_NOT_FOUND (450)*
GEOCODE_FROM_TO_NOT_FOUND (460)*
LOCATION_NOT_ACCESSIBLE (470)

These require further discussion:
OUTSIDE_BOUNDS (400)
GRAPH_UNAVAILABLE (503)

Values denoted with an asterisk aren't actually in use in the code, according to IntelliJ IDEA.

[abyrd]

Good point, the PlannerError looks like something left over from the early days of OTP that is no longer suitable. It should be changed in the 1.0 API.

I don't think we want to use HTTP status codes (or anything resembling them) to communicate the details of why a request failed, only whether it failed or not. For one thing, they only allow signaling one type of error at a time.

We should get rid of the numeric codes, use the strings themselves as the error codes, possibly just make them into an enum type, and include a set or list of them in the result object.

Then we'd only use HTTP error codes to indicate:
200 request succeeded
400 bad request (including endpoints out of geographic bounds)
408 search timeout
500 uncaught or unrecognized exception on the server side

In the event of a non-200 response we'd always include a collection of errors in the JSON response.

[abyrd]

Equally importantly (and this is the observation that originally led to the creation of this ticket) mistakes made by the client should not cause exceptions to be logged on the server side. So if a misplaced endpoint is handled by throwing and catching an exception (which it arguably should not be) the server should not dump the stack trace and exception for that error to its logs. Logging such common user-facing problems just clutters the logs and confuses the deployer of OTP.

[JordenVerwer]

I'll push the first commit to clear up the unnecessary logging right now, but after that I'll dig through the code to see if there are any other examples that need to be dealt with.

[abyrd]

Also, 404s are being included in the logs. Those should be silenced.

[github-actions]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days