Skip to content

Throw error on query parameter parse failure #649

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

Closed
wants to merge 2 commits into from

Conversation

Philonous
Copy link
Contributor

@Philonous Philonous commented Dec 12, 2016

QueryParam and QueryParams pass Nothing to the handler when parsing the parameters fails. This is counter-intuitive and IMO makes the parsing functionality almost useless.

This pull-request changes the HasServer instance to throw error400 when parsing fails. In case where parse failures are to be dealt with gracefully the user can define a new type:

data MayFail a = Success a
               | Failed Text Text -- Original Text and error message

instance FromHttpApiData a => FromHttpApiData (MayFail a) where
  parseQueryParam txt =
    Right $ case parseQueryParam txt of
              Left e -> Failed txt e
              Right r -> Success r

or similar or just take Text parameters directly.

(Maybe servant should provide this for convenience?)

Related to #256

Technical Details:

  • I added a new serverD block to the Delayed type because none of the existing blocks seemed to be appropriate. The block is executed last, even after bodyD because this is the only way to guarantee that 400 has the least priority (bodyD can also return 406 and 415)

RFCs:

  • When returning errors in the 400 range, the HTTP spec recommends :

The 4xx class of status code is intended for cases in which the client seems to have erred. Except when responding to a HEAD request, the server SHOULD include an entity containing an explanation of the error situation, and whether it is a temporary or permanent condition. These status codes are applicable to any request method. User agents SHOULD display any included entity to the user.

And of course this would greatly aid debugging of requests, but I left this as a TODO because I wasn't sure what format to put the error messages in and whether to include the parse errors.

  • Parse error propagation for Headers could use the same Delayed block, but this isn't implemented in this PR (yet?). In this case it might be better to rename it to something different than paramD. Suggestions?

@Philonous
Copy link
Contributor Author

Philonous commented Dec 12, 2016

Travis seems to fail because of -Wall + -Werror on warnings I didn't introduce here. Can I work around this?

@phadej
Copy link
Contributor

phadej commented Jan 16, 2017

I'm 👍 to this change. Others, @haskell-servant/maintainers ?

@fizruk
Copy link
Member

fizruk commented Jan 16, 2017

I'm 👍, but TODOs have to be figured out before merge.

@phadej
Copy link
Contributor

phadej commented Jan 16, 2017

Superseded by #670

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

Successfully merging this pull request may close these issues.

3 participants