Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add AdapterResponse types, and HvacAdapter #898

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

briantist
Copy link
Contributor

@briantist briantist commented Sep 18, 2022

Closes #797
Closes #293
Closes #385
Closes #660

This started after discussion about how we might handle 204 responses better in the JSONAdapter (see also #847), but led to us thinking about it in a new way that's a bit more comprehensive.

The basic idea is to return a specialized response class instead of raw data in some form or another.

The first members of this class are raw, status, and value, where status is meant to represent the HTTP status code returned from the request, raw is meant to be the raw response object (whatever it may be) or possibly None (this is Adapter-dependent), and value is meant to be the "usable" data/result of the request (also Adapter-dependent).

This is implemented here first as an abstract base class for adapter responses, an abstract subclass that is tuned for requests.Response-based response classes, and finally the HvacAdapterResponse which is first concrete response class.

HvacAdapterResponse is nominally very simple: it inherits from the RequestsAdapterResponse and only needs to override value; it takes similar steps to the JSONAdapter of trying to interpret the HTTP response as JSON, with an extra step for handling empty 204 responses. On a 204, an empty dict {} is returned.

If the response in not parseable as JSON, and the response code is not 204, then it returns None. If the caller knows how to handle the particular response, they can use the .raw member to get access to the response object and handle it however they wish.

HvacAdapterResponse includes an additional set of deprecated behaviors: it overrides __getattr__, __getitem__, __setitem__, and __delitem__, and proxies them to the value member instead. The purpose of this is so that code like resp = client.secrets.kv2.read_secret_version(...) which assumes that resp is a dict, can still work if resp is a HvacAdapterResponse; essentially letting consumers still treat it like a dict for a while.

Each time this happens, a deprecation warning will be shown. Consuming code should change their calls like resp.get('key', None) or resp['key'] to resp.value.get('key', None) or resp.value['key'], etc.


The next step is introducing a new Adapter, called the HvacAdapter. It operates rather transparently, based on RawAdapter, and simply returns an HvacAdatperResponse based on the requests.Response from the RawAdapter.


Basic (tentative) plan for introduction and use

v1.x

  • Introduce these response types and new Adapter
  • Update documentation and examples to show/recommend its use
  • When a client is created with no explicit Adapter, still default to JSONAdapter but show a warning that the default will become HvacAdapter in v2.0.0
  • Clients who explicitly want to retain JSONAdatper functionality can now explicitly specify it, and the warning will go away. JSONAdapter is not deprecated and their code will work indefinitely.

v2.0.0

  • Change the default Adapter when none is chosen to HvacAdapter. Responses are now of type HvacAdapterResponse
  • Clients who ignored the warnings/recommendations about the default change should still work due to the machinery in the response that allows to simulate a dict, but all of those accesses should be spitting out deprecation warnings now.
  • To make the warnings go away, clients may update their code to use the .value member, or may choose to explicitly set the JSONAdapter (or another one of their choosing).

v3.0.0

  • Remove all of the fallback overrides that allows for treating the response as a dict.

This gives a lot of advance warning of the changes coming, and provides a lot of overlapping functionality so that downstream projects have time to support both ways so that their consumers can still use a wide range of hvac versions.

Opting into HvacAdatper is available from the beginning; opting out to stick with JSONAdapter is also available from the beginning.


This is a draft, still WIP, and is subject to change.

@briantist briantist added the enhancement a new feature or addition label Sep 18, 2022
@codecov
Copy link

codecov bot commented Sep 18, 2022

Codecov Report

Merging #898 (58a2155) into main (31aca14) will increase coverage by 0.50%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #898      +/-   ##
==========================================
+ Coverage   81.98%   82.48%   +0.50%     
==========================================
  Files          65       65              
  Lines        3019     3089      +70     
==========================================
+ Hits         2475     2548      +73     
+ Misses        544      541       -3     
Impacted Files Coverage Δ
hvac/adapters.py 93.71% <100.00%> (+4.19%) ⬆️

... and 1 file with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a new feature or addition
Projects
None yet
1 participant