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

Allow dictionaries return values as JSON #3111

Merged
merged 2 commits into from May 24, 2019

Conversation

@pgjones
Copy link
Member

commented Mar 3, 2019

This is something I've been experimenting with in Quart and I don't see a downside. I can't find any old issues relating to this (although I've found it hard to search for). This is roughly an issue, but as a pull request (branch) you can take the code and try it out. Obliviously if you consider this a good idea I'll add much more testing and documentation.

This supports an increasingly common use-case whereby JSON is the primary response (rather than a templated string). Given Flask simplifies returning HTML responses, it seems fitting that it should also do so for JSON responses. In practice it allows,

@app.route("/")
def index():
    return {
        "api_stuff": "values",
    }

which is equivalent to

@app.route("/")
def index():
    return jsonify({
        "api_stuff": "values",
    })

Note

This doesn't support returning anything other than an associate array at the top level in the JSON response. I'm ok with this as in practice APIs are only extensible if the top level is an associate array.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Good idea, but I think this should be done on the application level.

Also, if I can return a dict to be jsonified, I'd expect to be able to do the same with let's say a number, boolean or a list - especially a list is quite common for JSON responses.

@davidism

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

This seems like a good idea. SO questions about "dict object is not callable" are common. This is already pretty common example of how to extend Flask.make_response.

I agree with only handling dict for now. List is problematic because returning a 2- or 3-tuple/list has meaning already, and iterables generally have a different meaning in WSGI. There's no way to return a string and get a JSON string. If we want to extend this to number, boolean, and null, we can address that later.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

Wouldn't it be more consistent to fail with a clear error message when returning a dict? And then document how to jsonify all returned values for the app / a blueprint?

@davidism

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

I had been thinking about making the error message nicer, since "X is not callable" is just confusing. I think we can both support dict and have a better error message for unrecognized types.

@davidism

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

ThiefMaster brings up that this might make it seem that we're endorsing "APIs should return dicts", which while a good general rule isn't always true. I'm not convinced one way or another yet. jsonify itself didn't support other types until recently, and it's still available to use directly. This change won't solve the second most common source of the error, which is users expecting to be able to return a cursor.fetchall() db result list.

Is there ever a case where we'd want to make a dict signify something about the response like a tuple does now? Perhaps as ASGI support is added? If we go through with this, we're basically locking this option out, although I'm not entirely happy with returning a tuple as a shortcut for a Response in the first place.

The lowest impact way to change this is to make a nice error message that mentions jsonify and points to the docs.

@pgjones

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

I think "APIs should return dicts" is a fine thing to endorse, and this is simply some sugar to make things easier (it doesn't actually make returning something else as JSON any more difficult).

I'm not aware of any extra meaning to a dict response and in my experience of ASGI with Quart I've not come across any desire to do this (other than for JSON).

I'm happy to add the error message with this, but the aim in my view is the simplified way to return JSON. I think the tuple shortcut is a strength of Flask, as it makes a HTML response very easy to construct - my hope is that this does so for JSON responses.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Why would you want to return a dict for an API that returns a collection, and does not have metadata associated with the collection itself (and thus requires an object on the top level).

Something like this in the GitHub API for example. It's a collection and thus a list - and pagination is handled solely out-of-band through headers.

In the past having an array on the top level was bad because of browser security problems that allowed circumventing the same-origin policy, but those have been fixed years ago.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Don't get me wrong, I'm all for making it easy to write proper APIs - but I'd rather have to opt-in to this behavior (e.g. for an API blueprint of the whole app), and in that case jsonify any return value that can be jsonified (ie pretty much anything that's not a response or a data, statuscode tuple)

@davidism

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Opting in isn't much different than what users already have to do to support this (they'd still need need to be aware that it's even possible, and import a separate Flask class, just wouldn't have to add a few lines to make_response).

This sort of seems to be getting into "what is good API design" territory. At some point we need to assume users with opinions about API design, such as pagination in headers or returning lists (which are both fine), will do that regardless of any default Flask provides. Or they'll choose an extension such as Flask-Restful and not be affected by it anyway. For beginners, this is one less thing to trip up on.

@pgjones pgjones force-pushed the pgjones:master branch from 11bb165 to 0c5ea7a Mar 10, 2019

@pgjones pgjones changed the title WIP: Allow dictionaries return values as JSON Allow dictionaries return values as JSON Mar 10, 2019

@pgjones

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2019

Given the discussion I've gone ahead and updated the documentation and tests (am I missing something? I expected I'd need to do more than this). I think this should be merged and would encourage your detailed review.
(I think the error message has already been fixed).

@pgjones pgjones requested a review from davidism Mar 18, 2019

@lepture

This comment has been minimized.

Copy link
Member

commented May 22, 2019

This seems not a general use case, there are many cases for API response, for example:

  1. list: []
  2. with status code: {}, 201

I think what you want can be done with a custom Flask:

class MyFlask(Flask):
    def make_response(self, rv):
        if isinstance(rv, dict):
            return jsonify(rv)
        return super(MyFlask, self).make_response(rv)
@pgjones

This comment has been minimized.

Copy link
Member Author

commented May 22, 2019

Thanks for you interest @lepture, I had hoped the discussion above answered your points - but now I think it isn't clear (any chance you could point out where?). I've also written this article which I hope clearly makes the case for this change.

Edit: This patch allows {}, 201 and a variant {}, {"X-Header": "value"}, 201

@davidism

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I'm going to say yes to this. The coverage of a common use case and tripping point for new users outweighs the restrictions it imposes on the format, and doesn't prevent using the already well-documented patterns and libraries for more complex API design.

@davidism davidism added the json label May 24, 2019

Allow dictionary return values as JSON
This supports an increasingly common usecase whereby JSON is the
primary response (rather than a templated string). Given Flask has a
short syntax for HTML reponses, it seems fitting that it should also
do so for JSON responses. In practice it allows,

     @app.route("/")
     def index():
         return {
             "api_stuff": "values",
         }

@davidism davidism added this to the 1.1.0 milestone May 24, 2019

@davidism davidism force-pushed the pgjones:master branch from 0c5ea7a to 7bf8366 May 24, 2019

@davidism

This comment has been minimized.

Copy link
Member

commented May 24, 2019

@pgjones I force pushed to get this in line with the latest changes and added a changelog. You're going to have to reset your local master branch after this is merged.

git fetch origin
git checkout master
git reset --hard origin/master
@davidism

This comment has been minimized.

Copy link
Member

commented May 24, 2019

I added a section to the quickstart titled "APIs with JSON". We should probably break out the JSON support docs into their own page at some point. Some of it is in the API docs, but it doesn't make a ton of sense there.

@davidism davidism merged commit 855d59b into pallets:master May 24, 2019

12 checks passed

Flask Build #20190524.4 succeeded
Details
Flask (Flask DocsHtml) Flask DocsHtml succeeded
Details
Flask (Flask Pypy3Linux) Flask Pypy3Linux succeeded
Details
Flask (Flask Python27Linux) Flask Python27Linux succeeded
Details
Flask (Flask Python27Windows) Flask Python27Windows succeeded
Details
Flask (Flask Python35Linux) Flask Python35Linux succeeded
Details
Flask (Flask Python36Linux) Flask Python36Linux succeeded
Details
Flask (Flask Python37Linux) Flask Python37Linux succeeded
Details
Flask (Flask Python37Mac) Flask Python37Mac succeeded
Details
Flask (Flask Python37Windows) Flask Python37Windows succeeded
Details
Flask (Flask VersionRange) Flask VersionRange succeeded
Details
Flask (FlaskOnNightly) FlaskOnNightly succeeded
Details
@pgjones

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

🎉 😄 thanks @davidism

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.