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

JSON support for test client and response object #1416

Closed
wants to merge 13 commits into from

Conversation

@adambyrtek
Copy link
Contributor

adambyrtek commented Apr 1, 2015

Common code has been refactored into a mixin shared between request and response classes. This is a continuation of #1408.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 2, 2015

Not ready for review yet.

getter = getattr(req, 'get_data', None)
if getter is not None:
return getter(cache=cache)

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 2, 2015

Author Contributor

The flag was unnecessary here for two reasons:

  1. Werkzeug's request.get_data() has cache=True as a default.
  2. JSONMixin.get_json() with caching enabled shouldn't call _get_data() more than once anyway.

Removed since the response object doesn't support explicit caching.

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 2, 2015

Member

get_json(cache=False) shouldn't store any data on the response object and consume any underlying buffers IIRC

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 2, 2015

Author Contributor

Makes sense... please take a look at the alternative approach I've just pushed.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 2, 2015

@untitaker I suggest to close #1408 and move the conversation to this pull request, since it incorporates both changes including documentation.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 4, 2015

This is quite a bunch to review. I'll take a look at it sometime, but I don't have the time ATM and I'd apprechiate if somebody else looked over it too.

email = 'john@example.com'
password = 'secret'
resp = c.post('/api/auth', json={'email': email, 'password': password})
assert verify_token(email, resp.json['token'])

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 4, 2015

Member

I don't think example code in the docs should be using a deprecated property.

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 6, 2015

Author Contributor

Done

this method is used by :meth:`get_json` when an error occurred. The
default implementation just raises a :class:`BadRequest` exception.
"""
raise BadRequest()

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 4, 2015

Member

What happened to the more verbose error in debug mode?

         ctx = _request_ctx_stack.top
         if ctx is not None and ctx.app.config.get('DEBUG', False):
             raise BadRequest('Failed to decode JSON object: {0}'.format(e))
         raise BadRequest()

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 6, 2015

Author Contributor

This is a default mixin implementation that works for both request/response. The more verbose error relies on request context, so it was moved to Request.on_json_loading_falied which overloads this one.

The diff can be a bit misleading so feel free to look at the full source.

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 6, 2015

Member

One could use a single method that first checks whether a context is available before trying to access current_app.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 4, 2015

Thanks @ThiefMaster

if cache:
self._cached_json = rv
return rv

def on_json_loading_failed(self, e):
"""Called if decoding of the JSON data failed. The return value of
this method is used by :meth:`get_json` when an error occurred. The

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 4, 2015

Member

Can this method be moved into the mixin?

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 6, 2015

Member

I've also noticed that it checks whether the request context is available, but then only accesses the current app. It should only check for the app context.

(not your fault, probably a relict of splitting the request context up into request and app context)

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 6, 2015

Author Contributor

You're really quick, actually I just had the same idea, see the last commit :)

'Use get_json() instead.'), stacklevel=2)
return self.get_json()

def _get_data_for_json(req, cache):

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 4, 2015

Member

First argument should be named self

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 6, 2015

Author Contributor

Missed this during refactoring. Fixed.

@@ -201,3 +213,10 @@ class Response(ResponseBase):
set :attr:`~flask.Flask.response_class` to your subclass.
"""
default_mimetype = 'text/html'

def _get_data_for_json(req, cache):

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster Apr 4, 2015

Member

First argument should be named self

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 6, 2015

Author Contributor

Done

CHANGES Outdated
@@ -8,6 +8,9 @@ Version 1.0

(release date to be announced, codename to be selected)

- Added `json` keyword argument to :meth:`flask.testing.FlaskClient.open`
(and related ``get``, ``post``, etc.), which makes it more convenient to
send JSON requests from the test client.

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 4, 2015

Member

Something's missing here, see PR title.

This comment has been minimized.

Copy link
@adambyrtek

adambyrtek Apr 6, 2015

Author Contributor

Added a second entry.

@adambyrtek adambyrtek changed the title JSON support for the response object JSON support for test client and response object Apr 4, 2015
@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 6, 2015

@untitaker @ThiefMaster Thanks a lot for the review, I've gone through all your comments, so please take one more look.

One thing that's not clear to me is what is the reason for deprecating the json property (the changelog doesn't help much)? For me it's much more elegant to write request.json['foo'] instead of request.get_json()['foo'], and properties are well-established in the Python community.

I understand that there is some computation and caching hidden under the hood, but most people working with JSON APIs won't have to worry about that, and the rest can use the getters directly if they want to.

@ThiefMaster

This comment has been minimized.

Copy link
Member

ThiefMaster commented Apr 6, 2015

I think you are supposed to use something like data = request.get_json() and then use data in your code. But I'm not a big fan of the deprecation either. It feels nicer to use request.json just like request.form etc.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 6, 2015

I think the general idea was to make resource management more explicit.

IIRC there was a Werkzeug or Flask issue where somebody expected to be able to read from the data attribute after already having accessed json, but doing so would have implicitly copied the request payload all over the place. Now you have to do it explicitly.

@ThiefMaster

This comment has been minimized.

Copy link
Member

ThiefMaster commented Apr 6, 2015

But it's the same for e.g. request.form - so I'd prefer not to have write uglier code for the same of someone who can't read the documentation ;)

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 6, 2015

I'm not sure where I stand either, but I think form being still there isn't a deliberate inconsistency.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 6, 2015

I use request.json all over the place in my API, and this is a backwards-incompatible change that would make the life of my team harder, so I'm not sure whether the benefit justifies the pain. I believe it should be the person who has an unusual use case having to go the extra mile (maybe with a more meaningful error message).

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 6, 2015

Regarding API breakage in general: To be fair, you have a lot of time to upgrade as Flask releases are really rare.

@ThiefMaster

This comment has been minimized.

Copy link
Member

ThiefMaster commented Apr 6, 2015

I think changing request.form to a method would be the kind of API breakage that pisses off people, even if it's in a major version bump ;)

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 6, 2015

Hmm, I think we should move this discussion somewhere else, perhaps a new issue.

I'll keep this PR unmerged and will properly review it tomorrow uhh sometime. I'd like more eyes on it too.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 6, 2015

Sure, let's track this separately, and I'm happy to stay involved. You know where I stand, even when this becomes deprecated I will still use request.json on my project because I believe it leads to cleaner code, and I cannot justify the refactoring.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 6, 2015

Just to clarify, this should be ready for the final review regardless of the result of that discussion.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 12, 2015

@untitaker Gentle ping.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 12, 2015

@adambyrtek Could you check whether the new attrs for response show up correctly in the docs?

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 12, 2015

@untitaker You mean the API docs? Now is does, it never showed up for Request by the way.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 15, 2015

@untitaker What do you think?

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Sep 28, 2015

Yeah, there's no policy. You may modify history however you want until it hits master.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Sep 28, 2015

@untitaker I prefer to keep the history for the record (unless it contains something obviously unnecessary or erroneous) so feel free to merge when you're ready to do so.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Sep 28, 2015

I pinged @mitsuhiko about this again in IRC.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Sep 28, 2015

Just to be sure you remember, you said "Will merge. Could you rebase?" some time ago (see #1416 (comment)). Anyway, I'm happy to wait for the blessing.

While we're at this, it would be great to get @mitsuhiko opinion on #1421

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Sep 28, 2015

Argh. Sorry. I don't know what to do.

I can't reach him.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Sep 29, 2015

@untitaker Maybe you should just make a judgement call? I don't think this PR introduces anything revolutionary. It simply adds JSON capabilities to the response object, mostly for testing purposes.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Sep 29, 2015

Yes, and @mitsuhiko has been known to oppose decisions that seemed straightforward to anybody else.

Apparently that means that no real progress can be made, but it's not my decision to judge whether that's a good outcome -- it's still Armin's project after all.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Sep 29, 2015

Fair enough, I didn't know that he is the sole maintainer.

@@ -20,7 +21,7 @@
from urlparse import urlsplit as url_parse


def make_test_environ_builder(app, path='/', base_url=None, *args, **kwargs):
def make_test_environ_builder(app, path='/', base_url=None, json=None, *args, **kwargs):

This comment has been minimized.

Copy link
@johnnyg

johnnyg Nov 26, 2015

I realise this comment is a bit late but wouldn't it be better to not make json a keyword argument and instead check for it inside kwargs? This would mean that passing json=None would send a valid json request with data as 'null' instead of not doing anything. It also has the added benefit that you don't need to rename your import of json.

This comment has been minimized.

Copy link
@gregorynicholas

gregorynicholas Apr 21, 2016

i agree, but the method signature becomes less explicit, and relies pretty heavily on implicit knowledge of the workings .

besides look up at url_parse

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 21, 2016

Member

I agree with @johnnyg.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Nov 27, 2015

@mitsuhiko I've seen your recent comment on another JSON-related issue. Would you mind taking a look at this one?

return self.get_json()

def _get_data_for_json(self, cache):
getter = getattr(self, 'get_data', None)

This comment has been minimized.

Copy link
@gregorynicholas

gregorynicholas Apr 21, 2016

just use hasattr() ..?

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 21, 2016

Member

@gregorynicholas Would you really want to try to call get_data if it's None?

@@ -201,3 +206,10 @@ class Response(ResponseBase):
set :attr:`~flask.Flask.response_class` to your subclass.
"""
default_mimetype = 'text/html'

def _get_data_for_json(self, cache):

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 21, 2016

Member

This method is already in JSONMixin.

@adambyrtek

This comment has been minimized.

Copy link
Contributor Author

adambyrtek commented Apr 22, 2016

Thanks for the comments. I'm happy to do the changes but only if this has a chance of moving forward.

@untitaker untitaker assigned untitaker and unassigned mitsuhiko Apr 22, 2016
@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Apr 22, 2016

That is totally understandable, and thank you for all your work. I personally
want this to land in 0.11.

On Fri, Apr 22, 2016 at 09:36:16AM -0700, Adam Byrtek wrote:

Thanks for the comments. I'm happy to do the changes but only if this has a chance of moving forward.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1416 (comment)

@mitsuhiko

This comment has been minimized.

Copy link
Member

mitsuhiko commented Apr 22, 2016

I think it generally looks reasonable. I don't have any arguments for not merging it.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented May 4, 2016

Excellent. @adambyrtek do you have time?

@untitaker untitaker added the waiting label May 4, 2016
@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented May 30, 2016

@adambyrtek So this didn't make it in time. Are you still interested in updating this?

@DoWhileGeek

This comment has been minimized.

Copy link

DoWhileGeek commented Jun 14, 2016

pallets/werkzeug#948 addresses the Response side of this issue. It needs a review if anyone wants to jump on it.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Aug 19, 2016

I'm picking this up at #1984

@untitaker untitaker closed this Aug 19, 2016
@untitaker untitaker removed the waiting label Aug 19, 2016
@davidism davidism removed the needs review label Apr 19, 2017
@adambyrtek adambyrtek deleted the adambyrtek:response-json branch Jan 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.