Swap json with simplejson

[janmasarik]

Hey,

During implementation of hvac, I encountered issues with getting unicode instead of strings from hvac.Client.read() in python2. This is caused by legacy behaviour of built-in json module which is used for loading the secret.

As json seems as just outdated simplejson (explanation), I think it would make sense to use simplejson instead.

Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 12:39:47)
[GCC 4.2.1 (Apple Inc. build 5666) (dot 3)] on darwin
...
>>> vault.read('secret/db')['data']['data']
{u'host': u'secret', u'db': 1, u'port': 6379}
>>> simplejson.loads(simplejson.dumps(vault.read('secret/db')['data']['data']))
{'host': 'secret', 'db': 1, 'port': 6379}

Basically the issue is:

>>> import json
>>> json.loads(json.dumps("ascii_text"))
u'ascii_text'
>>> import simplejson
>>> simplejson.loads(simplejson.dumps("ascii_text"))
'ascii_text'

This may easily raise issues in python2 as most of the code uses python2 strings ("string_here", not u"string_here"). Example is urllib.parse():

...
  File "/usr/local/lib/python2.7/site-packages/future/backports/urllib/parse.py", line 418, in urljoin
    base, url, _coerce_result = _coerce_args(base, url)
  File "/usr/local/lib/python2.7/site-packages/future/backports/urllib/parse.py", line 115, in _coerce_args
    raise TypeError("Cannot mix str and non-str arguments")
TypeError: Cannot mix str and non-str arguments

I would happily contribute with this bugfix, but I would like to get your green light before following all that struggle mentioned in CONTRIBUTING. :-)

[jeffwecan]

Thanks for the input @janmasarik!

In the case of hvac, I believe the data types within the parsed JSON response from the read() method are ultimately determined by our external requirement in the form of the requests module and that module's conditional logic around choosing json or simplejson as its parser. From your issue's description, I do see where the use of simplejson would offer utility in a general sense. However I'm hesitant to both:

• increase the scope of required modules for hvac in order to address the concern raised here. I've been operating under the belief that the less external dependencies pulled into a module, such as hvac the better). In other words: to meet this stated goal of this issue's description, and with the fact that hvac is ultimately just wrapping some Vault-specific logic on to of python-requests, we would be obligated to pull in simplejson as a requirement of hvac.
• potentially cause unexpected side-effects with folks that have come to rely on the types returned in our current JSON-parsing status quo + Python 2.7 (While this can be called out in a future hvac release's changelogs, that scenario is still a bit more of a "caveat emptor" situation than hvac's maintainer's wish to take lightly.)

That said, I'm willing to track down and confirm or deny those concerns if meeting them gets us closer to a more consistent set of behaviors across Python 2.7 / 3.x. So I'll try to investigate the matter time permitting.

Otherwise, please do feel free to submit any tentative contributions along those lines! The module release runbook steps in our contribution documentation (i.e., the "Package Publishing Checklist" section) are indeed more onerous than need-be at present. However that subset of the documentation is solely centered around handling new releases. I.e., incremental patches / pull requests are welcomed and any finicky bits around releasing those changes can be offloaded to this module's maintainers 😉.

[janmasarik]

Thanks for your extensive response and thanks for the great work you are doing on hvac @jeffwecan !

I haven't dig into requests internals, but you seem to be indeed right that it's the thing here.
I wasn't able to verify this behaviour directly on requests though as it returns unicode even if simplejson should be imported.

Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 12:39:47)
>>> import simplejson
>>> import requests
>>> result = requests.get("https://www.jsonip.com").json()
>>> result
{u'ip': u'127.0.0.1', u'about': u'/about', u'reject-fascism': u'Support Planned Parenthood: https://www.plannedparenthood.org/get-involved', u'Pro!': u'http:https://getjsonip.com'}
>>> simplejson.loads(simplejson.dumps(result))
{'ip': '127.0.0.1', 'about': '/about', 'reject-fascism': 'Support Planned Parenthood: https://www.plannedparenthood.org/get-involved', 'Pro!': 'http:https://getjsonip.com'}  # notice the str instead of unicode

However, I found simplejson issue where this behaviour is actually described as a bug, which makes sense as it doesn't keep the response type consistent as described in docs.

For me, this is still a feature due to backwards compatibility, but I'm really not sure if it's the case with others + both of your points are indeed valid.

Because of this, I would maybe change the subject of the issue to only add a warning about Py2 unicode behaviour. (or not as we should all migrate to Py3 anyway:-)).

What do you think?

[jeffwecan]

A callout in this project's documentation sounds totally reasonable! I'll use this card to track that goal at the very least.