Add new top level api function to get the response object of last request

[nmuesch]

This PR adds a new get_http_respone top level function to the api module. The primary goal is to allow users to call this after any other api call to see the response of the previous call. This allows you to inspect things like status_code, headers, etc and currently just returns the requests response - https://2.python-requests.org/en/master/api/#requests.Response

This approach also has the benefit of not adding response metadata to the actual data object returned by the client.

The code would look something like this:

api.Monitor.get_all()
resp = api.get_http_response()

[zippolyte]

LGTM

[mzizzi]

Can we revert this and implement it a bit differently? The method proposed here doesn't work well in multi-threaded environments. Two threads making api calls and attempting to get results via api.get_http_response() will lead to unpredictable results when trying to fetch the last request made.

e.g.

def task1():
    api.Monitor.get_all()
    reponse = api.get_http_response()

def task2():
    api.monitor.search()
    response = api.get_http_response()

t1 = threading.Thread(target=task1)
t2 = threading.Thread(target=task1)

t1.start()
t2.start()
t1.join()
t2.join()

This will be the case for any multi-threaded web server app.If we wanted to support this type of behavior we could use thread locals... But even that would break down if/when async support is added to this SDK.

We should instead modify the SDK to return the response when requests are made.

[nmuesch]

Hey @mzizzi Thanks for the information! When creating this I was aiming to avoid:

1. Breaking backwards compatibility (Which means its difficult to start passing back multiple return values from this method back up the stack to the caller)
2. Start packing the response metadata into the data object. Since the response.content that the submit method returns can either be a list or dictionary, I think its best to not put this response metadata inside that object.

I think we could maybe get the best of all scenarios here by adding a new global variable that is set during initialization of the api client that basically would be used to tell this submit method to return either the full response object back, or continue returning the response's content. That way we can keep backwards compatibility (by keeping the flag set to return the content by default) but also be thread safe since each api call directly returns all the requested data.

What do you think?

[mzizzi]

Breaking backwards compatibility (Which means its difficult to start passing back multiple return values from this method back up the stack to the caller)

Very personal opinion here but I feel that back compat will need to be broken at some point. The SDK, as written, hides some very important information from callers. It doesn't provide a way to get full response objects (needed for headers) and suppresses meaningful error messages and wraps them broad errors of its own.

e.g. 404 errors are swallowed by the SDK, the caller is left to do string compares out of a response dictionary to try and figure out what went wrong.

More thoughts on this here: #408

Start packing the response metadata into the data object. Since the response.content that the submit method returns can either be a list or dictionary, I think its best to not put this response metadata inside that object.

That would work, but I share your feelings about it not being a very clean solution.

I think we could maybe get the best of all scenarios here by adding a new global variable that is set during initialization of the api client that basically would be used to tell this submit method to return either the full response object back, or continue returning the response's content. That way we can keep backwards compatibility (by keeping the flag set to return the content by default) but also be thread safe since each api call directly returns all the requested data.

This is starting to head in the right direction. I'd prefer to go ahead and break back compat and overhaul exception handling at the same time though.

[mzizzi]

Explicitly tagging @zippolyte to get eyes on this again for a revert before the next release is cut for pypi and this becomes permanent.

[zippolyte]

We'll work on replacing this with @nmuesch's suggestion since the proposed solution seems to work for you.
We won't make the full overhaul and the breaking changes right now though.
Will let you know as soon as the PR is open.

[nmuesch]

Hey @mzizzi Here is the new PR based on what we talked about here - #414

I think it's best to avoid breaking backwards incompatibility for now. In the future we may break backwards compatibility via a major version release should we start modifying the exception handling and other pieces of the interface.