Make returned responses more consistent

[llamasoft]

This is another big PR so apologies in advance. This PR fixes issue #525 and arises from the discussion therein.

This PR does one thing: create a new JSONAdapter and makes it the default. The "JSON adapter" works just like the previous "Request" adapter except the return value uses the following logic:

if response.status_code == 200:
    try:
        return response.json()
    except ValueError:
        pass
return response

The net result is that all client functions now have consistent logic regarding what they return.
As a result, all instances of the following client logic have been removed:

return response.json()

and

if response.status_code == 204:
    return response
else:
    return response.json()

This will likely cause some breaking changes for some users, especially with regards to the direct client.[read,list,write,delete] methods as they will no longer throw a ValueError from assuming the response always contains a JSON body.

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update some of our documentation based on your changes.
See: https://github.com/hvac/hvac/blob/develop/CONTRIBUTING.md#documentation

[codecov-io]

Codecov Report

Merging #537 into develop will decrease coverage by 0.48%.
The diff coverage is 79.15%.

@@             Coverage Diff             @@
##           develop     #537      +/-   ##
===========================================
- Coverage    83.03%   82.54%   -0.49%     
===========================================
  Files           55       54       -1     
  Lines         2976     2859     -117     
===========================================
- Hits          2471     2360     -111     
+ Misses         505      499       -6

Impacted Files	Coverage Δ
hvac/api/system_backend/capabilities.py	21.42% <0%> (+1.42%)	⬆️
hvac/api/auth_methods/ldap.py	100% <100%> (ø)	⬆️
hvac/api/system_backend/audit.py	94.73% <100%> (-0.72%)	⬇️
hvac/api/system_backend/init.py	56.25% <100%> (-2.58%)	⬇️
hvac/api/secrets_engines/aws.py	97.87% <100%> (-0.31%)	⬇️
hvac/api/system_backend/wrapping.py	100% <100%> (ø)	⬆️
hvac/api/system_backend/seal.py	100% <100%> (ø)	⬆️
hvac/api/system_backend/policy.py	100% <100%> (ø)	⬆️
hvac/api/secrets_engines/azure.py	100% <100%> (ø)	⬆️
hvac/api/system_backend/health.py	85.71% <100%> (-1.79%)	⬇️
... and 60 more

---

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 15df074...7ebfacd. Read the comment docs.

[llamasoft]

I just had the last-second realization that the login method on the Adapter base class doesn't make sense. The request method it depends on is abstract, so we have zero guarantees about the object type it returns. Instead, I've moved the token extraction logic to a separate abstract method, get_login_token.

I also forgot to mention in the original PR that I added an adapter option called ignore_exceptions that optionally overrides the raise_exception parameter passed to request. This allows a way to suppress exceptions and receive a raw Response object (normally not possible because the raise_exception argument isn't accessible).

[llamasoft]

I just realized an additional breaking change regarding this PR: the lookup_* methods (e.g. client.secrets.identity.lookup_[entity|group]) now return a Response[204] (truthy) instead of None (falsy) if the lookup returned no results.

Unfortunately, I can't think of a good way to resolve this issue. The Requests Adapter is created from a user-specified class so we can't assume in advance what sort of data type it will return.
Honestly, given that we can't guarantee return types, we probably shouldn't be interacting with response objects at all, including the helpers on read_* methods that return response.get('data').

What are your thoughts on this matter? I can revert the lookup_* to return None on 204 responses, but that logic may end up causing issues with custom adapters.

[drewmullen]

since we use this library for downstream ansible modules, i was mostly concerned with the expected return values; whether or not they change based on the new json adapter. based on initial tests i dont see breaking changes happening to https://github.com/TerryHowe/ansible-modules-hashivault

based on this consideration, this PR has my 👍 . it should definitely be minor or possibly major release though

    print(client.auth.azure.read_config())
    # prev: {'client_id': 'test', 'environment': 'AzurePublicCloud', 'resource': 'test', 'tenant_id': 'test'}
    # new:  {'client_id': 'test', 'environment': 'AzurePublicCloud', 'resource': 'test', 'tenant_id': 'test'}

the following outputs are identical except for request_id (expected)

new `sys.list_auth_method` output

#list_auths NEW ''' { "token/": { "accessor": "auth_token_8e77bdb2", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "token based credentials", "local": false, "options": null, "seal_wrap": false, "type": "token", "uuid": "9aae3834-9e4e-3c3c-a891-d34b49f5dabd" }, "azure/": { "accessor": "auth_azure_2154cde7", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "", "local": false, "options": null, "seal_wrap": false, "type": "azure", "uuid": "504b59a1-7d52-fbd1-082f-c1625b165e58" }, "request_id": "62e1e72f-ebf0-0bde-1ef8-f5696135a07f", "lease_id": "", "renewable": false, "lease_duration": 0, "data": { "azure/": { "accessor": "auth_azure_2154cde7", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "", "local": false, "options": null, "seal_wrap": false, "type": "azure", "uuid": "504b59a1-7d52-fbd1-082f-c1625b165e58" }, "token/": { "accessor": "auth_token_8e77bdb2", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "token based credentials", "local": false, "options": null, "seal_wrap": false, "type": "token", "uuid": "9aae3834-9e4e-3c3c-a891-d34b49f5dabd" } }, "wrap_info": null, "warnings": null, "auth": null } '''

previous `sys.list_auth_method` output

#list auths OLD ''' { "token/": { "accessor": "auth_token_8e77bdb2", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "token based credentials", "local": false, "options": null, "seal_wrap": false, "type": "token", "uuid": "9aae3834-9e4e-3c3c-a891-d34b49f5dabd" }, "azure/": { "accessor": "auth_azure_2154cde7", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "", "local": false, "options": null, "seal_wrap": false, "type": "azure", "uuid": "504b59a1-7d52-fbd1-082f-c1625b165e58" }, "request_id": "7797ff7c-0fcf-8469-083d-146f1cb0d7b9", "lease_id": "", "renewable": false, "lease_duration": 0, "data": { "azure/": { "accessor": "auth_azure_2154cde7", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "", "local": false, "options": null, "seal_wrap": false, "type": "azure", "uuid": "504b59a1-7d52-fbd1-082f-c1625b165e58" }, "token/": { "accessor": "auth_token_8e77bdb2", "config": { "default_lease_ttl": 0, "force_no_cache": false, "max_lease_ttl": 0, "token_type": "default-service" }, "description": "token based credentials", "local": false, "options": null, "seal_wrap": false, "type": "token", "uuid": "9aae3834-9e4e-3c3c-a891-d34b49f5dabd" } }, "wrap_info": null, "warnings": null, "auth": null } '''

great work @llamasoft and thanks for the great contribution

[llamasoft]

Any word on if response.get('data') should be removed as well? It's used in a few places, but not in others. In my personal experience, it made return values unpredictable. For example, most read_* methods use it, but lookup_* methods don't.

Unfortunately, removing them will be a breaking change for some users.

[jeffwecan]

Any word on if response.get('data') should be removed as well?

I suppose I feel it makes most sense to return the entire response in all cases. In general, this module is intended to be a pretty basic wrapper for Vault API (at least as far as I'm concerned).

[drewmullen]

I agree with @jeffwecan . we had explored the idea of making all returns just the data portion but there are some useful values provided from vault that shouldnt be stripped (see below).

#483 (comment)

[Lucas-C]

This looks like an nice improvement !
What is the status of this PR ? :)

[jeffwecan]

@Lucas-C: I figure it is about time to get this PR put in place! :D

Will start working on getting it trued back up with the current state of the develop branch...

[jeffwecan]

Planning on merging this in today. To close the loop on returning the entire response dicts versus only the data key: I still want to see use move to the former in all places eventually. However I'd rather tackle that in another PR / release since its most certainly a breaking change (as @llamasoft alluded to).

[jeffwecan]

[trishankatdatadog]

Okay, this one was really confusing, because the documentation, which really copies doctstrings from the source code, still says to expect requests responses instead of the JSON body for non-204 responses. Should we update the docstrings?

[jeffwecan]

I would imagine so, and sorry for the added confusion! @trishankatdatadog, I went ahead and quoted your comment into a new issue (#537) and will try to get a review of docstrings, etc. done, and any incorrect return types updated, sometime over the next week or so. (Though contributions along those lines are of course also very welcomed. ☺)

[trishankatdatadog]

I would imagine so, and sorry for the added confusion! @trishankatdatadog, I went ahead and quoted your comment into a new issue (#537) and will try to get a review of docstrings, etc. done, and any incorrect return types updated, sometime over the next week or so. (Though contributions along those lines are of course also very welcomed. ☺)

Thanks! Also wrong link for issue 🙂