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

Return 400 if required query parameter is missing #251

Open
paul-lysak opened this issue Sep 28, 2018 · 4 comments
Open

Return 400 if required query parameter is missing #251

paul-lysak opened this issue Sep 28, 2018 · 4 comments

Comments

@paul-lysak
Copy link

If method and path in request match some route but don't contain mandatory query parameters I would expect a 400 error response rather then attempts to match subsequent routes. Otherwise, it's hard to do error handling properly.
This gist gives an example of what I mean: https://gist.github.com/paul-lysak/c3305c5bb0771fd97e9f4e7041dabfcb
It's possible to emulate such behavior by adding one more route with the same path and without query parameters,
but it comes at a cost:

  • it won't be able to tell which query parameters are missing if there are multiple query parameters
  • generated Swagger spec won't contain any definitions for query parameters - it describes only the "fallback" route

This is somewhat similar to #55.

@chuwy
Copy link
Contributor

chuwy commented Sep 30, 2018

I'd expect exactly this behavior: in the first route we stated that service needs a barId param, then client didn't provide it and that's why URL was not matched. I think if /foo/bar needs to return 400 then we have to handle it explicitly: make barId optional and check if it is provided/valid or return 400 otherwise.

@paul-lysak
Copy link
Author

@chuwy the swagger spec would be incorrect then which undermines the whole point of using the Rho - a required parameter will be indicated as optional there.

@chuwy
Copy link
Contributor

chuwy commented Oct 1, 2018

I think we need to decide here if we want to:

  1. Match it and return 400 (with optional parameter)
  2. Not match it (and have required parameter)

Both options look valid to me when they're explicitly stated by developer. But maybe this is some kind of personal preference, I'd like to hear other people's opinion.

@zarthross
Copy link
Member

zarthross commented Oct 7, 2018

I suspect we should only be matching on the path and not on the query parameters, and letting the route fail if required parameters aren't provided. Unfortunately we can't (currently) prevent users from accidentally writing 2 routes with the same path but different query strings matchers, so users would only ever see the first fail.

I really wonder what the original design choice was here and why?

Any thoughts @bryce-anderson or @rossabaker?

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

3 participants