[yesod-test] Adds requireJSONResponse function by MaxGabriel · Pull Request #1646 · yesodweb/yesod

MaxGabriel · 2019-11-24T20:55:29Z

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:

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.
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.
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:

Bumped the version number
Documented new APIs with Haddock markup
Added @since declarations to the Haddocks for new, public APIs

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)

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 · 2019-11-24T20:59:51Z

This closes #1643

MaxGabriel · 2019-11-24T21:06:46Z

yesod-test/Yesod/Test.hs

+        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?


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?

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

This seems pretty reasonable

Cool, implemented the body preview

MaxGabriel · 2019-11-24T21:08:14Z

yesod-test/Yesod/Test.hs

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


Copied from acceptsJson, mostly

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

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?

That sounds good

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 · 2019-11-29T15:49:47Z

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)

snoyberg

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

MaxGabriel · 2019-12-01T13:04:00Z

I'll take the release 👍

(This was missing from #1646)

MaxGabriel · 2019-12-01T13:11:54Z

Done 👍

MaxGabriel · 2019-12-01T13:12:33Z

(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)

MaxGabriel added 3 commits November 24, 2019 15:31

..

596db81

..

92afb11

MaxGabriel commented Nov 24, 2019

View reviewed changes

MaxGabriel added 5 commits November 24, 2019 16:11

..

91b7574

Print preview of JSON body in case of parse failure

0025226

..

42d41f7

..

561adc2

..

8ee7718

snoyberg approved these changes Dec 1, 2019

View reviewed changes

MaxGabriel merged commit 3fac351 into master Dec 1, 2019

MaxGabriel added a commit that referenced this pull request Dec 1, 2019

Require latest yesod-core for yesod-test

4f51153

(This was missing from #1646)

jezen deleted the requireJSONResponse branch December 17, 2019 09:26

Conversation

MaxGabriel commented Nov 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGabriel commented Nov 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGabriel commented Nov 29, 2019

Uh oh!

snoyberg left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGabriel commented Dec 1, 2019

Uh oh!

MaxGabriel commented Dec 1, 2019

Uh oh!

MaxGabriel commented Dec 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MaxGabriel commented Nov 24, 2019 •

edited

Loading