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

[yesod-test] Adds requireJSONResponse function #1646

Merged
merged 8 commits into from Dec 1, 2019
Merged

Conversation

@MaxGabriel
Copy link
Member

MaxGabriel commented Nov 24, 2019

This function checks that a response body is JSON, and parses it into a Haskell value. Having something like this function is pretty essential to using Yesod as a JSON API server, so I think it's a good addition. You can use it to parse a Haskell record directly (usually by adding FromJSON classes to your response types), or parse a Value and pull out individual fields, maybe using something like aeson-lens (though probably a testing-specific library would be better).

I debated over these things:

  1. The name. I was thinking of something like [assert/require/decode/parse]JSON[Response/Body]. I ultimately went with requireJSONResponse:
    • decode/parse sound like the aeson functions that return Either or Maybe, and I wanted this function to throw an error if it failed
    • I'm open to using assertJSONResponse—it matches the other functions (assertEq) better—but I think it reads less like English.
    • I chose Response over Body because (a) It also checks the content-type header, which is not in the body (b) "Body" felt slightly in-the-weeds of HTTP; I think "response" is more approachable.
    • Edit: Oh, just realized the function on the serve is called requireJsonBody. That kind of rules out that name to avoid duplicate identifier issues.
  2. Should it require the JSON content type? You can definitely have a server that returns JSON without JSON content types, but I think that's a such a bad idea, it's more likely requiring it helps people if they accidentally don't add the header.
  3. Should it take a String parameter to add to the error message? This would match assertEq, but other functions like statusIs don't take a message. Ultimately I went without it, because the messages felt like I was repeating myself: (comment :: Comment) <- requireJSONResponse "the response has a comment"

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)
MaxGabriel added 3 commits Nov 24, 2019
This function checks that a response body is JSON, and parses it into a Haskell value. Having something like this function is pretty essential to using Yesod as a JSON API server, so I think it's a good addition. You can use it to parse a Haskell record directly (usually by adding FromJSON classes to your response types), or parse a Value and pull out individual fields, maybe using something like `aeson-lens` (though probably a testing-specific library would be better).

I debated over these things:

1. The name. I was thinking of something like [assert/require/decode/parse]JSON[Response/Body]. I ultimately went with requireJSONResponse:
	- decode/parse sound like the aeson functions that return Either or Maybe, and I wanted this function to throw an error if it failed
	- I'm open to using `assertJSONResponse`—it matches the other functions (`assertEq`) better—but I think it reads less like English.
	- I chose Response over Body because (a) It also checks the content-type header, which is not in the body (b) "Body" felt slightly in-the-weeds of HTTP; I think "response" is more approachable.
2. Should it require the JSON content type? You can definitely have a server that returns JSON without JSON content types, but I think that's a such a bad idea, it's more likely requiring it helps people if they accidentally don't add the header.
3. Should it take a String parameter to add to the error message? This would match `assertEq`, but other functions like `statusIs` don't take a message. Ultimately I went without it, because the messages felt like I was repeating myself: `(comment :: Comment) <- requireJSONResponse "the response has a comment"`
..
..
@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Nov 24, 2019

This closes #1643

isJSONContentType
(failure $ T.pack $ "Expected `Content-Type: application/json` in the headers, got: " ++ show headers)
case eitherDecode' body of
-- TODO: include full body in error message?

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 24, 2019

Author Member

Could do this if there's a failure. I think it would mostly be quite useful, but would be large and sometimes extremely large (e.g. Base64 encoded images). Thoughts?

This comment has been minimized.

Copy link
@snoyberg

snoyberg Nov 25, 2019

Member

How about including the first 256 (arbitrary number) bytes?

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 25, 2019

Author Member

This seems pretty reasonable

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 29, 2019

Author Member

Cool, implemented the body preview

requireJSONResponse = do
withResponse $ \(SResponse _status headers body) -> do
let mContentType = lookup hContentType headers
isJSONContentType = maybe False (\contentType -> BS8.takeWhile (/= ';') contentType == "application/json") mContentType

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 24, 2019

Author Member

Copied from acceptsJson, mostly

This comment has been minimized.

Copy link
@snoyberg

snoyberg Nov 25, 2019

Member

Would it work to use acceptsJson here? That would be preferable, since the set of acceptable JSON mime types may expand in the future.

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 25, 2019

Author Member

acceptsJson is MonadHandler, so we couldn't just drop it in here I don't think, but I could create a new function exposed from yesod-core that takes a list of headers ([(CI ByteString, ByteString)], aka RequestHeaders) and returned if one of the was a JSON content type?

This comment has been minimized.

Copy link
@snoyberg

snoyberg Nov 26, 2019

Member

That sounds good

This comment has been minimized.

Copy link
@MaxGabriel

MaxGabriel Nov 29, 2019

Author Member

Realized that the Accept header is slightly different, since it has a list of preferred MIME types. I added a contentTypeIsJson function for general use on Content-Type.

MaxGabriel added 5 commits Nov 24, 2019
..
..
..
..
@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Nov 29, 2019

Ok I think this is ready for final review. One last thing is should the function be renamed requireJsonResponse? This matches Yesod.Core.Json's naming scheme, but not aeson's functions (parseJSON)

Copy link
Member

snoyberg left a comment

LGTM! Want me to merge or release, or you want to?

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Dec 1, 2019

I'll take the release 👍

@MaxGabriel MaxGabriel merged commit 3fac351 into master Dec 1, 2019
18 checks passed
18 checks passed
yesodweb.yesod Build #20191129.7 succeeded
Details
yesodweb.yesod #20191129.8 succeeded
Details
yesodweb.yesod (Linux nightly) Linux nightly succeeded
Details
yesodweb.yesod (Linux stack-lts-11) Linux stack-lts-11 succeeded
Details
yesodweb.yesod (Linux stack-lts-12) Linux stack-lts-12 succeeded
Details
yesodweb.yesod (Linux stack-lts-14) Linux stack-lts-14 succeeded
Details
yesodweb.yesod (Linux stack-lts-9) Linux stack-lts-9 succeeded
Details
yesodweb.yesod (Linux stack-persistent-2-10) Linux stack-persistent-2-10 succeeded
Details
yesodweb.yesod (Linux stack-persistent-2-9) Linux stack-persistent-2-9 succeeded
Details
yesodweb.yesod (Windows stack-lts-11) Windows stack-lts-11 succeeded
Details
yesodweb.yesod (Windows stack-lts-12) Windows stack-lts-12 succeeded
Details
yesodweb.yesod (Windows stack-lts-14) Windows stack-lts-14 succeeded
Details
yesodweb.yesod (macOS stack-lts-11) macOS stack-lts-11 succeeded
Details
yesodweb.yesod (macOS stack-lts-12) macOS stack-lts-12 succeeded
Details
yesodweb.yesod (macOS stack-lts-14) macOS stack-lts-14 succeeded
Details
yesodweb.yesod (macOS stack-lts-9) macOS stack-lts-9 succeeded
Details
yesodweb.yesod (macOS stack-persistent-2-10) macOS stack-persistent-2-10 succeeded
Details
yesodweb.yesod (macOS stack-persistent-2-9) macOS stack-persistent-2-9 succeeded
Details
MaxGabriel added a commit that referenced this pull request Dec 1, 2019
(This was missing from #1646)
@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Dec 1, 2019

Done 👍

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Dec 1, 2019

(note: this PR was missing a requirement from yesod-test that it was getting the latest yesod-core for the new content type check. I pushed that directly to master)

@jezen jezen deleted the requireJSONResponse branch Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.