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

Error payload #186

Merged
merged 27 commits into from
Jul 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0889ea1
Initial implementation
stefanoborini Jul 27, 2016
5c4fe7f
Merge branch 'master' into 175-error-payload
stefanoborini Jul 27, 2016
9764797
Added more control, and generalized exception
stefanoborini Jul 27, 2016
eeda70b
Wrong HTTPError
stefanoborini Jul 27, 2016
25caf30
Fixed documentation
stefanoborini Jul 27, 2016
dabad45
Fixed flake
stefanoborini Jul 27, 2016
fb137ac
Added missing yield
stefanoborini Jul 27, 2016
8b5909b
Adapted against current json handler in javascript
stefanoborini Jul 27, 2016
873af13
Polished the concept of representable exceptions
stefanoborini Jul 27, 2016
35f42e4
Workaround for 201 Created error and Content-Type correctness
stefanoborini Jul 27, 2016
424e572
Better to have the message explicit.
stefanoborini Jul 28, 2016
138f8fd
Set content type on the payloaded exception
stefanoborini Jul 28, 2016
cc26710
Test for payloaded http error
stefanoborini Jul 28, 2016
61b10f4
Removed content type when finish on empty exception
stefanoborini Jul 28, 2016
ba04eaf
Added more tests
stefanoborini Jul 28, 2016
b70230b
Added the items to the unprocessable
stefanoborini Jul 28, 2016
0ac6eae
Added tests for the container mapping id handling
stefanoborini Jul 28, 2016
6e5ed8e
Readapted tests for dummy mocks
stefanoborini Jul 28, 2016
7bb495d
Revert "Readapted tests for dummy mocks"
stefanoborini Jul 28, 2016
901f0d2
Merge branch 'master' into 175-error-payload
stefanoborini Jul 28, 2016
84df4e9
Readapted tests for dummy mocks
stefanoborini Jul 28, 2016
246113d
Adapted tests
stefanoborini Jul 28, 2016
e919598
Neutralized apidoc change
stefanoborini Jul 28, 2016
da7287f
Restored dummy
stefanoborini Jul 28, 2016
446fe7e
Changed test name
stefanoborini Jul 28, 2016
64ea0b1
Regreened tests
stefanoborini Jul 28, 2016
869230a
Added tests for except conditions
stefanoborini Jul 28, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ help:
@echo " coverage to run coverage check of the documentation (if enabled)"
@echo " dummy to check syntax errors of document sources"
@echo " uml to convert all uml files into images"
@echo " apidoc to rebuild the apidoc documents"

.PHONY: clean
clean:
Expand Down
30 changes: 30 additions & 0 deletions doc/source/api/remoteappmanager.rest.http.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
remoteappmanager.rest.http package
==================================

Submodules
----------

remoteappmanager.rest.http.httpstatus module
--------------------------------------------

.. automodule:: remoteappmanager.rest.http.httpstatus
:members:
:undoc-members:
:show-inheritance:

remoteappmanager.rest.http.payloaded_http_error module
------------------------------------------------------

.. automodule:: remoteappmanager.rest.http.payloaded_http_error
:members:
:undoc-members:
:show-inheritance:


Module contents
---------------

.. automodule:: remoteappmanager.rest.http
:members:
:undoc-members:
:show-inheritance:
15 changes: 7 additions & 8 deletions doc/source/api/remoteappmanager.rest.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
remoteappmanager.rest package
=============================

Subpackages
-----------

.. toctree::

remoteappmanager.rest.http

Submodules
----------

Expand All @@ -12,14 +19,6 @@ remoteappmanager.rest.exceptions module
:undoc-members:
:show-inheritance:

remoteappmanager.rest.httpstatus module
---------------------------------------

.. automodule:: remoteappmanager.rest.httpstatus
:members:
:undoc-members:
:show-inheritance:

remoteappmanager.rest.registry module
-------------------------------------

Expand Down
8 changes: 8 additions & 0 deletions doc/source/api/remoteappmanager.rst
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ remoteappmanager.jinja2_adapters module
:undoc-members:
:show-inheritance:

remoteappmanager.netutils module
--------------------------------

.. automodule:: remoteappmanager.netutils
:members:
:undoc-members:
:show-inheritance:

remoteappmanager.paths module
-----------------------------

Expand Down
34 changes: 31 additions & 3 deletions remoteappmanager/rest/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from remoteappmanager.rest import httpstatus
from remoteappmanager.rest.http import httpstatus


class RESTException(Exception):
Expand All @@ -9,6 +9,26 @@ class RESTException(Exception):
#: Missing any better info, default is a server error.
http_code = httpstatus.INTERNAL_SERVER_ERROR

def __init__(self, message=None, **kwargs):
"""Initializes the exception. keyword arguments will become
part of the representation as key/value pairs."""
self.message = message
self.info = kwargs if len(kwargs) else None

def representation(self):
"""Returns a dictionary with the representation of the exception.
"""
data = {
"type": type(self).__name__
}
if self.message is not None:
data["message"] = self.message

if self.info is not None:
data.update(self.info)

return data


class NotFound(RESTException):
"""Exception raised when the resource is not found.
Expand All @@ -17,6 +37,11 @@ class NotFound(RESTException):
"""
http_code = httpstatus.NOT_FOUND

def representation(self):
"""NotFound is special as it does not have a representation,
just an error status"""
return None


class BadRequest(RESTException):
"""Exception raised when the resource representation is
Expand All @@ -27,5 +52,8 @@ class BadRequest(RESTException):
http_code = httpstatus.BAD_REQUEST


class InternalServerError(RESTException):
pass
class Unable(RESTException):
"""Exception raised when the CRUD request cannot be performed
for whatever reason that is not dependent on the client.
"""
http_code = httpstatus.INTERNAL_SERVER_ERROR
Empty file.
36 changes: 36 additions & 0 deletions remoteappmanager/rest/http/payloaded_http_error.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from tornado.web import HTTPError


class PayloadedHTTPError(HTTPError):
def __init__(self, status_code,
payload=None,
content_type=None,
log_message=None,
*args, **kwargs):
"""Provides a HTTPError that contains a string payload to output
as a response. If the payload is None, behaves like a regular
HTTPError, producing no payload in the response.

Parameters
----------
payload: str or None
The payload as a string
content_type: str or None
The content type of the payload
log_message: str or None
The log message. Passed to the HTTPError.
"""
super().__init__(status_code, log_message, *args, **kwargs)

if payload is not None:
if not isinstance(payload, str):
raise ValueError("payload must be a string.")

if content_type is None:
content_type = "text/plain"
else:
if content_type is not None:
raise ValueError("Content type specified, but no payload")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would anyone say "application/json" and then provide no payload?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not what you would expect, but does it actually cause problem? If not, seem to be an unnecessary potential failure here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not unnecessary. The class constraints are clear. if you specify the payload, you can or cannot specify the content type. If you don't, it's by default text/plain. If you don't specify a payload, there's no reason for specifying the content type, and if you do, you are doing something very wrong, because it means that your payload is None by accident.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it wrong to set the header content-type to something but provide an empty content?
(by the way you could default payload to an empty string, just an idea)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a difference between empty payload an no payload. some http responses don't want payload, and it's invalid if they do provide one (e.g. 204 No content, or 404 Not Found)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in the context of REST, I mean. 404 can have a payload in non-rest context

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No payload but has content-type header - is it just because it is wrong in terms of definition or does it cause error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it causes an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you explain how please? I just wanted to understand.


self.content_type = content_type
self.payload = payload
60 changes: 52 additions & 8 deletions remoteappmanager/rest/rest_handler.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
from remoteappmanager.handlers.base_handler import BaseHandler
from tornado import gen, web, escape

from remoteappmanager.rest import exceptions
from remoteappmanager.rest.http import httpstatus
from remoteappmanager.rest.http.payloaded_http_error import PayloadedHTTPError
from remoteappmanager.rest.registry import registry
from remoteappmanager.utils import url_path_join, with_end_slash
from remoteappmanager.rest import httpstatus, exceptions
from tornado import gen, web, escape


class RESTBaseHandler(BaseHandler):
Expand All @@ -25,6 +26,44 @@ def get_resource_handler_or_404(self, collection_name):
except KeyError:
raise web.HTTPError(httpstatus.NOT_FOUND)

def write_error(self, status_code, **kwargs):
"""Provides appropriate payload to the response in case of error.
"""
exc_info = kwargs.get("exc_info")

if exc_info is None:
self.clear_header('Content-Type')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Content-Type' is by default not set unless you overload set_default_header. Before calling write_error, send_error reset the header using set_default_header. Why do we need to clear 'content-type' here? (no effect if set_default_header is passed, override set_default_header if it was defined for 'content-type')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write error is responsible for producing the right output. The content type depends on the payload. We know the payload here, so we zero the content-type here.

self.finish()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finish with the status_code (the argument) as content. exc_info may not be available if web.RequestHandler.send_error is manually called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the status code is set somewhere else. write error is just in charge of rendering the payload.


exc = exc_info[1]

if isinstance(exc, PayloadedHTTPError) and exc.payload is not None:
Copy link
Contributor

@kitchoi kitchoi Jul 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, convert RestException to PayloadedHTTPError here, this way the Resource only had to raise a RestException and the payload is handled here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of the design decision. My vision is that this method only handles http-level exception types, with the conversion of exception type done at the individual http method handlers. This allows us to take appropriate action and, for example log contextually to the method handler. Converting here we lose this specificity.

I don't know which one is best. For sure, once we were to go for a write_error method that handles rest exceptions, then it should handle all exceptions. Plus, write_error is just responsible for the payload. We would have to reimplement send_error too, so that proper status can be set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vision is that this method only handles http-level exception types, with the conversion of exception type done at the individual http method handlers.

We can't prevent non-http-level exception from arriving at write_error. write_error is intended to be able to handle BaseException in general.

This allows us to take appropriate action and, for example log contextually to the method handler. Converting here we lose this specificity.

I don't think converting a RestException to PayloadedHTTPError here stop you from logging contextually. If you must log, you can do so in Rest*Handler

try:
   yield method()
except Exception:
   log.exception
   raise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write an issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean opening an issue for logging? I only meant to say that 'converting a RestException to PayloadedHTTPError does not stop you from logging contextually', I don't mean that there is an issue with logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to handle exceptions at the write_error level I need to do some adjustments to the current code. My main concern is if it's a good idea to handle a broad range of exceptions types there.

self.set_header('Content-Type', exc.content_type)
self.finish(exc.payload)
else:
# For non-payloaded http errors or any other exception
# we don't want to return anything as payload.
# The error code is enough.
self.clear_header('Content-Type')
self.finish()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am correct, we need to call finish with the status code here, otherwise all the raise web.HTTPError would not have the status code included in the response.
See http://www.tornadoweb.org/en/stable/_modules/tornado/web.html#RequestHandler.write_error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finish does not take the status code.

Copy link
Contributor

@kitchoi kitchoi Jul 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I meant to have the status code in the response body as text, not the response.status_code attribute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not necessary but convenient to see the status code displayed on the page.


def rest_to_http_exception(self, rest_exc):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Class method for PayloadedHTTPError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. In the future, the conversion will be done by a serializer that may produce xml instead of json. This serializer will be a instance var.

"""Converts a REST exception into the appropriate HTTP one."""

representation = rest_exc.representation()
payload = None
content_type = None

if representation is not None:
payload = escape.json_encode(representation)
content_type = "application/json"

return PayloadedHTTPError(
status_code=rest_exc.http_code,
payload=payload,
content_type=content_type
)


class RESTCollectionHandler(RESTBaseHandler):
"""Handler for URLs addressing a collection.
Expand All @@ -38,7 +77,7 @@ def get(self, collection_name):
try:
items = yield res_handler.items()
except exceptions.RESTException as e:
raise web.HTTPError(e.http_code)
raise self.rest_to_http_exception(e)
except NotImplementedError:
raise web.HTTPError(httpstatus.METHOD_NOT_ALLOWED)
except Exception:
Expand Down Expand Up @@ -67,7 +106,7 @@ def post(self, collection_name):
try:
resource_id = yield res_handler.create(data)
except exceptions.RESTException as e:
raise web.HTTPError(e.http_code)
raise self.rest_to_http_exception(e)
except NotImplementedError:
raise web.HTTPError(httpstatus.METHOD_NOT_ALLOWED)
except Exception:
Expand All @@ -87,6 +126,7 @@ def post(self, collection_name):

self.set_status(httpstatus.CREATED)
self.set_header("Location", location)
self.clear_header('Content-Type')
self.flush()


Expand All @@ -104,7 +144,7 @@ def get(self, collection_name, identifier):
try:
representation = yield res_handler.retrieve(identifier)
except exceptions.RESTException as e:
raise web.HTTPError(e.http_code)
raise self.rest_to_http_exception(e)
except NotImplementedError:
raise web.HTTPError(httpstatus.METHOD_NOT_ALLOWED)
except Exception:
Expand All @@ -128,6 +168,8 @@ def post(self, collection_name, identifier):

try:
exists = yield res_handler.exists(identifier)
except exceptions.RESTException as e:
raise self.rest_to_http_exception(e)
except NotImplementedError:
raise web.HTTPError(httpstatus.METHOD_NOT_ALLOWED)
except Exception:
Expand Down Expand Up @@ -156,7 +198,7 @@ def put(self, collection_name, identifier):
try:
yield res_handler.update(identifier, representation)
except exceptions.RESTException as e:
raise web.HTTPError(e.http_code)
raise self.rest_to_http_exception(e)
except NotImplementedError:
raise web.HTTPError(httpstatus.METHOD_NOT_ALLOWED)
except Exception:
Expand All @@ -166,6 +208,7 @@ def put(self, collection_name, identifier):
identifier))
raise web.HTTPError(httpstatus.INTERNAL_SERVER_ERROR)

self.clear_header('Content-Type')
self.set_status(httpstatus.NO_CONTENT)

@web.authenticated
Expand All @@ -176,7 +219,7 @@ def delete(self, collection_name, identifier):
try:
yield res_handler.delete(identifier)
except exceptions.RESTException as e:
raise web.HTTPError(e.http_code)
raise self.rest_to_http_exception(e)
except NotImplementedError:
raise web.HTTPError(httpstatus.METHOD_NOT_ALLOWED)
except Exception:
Expand All @@ -186,4 +229,5 @@ def delete(self, collection_name, identifier):
identifier))
raise web.HTTPError(httpstatus.INTERNAL_SERVER_ERROR)

self.clear_header('Content-Type')
self.set_status(httpstatus.NO_CONTENT)
Loading