racket-request icon indicating copy to clipboard operation
racket-request copied to clipboard

JSON Requester

Open DarrenN opened this issue 9 years ago • 12 comments

Fixes #12

  • Provides json-requester
  • json-requester returns json-response struct
  • When making requests with a json-requester the following headers are automatically injected: Accept: application/json and Content-Type: application/json. These can be overridden as normal.
  • When making requests with a json-requester the body needs to be a valid jsexpr? and will be automatically converted to a bytes representation of a JSON string (ex: (hasheq 'grand "larceny") -> "{\"grand\": \"larceny\"}")
  • json-response expects the response body to be a valid jsexpr?

DarrenN avatar Aug 28 '16 18:08 DarrenN

Coverage Status

Coverage decreased (-1.5%) to 83.222% when pulling ea650228b1f07f956fe5e96da0473728e4de11a3 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Aug 28 '16 18:08 coveralls

Coverage Status

Coverage decreased (-0.9%) to 83.763% when pulling fd4b7ef497f37d2bfe59aebad94674fcd6b94188 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Aug 28 '16 19:08 coveralls

Looks like some deps didn't install correctly on older versions of Racket. Usually I would restart the travis build but that doesn't seem to be an option on the free version. Hoping it will shake out on future commits pushed up to this PR.

DarrenN avatar Aug 28 '16 19:08 DarrenN

I have some general comments before addressing specifics:

  • I don't think this belongs in base.rkt. That module is for only the minimal API of requesters, as little as possible should go in there. I recommend a private/json.rkt module that exclusively contains the json stuff.
  • Rather than changing any of the call/input-url functions or the http-requester, it would be cleaner to make a json requester a simple wrapper over any requester that works with http-response structs and byte requests.
  • When sending a request with Accept: application/json, there is no guarantee that the response body will actually be json. The server could send back a 406 Not Acceptable response, or it could ignore the request completely. If the response of a json-requester is going to contain the headers and code, even in the case of an error code like 406, then it can't contain the body parsed ahead of time because the body might not be valid json. Not sure how to resolve this, but it might require that json-requester be built on top of http-requester/exn rather than http-requester.

jackfirth avatar Aug 28 '16 23:08 jackfirth

Coverage Status

Coverage increased (+0.7%) to 85.392% when pulling 6114fbc62740cbc586e4df5cec5ee24d4c5933f0 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Sep 04 '16 20:09 coveralls

I think I addressed most of your comments in the recent push:

  • Move code into json.rkt
  • json-requesternow does all its work with simple wrappers around http-requester
  • I just bit the bullet and dispensed with json-response as a concept. json-requester now uses http-requester/exn as its base and throws exceptions on error codes. This means that json-requester only returns the parsed body (jsexpr?) or an exn.

Ideally I would have liked to keep the json-response struct with headers and codes, but as you mentioned it proved difficult when dealing with non-JSON responses. I wonder if there is a middle ground, perhaps keeping json-response with a non-parsed body and creating json-requester/exn that does attempt to parse the body into a jsexpr?.

DarrenN avatar Sep 04 '16 20:09 DarrenN

Went ahead an rolled a new exception (exn:fail:json) that is thrown when json-requester receives invalid JSON as a response. Added an integration test for it.

DarrenN avatar Sep 20 '16 22:09 DarrenN

Coverage Status

Coverage increased (+5.9%) to 90.596% when pulling 84f1237402a701a3e1a487691cfc69635878ffb6 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Sep 20 '16 22:09 coveralls

Coverage Status

Coverage decreased (-0.05%) to 84.636% when pulling 84f1237402a701a3e1a487691cfc69635878ffb6 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Sep 20 '16 22:09 coveralls

Coverage Status

Coverage decreased (-0.07%) to 84.615% when pulling 5572e691f795a30b5dd1b262c266e1b1acde0037 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Oct 03 '16 13:10 coveralls

Coverage Status

Coverage decreased (-0.09%) to 84.595% when pulling 189e96258047360ad5f060f4727ee785e566e995 on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Nov 26 '16 17:11 coveralls

Coverage Status

Coverage decreased (-0.05%) to 84.636% when pulling a351c07461c4b728ed3f031f0acb781ed0d30a7d on DarrenN:json-requester into 49936b297d43021cb3b67c009fcbf950ef32f61d on jackfirth:master.

coveralls avatar Nov 26 '16 18:11 coveralls