JSON custom default and object hook

[jhildreth]

fixes #1192

Here's my first attempt at 1192. I made my best guess as to where to document these changes...

A couple questions:

• In the updated documentation, should we link to the Python json docs as in the initial issue, or is what is here sufficient?
• Is there somewhere else the SimpleTestObject class should live, given that it's duplicated in both test_request_media.py and test_response_media.py?

[jhildreth]

Ah, looks like ujson does not support the object_hook parameter on ujson.loads or the default parameter on ujson.dumps.
They have a closed issue that points out the lack of support for object_hook, but no resolution. There is an open issue that also refers to the lack of support for default.
I'm not sure how it would be best to address this.

[jhildreth]

So, I've thought about a few different ways to handle the fact that ujson doesn't support these kwargs, and locally I've implemented what I feel like is the best idea I've come up with. I'm checking once if ujson is installed when the JSONHandler is instantiated, using pkgutil.find_loader. I didn't expect that this would introduce a lot of performance overhead. I'm then conditionally setting the contents of a couple of dictionaries in JSONHandler, which can be unpacked in the call to json.dumps in serialize and the call to json.loads in deserialize (so that the kwargs default/object_hook are not passed if ujson is installed).

I'm also raising an exception if either argument is specified when JSONHandler is instantiated if ujson is installed (so that there would be an early indication of a problem, rather than the parameter(s) being silently ignored by JSONHandler).

I've also modified the tests I wrote to expect these exceptions when ujson is installed - having the tests set up like this accomplishes what I'm going for, but feels just a little funny.

I plan to think about this a little bit more before pushing up these changes. I'm certainly open to other suggestions!

[codecov]

Codecov Report

Merging #1202 into master will decrease coverage by 0.16%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1202      +/-   ##
==========================================
- Coverage     100%   99.83%   -0.17%     
==========================================
  Files          38       38              
  Lines        2484     2494      +10     
  Branches      364      366       +2     
==========================================
+ Hits         2484     2490       +6     
- Misses          0        2       +2     
- Partials        0        2       +2

Impacted Files	Coverage Δ
falcon/media/json.py	84.61% <66.66%> (-15.39%)	⬇️
falcon/media/msgpack.py	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6532ad0...bfc013d. Read the comment docs.

[jhildreth]

I'm happy to work on the test coverage a bit, but I think I'll wait for a little feedback on if I'm even headed in the direction we want to go, first. :-)

[jhildreth]

Hmm, still struggling with coverage on the exceptions that fire if setting default or object_hook when ujson is installed.

I tried using unittest.mock.patch to make it look as though ujson is installed when the tests are run, to force execution down that path and test that the exceptions are raised - but I forgot that mock isn't available in some of the earlier supported Python versions.

Not sure if I'm overlooking something here, or if taking a different tack would be appropriate. I'll give it some more thought - and I remain very open to feedback!

[kgriffs]

Hi @jhildreth, thanks for your patience. I think that if we implement #1206 it will make your work a lot easier. We can stipulate that if a third-party json library is configured, it must support the hooks in question in order to use this feature.

[jhildreth]

Hi @kgriffs, sure thing! I'm sorry, I've sort of let this drop off my radar lately. I agree that addressing #1206 first would help a lot - I'll keep my eye on that one. I like the idea of stipulating third-party libraries must support these hooks.

[kgriffs]

Thanks for your work on this! It really helped us compare and contrast the different ways we could approach this problem. That being said, after discussing this with the maintainers team we ultimately decided to go with #1404. If you have a minute to take a look and provide your feedback, that would be great.

[kgriffs]

[superseded by #1404]

[jhildreth]

Sure thing! I glanced at #1404, but honestly it's been a little while since I looked at this code, so I'm not as familiar with the code as I could be. It looks like a lot of good thought has gone into it!