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

Json matcher doesn't accept correct header #46

Closed
Philonous opened this issue Nov 3, 2017 · 7 comments
Closed

Json matcher doesn't accept correct header #46

Philonous opened this issue Nov 3, 2017 · 7 comments
Labels

Comments

@Philonous
Copy link
Contributor

I'm trying to test a REST api written in servant. To that end I have a test like this:

get "/users" `shouldRespondWith` [json|[{"name":"bob"}]|]

That test fails with

  1) GET /users responds with [User]
       missing header:
         Content-Type: application/json
       the actual headers were:
         Content-Type: application/json;charset=utf-8

The actual header is compatible with the required one, so this shouldn't fail.

@sol sol added the invalid label Nov 3, 2017
@sol
Copy link
Member

sol commented Nov 3, 2017

@Philonous As I understand it application/json; charset=utf-8 is non-standard. See also #40 and elsewhere on the Internet.

@sol
Copy link
Member

sol commented Nov 5, 2017

@Philonous if you want to test your current behavior, you can do this with:

get "/users" `shouldRespondWith` [json|[{"name":"bob"}]|] {matchHeaders = ["Content-Type" <:> "application/json; charset=utf-8"]}

@sol
Copy link
Member

sol commented Nov 5, 2017

@jkarni any opinion on this. Have you had a discussion on what the right thing for servant is before?

@Philonous
Copy link
Contributor Author

Servant seems to implement it on purpose to appease other clients (see haskell-servant/servant#849).

Also, the JSON rfc remarks that although it isn't defined, it should be ignored by compliant recipients:

Note: No "charset" parameter is defined for this registration.
Adding one really has no effect on compliant recipients.

So I think this should be fixed here and have implemented a patch: #47

@sol
Copy link
Member

sol commented Nov 8, 2017

I added a lengthy source code comment in 791dcbb that elaborates my view on this matter.

@sol
Copy link
Member

sol commented Nov 8, 2017

Fixed.

@sol sol closed this as completed Nov 8, 2017
@Philonous
Copy link
Contributor Author

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants