VOLTTRON / volttron

VOLTTRON Distributed Control System Platform
https://volttron.readthedocs.io/
Other
458 stars 216 forks source link

Return Custom Status code in WebAgent response #1367

Closed sheridaj closed 7 years ago

sheridaj commented 7 years ago

Currently, the web agent will only return a 200 OK response for a successful call to an endpoint. It would be nice to return other status codes defined by the callback function.

Currently, the callback function can return a tuple of length 2. The first element in the tuple is the data for the response, the second element is a list of tuples, each tuple being a header-name, value. e.g. return "My Data", [("Content-Type", "application/sep+xml")]

Proposed solution: Make the first element a dictionary: e.g. return {"data": "My Data", "status_code": "200 OK"}, [("Content-Type", "application/sep+xml")]

However, there are other ways to implement this change. If the community has any feedback or preference about implementation, it is more than welcome

craig8 commented 7 years ago

Adding some context we are talking about the method create_response

https://github.com/VOLTTRON/volttron/blob/develop/volttron/platform/web.py#L627

def create_response(self, res, start_response): 

In the current implementation res can be a dictionary (for json-rpc), a or a list/tuple with the first element the data and the second element a "headers" element to specify return type, or a string which will be turned into json.

So the solution above would turn the first element of the tuple into a dictionary with data and status_code as the keys.

The assumption being that if that first element isn't a dictionary it would just be data with a default 200 OK code?

Correct?

acedrew commented 7 years ago

I don't think it's unreasonable to expect every endpoint to know its appropriate status code. Why not make it just a flat tuple of (status, data, headers)? I think I'm the only one currently affected by changes to this, and I'm happy to adapt to something that will be more consistent long term.

craig8 commented 7 years ago

I agree, however I would suggest that having to set the status code on all endpoints seems repetitive, especially since most will be a 200 OK (I would assume). That being said I do like the flat tuple that moves status to the end...so

(data, headers, status) if status is not set then have it be a default '200 OK'

I'll add tests to make sure this backward compatible and we can go from there.

acedrew commented 7 years ago

Shouldn't you be able to send a bad status with no content?

bbarcklay commented 7 years ago

Yes and SEP2 specifies HTTP 201 and 204 with no content. Perhaps this is a good time to address the implied todo in the code?

Note this is specific to volttron central agent and should probably not be at this level of abstraction.

If res was a response dictionary with keys: 'data', 'status_code' and 'headers', you could do convenient defaults, like 200, and follow a common convention for returning an HTTP response. Would it be a large effort to modify Volttron Central to use this interface?

acedrew commented 7 years ago

@craig8, hmmmmm @ffreckle 's point sounds a lot like our original discussion about non-json/html dynamic content...

acedrew commented 7 years ago

In thinking about this more, I think that what should happen is an abstraction between a raw response and a json endpoint, we should just register them as separate endpoints. This will simplify keeping defaults for json RPC and allowing other applications to have control over the http(s) connection. Anything else will continue diverging use cases and exposing more and more edge cases.

craig8 commented 7 years ago

Abstraction is a good thing. We could use Flask ( a library I really like) and look at their response class and follow some things from it (http://flask.pocoo.org/docs/0.12/api/#flask.Response). That class references (http://flask.pocoo.org/docs/0.12/api/#flask.Flask.make_response) which is what the original intent was.

So basically I would like to see a Response class that is generated from the the tuple make response and then that object can be used from the web.py to build the response.

Oh and I think that it won't be too difficult to refactor existing agents and now is a good time as we look to 4.5 and 5 at the end of the year.

This refactor would help with #1378 as well

acedrew commented 7 years ago

Checkout the proposed WebResponse Class I referenced in #1382, I would love comments there.