API endpoint methods should return response objects

[Quantra]

Issue:

Endpoint methods currently return the JSON result from the API as a python object.

This means all other information regarding the response is lost, such as the status code. It also means the return type is unknown as it can sometimes be a list and other times a dict.

This leads to the issue that the only way to determine if a response was a 400 or 200 is by inspecting the returned result.

Proposed solution:

Return response objects with either the content attribute decoded and replaced in place or with another attribute result with the decoded JSON. the decoded json content is available with response.json()

I am happy to make a PR for this but it's a backwards incompatible change!

Perhaps there is a way to make it backwards compatible by returning an object of a custom class that behaves as the list/dict but with extra attributes for the response object. But this sounds complicated =]

[yqdv]

@Quantra
First, I apologize for missing these two issues you opened almost six months ago. I just noticed them now while working on a feature request.

One of the basic design decisions that we made when designing the library was that we were going to operate using normal python objects (dicts and lists) rather than special API classes with associated behavioral methods. This makes it much easier to interoperate with raw API requests where necessary.

That design decision was primarily focused on how items are manipulated - for example, using opalapi.apps.delete_one({'id': something}) instead of some_app_instance.delete(). Unfortunately, that goal of using only basic python objects at the input/output boundary also results in the undesirable behavior you've identified here.

Your proposed solution seems correct. We would refactor and redefine the API call outputs to return the requests response object directly, rather than trying to shield the user from the foreign object.

This can be implemented. As it is a breaking change, it will require perhaps a major version bump. We'll make a test case to illustrate the difference and discuss it internally.

[Quantra]

@yqdv no problem! I have been guilty of the same more than once.

With the power of hindisght I think your design decision has merit. For my project I wrote a small API client which returned response objects. Because I'm using Celery I then need to perform some processing of the response object in order for it to be JSON serializable. So in this case having a dict returned from the API would have been beneficial.

So perhaps it would be better to stay with a dict but expand it to include more data as this would fit better with your design?

This is the dict I save in each task:

        data = {
            "status_code": response.status_code,
            "url": response.url,
            "text": response.text,
            "reason": response.reason,
            "data": response.json(),
        }

And here is the client I wrote: https://gist.github.com/Quantra/5aba08620cb85c0d6fa3896232f1cb46

Whilst my client is somewhat stringly typed it is suitable for my needs and I have no intention to replace it. So on that basis please feel free to close out this issue and issue #11.

Briefly my project and use case is a larger management system that has an Opalstack dashboard component where I can manage multiple accounts connected via API keys. If it would be beneficial to you/Opalstack or just of interest to know more please message/email directly =]

[yqdv]

@Quantra Thanks a lot for your comment; it elucidates the motivation for your approach in a practical way.

The more I think this over, the more I am coming to the conclusion that we don't want a breaking change (major version) and that the functions should return the simple python objects in the way they do today. So, what we really need is just a way to get access to the raw response object; the user could then reference response.status_code, response.reason, etc as needed.

One thing to expose is the ensure_status parameter on these calls. By default, a request expects a successful API response, but by passing ensure_status=[] it essentially says "any status code in the response is OK", avoiding the raised exception. Then, by exposing the raw response object, the user can then proceed as needed.

I still need to think a but about exactly the right interface, but I everything you said has merit and I will try to implement an elegant solution.