hfaran / Tornado-JSON

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

if output is empty, auto return the error 404. #73

Closed hyacinthus closed 9 years ago

hyacinthus commented 9 years ago

Hey, How about automatically return 404 when output data is empty?

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.33%) to 97.11% when pulling 537e234ac2b46bd276725dded6b29f236abcc05d on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

hfaran commented 9 years ago

Hmm, I'm not quite certain if this is something we want to bake directly into schema.validate with no option to disable, since it's entirely possible someone may want an empty response from a request as defined behaviour (although this is unlikely).

That being said, I think it would be best to add a boolean keyword argument here like on_empty_404=False for example. Then if you wanted to use it by default, you could just create a partial in your application like so, and use it:

import functools
validate = functools.partial(schema.validate, on_empty_404=True)
hfaran commented 9 years ago

Could you also add a test for this? Thanks!

alexzrh commented 9 years ago

There are definitely times when you may want an empty response. For example, GET a list of user-uploaded images. If you have a brand-new user and the request is GET images, the result should be empty to indicate they haven't uploaded any images yet. Returning a 404 would be atypical behavior in this very standard use case. Returning an empty set is expected.

hyacinthus commented 9 years ago

@hfaran @malexh Agree with you. Boolean keyword is better. I'll test it.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.33%) to 97.11% when pulling 05db567c36091590413c3fee59024bd24a22e175 on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

hyacinthus commented 9 years ago

In fact I can't find a right place to add this feature. But It is stupid to check the output in every handler, or a decorator one more? And now, I need import functools.partial, 2 lines per file, neither better nor worse...

hfaran commented 9 years ago

Well, you can just define a custom version of the decorator in your project like I do above, and then just import that.

For example, if you package is called mywebapp, just create a schema module and add this to it:

import functools

from tornado_json import schema

validate = functools.partial(schema.validate, on_empty_404=True)

Then you can use mywebapp.schema.validate instead of tornado_json.schema.validate.

hyacinthus commented 9 years ago

Thanks , Good Idea .

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.33%) to 97.11% when pulling 61a5c7d6e32f552a476ee33693641869d7965c39 on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

hfaran commented 9 years ago

@hyacinthus Are you planning on adding a test for this?

hyacinthus commented 9 years ago

Oh, sorry, I've misconstrued your words about test... I'll add it later.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.47% when pulling d7555245b86a67e8007d136303bb636223640654 on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

hyacinthus commented 9 years ago

OK

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.47% when pulling 87c39561dd0b286d5a7838e01f693f82e1d9b5ac on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.47% when pulling 87c39561dd0b286d5a7838e01f693f82e1d9b5ac on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.47% when pulling 87c39561dd0b286d5a7838e01f693f82e1d9b5ac on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+0.03%) to 97.47% when pulling 87c39561dd0b286d5a7838e01f693f82e1d9b5ac on Tarsbot:master into 973640b7aa333a3008d83ead198b11feae111942 on hfaran:master.

hfaran commented 9 years ago

Looks good! Merging.

hyacinthus commented 9 years ago

Thank you for your advice and patience.