consider HTTP 400 over 404 for pathVar parse failures

[aryairani]

Given: https://gist.github.com/refried/0eb77004d2f41c470e71

curl -v localhost:8081/puppy/bySSN/foo returns 404 with the body /puppy/bySSN/foo not found, which made me think something was wrong with my route, when it was really something wrong with my request (parse failure).

I think there's an argument to be made for 404, or a different code, or a different body, or maybe the answer is documentation on pathVar. What do you think?

[LeifW]

I'd like to see a 400 Bad Request: foo is not in a format valid for a SSN: xxx-xx-xxxx in response to this, ideally.

[bryce-anderson]

Documentation is certainly a thing we can improve all around.
I think there is a case to be made for a situation like this but it requires a bit of work in the PathTree. In particular, the matchString method is responsible to checking/parsing the route. Right now it returns a HList of successful parameters on a successful match, or null. I think this could be replaced with a RouteResult[HList] without sacrificing performance and allowing the route to continue to 'fall through' while preserving the fact that one failed and perhaps returning a 400 Bad Request.

On the other hand, when I think of a route I think that a path should either match or not, and I'd preserve the 'Bad Request' for a query or headers based parameter. I'm willing to have strong opinions about it one way or the other?

[LeifW]

Maybe the 400 is more appropriate for improperly formatted query params, not routes that we've dictated must be parseable as a some kind of data type?

[bryce-anderson]

@LeifW, that my knee jerk feeling, but I'm far from an expert.

As a counter example, I think a more traditional framework would offer up the strings and it would be up to the route to determine if they are valid or not, in which case they would probably return a 400.

Any thoughts?

[LeifW]

I asked around. "RESTfulness calls for a 404", since a non-404 response would imply the existence of a resource there. I think I see that behavior elsewhere. Maybe if you want better error messages on these kind of keys, have them as a query param, instead? :/
But, maybe we could still make the body of the 404 more descriptive: "close, but no cigar"?

[aryairani]

Maybe I'm relatively new to REST, I would love to see more references on what kinds of things should be path params vs query params -- but "query params are used when you want more detailed errors" doesn't scratch the itch for me :-)
My gut sort of says "query params are optional params", but I know that isn't quite right either.

Not sure if there'd be a general way to say "404 because this happened during parsing". And even if there were a way, I don't know if it would be a good idea in general. But maybe during development? But supporting multiple modes creates even more work!

[aryairani]

I just realized I don't care at all what the HTTP code is, I just wished for debugging clues.

[bryce-anderson]

Maybe this is a good case for simply adding some debug logging during path evaluation?

[aryairani]

That could work. Are there already some logging facilities to plug into?

I mean, a partial solution could also just be "remember how path var parsing works, now that you know", but if there's an easy way to get warnings, why not.

[bryce-anderson]

I don't believe the PathTree type has any logging in it yet. I don't see a reason not to add it. We've been using log4s which has been really nice.