hfaran / Tornado-JSON

A simple JSON API framework based on Tornado
http://tornado-json.readthedocs.org/
MIT License
273 stars 60 forks source link

Can't use @schema.validate and return error from `get()` method #63

Closed rutsky closed 9 years ago

rutsky commented 9 years ago

Consider following example:

class ObjectsHandler(APIHandler):
    __url_names__ = ["objects"]

    @schema.validate(output_schema={
        "type": "object",
    })
    def get(self, id):
        object = some_find_object(id)

        if object is None:
            self.error("Object not found")
        else:
            return object

I want to return object type if object is found and output error Jsend status if it's not. According to validate() implementation it's currently impossible.

rutsky commented 9 years ago

I made workaround for this issue: if request is finished, don't do output validation. What do you think @hfaran ?

hfaran commented 9 years ago

Hey @rutsky, thanks for the patch! I'm a little busy at the moment, but I will take a closer look in a couple days! If I don't, please definitely feel free to bump this to remind me. Thanks! :)

hfaran commented 9 years ago

Hello again @rutsky. Had a few minutes to look closer at your issue.

In fact, you should not be using self.error directly here. You should let Tornado use it appropriately; what you should use yourself is, raise an APIError instead (or alternatively, just HTTPError from Tornado if you wish). This is the proper way that errors should be thrown using Tornado.

See one of my small projects that uses Tornado-JSON that has a similar pattern to what you describe.

rutsky commented 9 years ago

I'm new to Tornado-JSON and missed APIError exception. I agree that this is better ways of handling errors than manually writing error with self.error, so this issue is resolved. Thank you, @hfaran!

By the way, it would be nice to see examples of using APIError and api_assert in "helloworld" or "cars" demos.

hfaran commented 9 years ago

That is a fair point; they would be useful to have in the helloworld demo. I will walk you through how error propagation works in Tornado and then perhaps move this into the documentation.

When you raise an Exception, which in this let's say is APIError, Tornado will catch it and then call write_error method of the RequestHandler instance. Here is the one for APIHandler. As you can see, it takes care of calling either JSendMixin.fail or JSendMixin.error depending on what is appropriate.

It is a similar idea with JSendMixin.success. Ideally, you don't need to call this directly either, and can simply use the return <data> pattern provided by the schema.validate decorate which handles calling success for you.

My point with all of the above is, ideally you never need to call any methods from JSendMixin directly; they are all abstracted away by other patterns provided by Tornado-JSON. :)

rutsky commented 9 years ago

My point with all of the above is, ideally you never need to call any methods from JSendMixin directly; they are all abstracted away by other patterns provided by Tornado-JSON. :)

Yes, I agree, unfortunately the only example of handling error in demo application that I found was using JSendMixin directly in https://github.com/hfaran/Tornado-JSON/blob/master/demos/rest_api/cars/api/__init__.py#L76 :)

Thanks for the help, I'm using raise APIError now.