Add ability to return a JSON body from an exception #230

Merged
merged 2 commits into from Jan 24, 2016

Projects

None yet

6 participants

@sigmavirus24
Contributor

Closes #209

@bertjwregeer bertjwregeer commented on an outdated diff Jan 13, 2016
@@ -308,6 +322,9 @@ def generate_response(self, environ, start_response):
if accept and 'html' in accept or '*/*' in accept:
content_type = 'text/html'
body = self.html_body(environ)
+ elif accept and 'json' in accept:
@bertjwregeer
bertjwregeer Jan 13, 2016 Member

If you have an XHR request that also still has */* in it, this case will not get hit. Is this acceptable? Or do we want to see if JSON is listed first before checking for HTML/*/*?

@bertjwregeer
Member

If you could please make sure that coverage is back to 100% I will accept this PR.

@mmerickel
Member

I would really prefer a solution that parsed the accept and used best_match if we are going down this route of caring about accept.

accept = webob.accept.Accept.parse(environ.get['HTTP_ACCEPT'])
match = accept.best_match('text/html', 'application/json')
...
@bertjwregeer
Member

We currently already care about accept, in that it checks if HTML exists or not. Using best_match would indeed be a better solution.

@bertjwregeer
Member

This kind of goes hand in hand with: Pylons/pyramid#1378 in that we could use very similar to almost exactly the same code in both codebases since Pyramid's HTTPExceptions are a strict superset of the WebOb ones.

@sigmavirus24 sigmavirus24 Use Accept to parse headers and find the best match
Add more tests to bring coverage back up to 100%
539a9f7
@sigmavirus24
Contributor

@bertjwregeer I've addressed the feedback thus far. Travis is running, but just wanted to let you know this should be ready for review again (I've 100% coverage locally.)

@mmerickel
Member

Do you have a need for passing a custom json_formatter into __init__? That smells like feature creep to me.

@sigmavirus24
Contributor

@mmerickel it's mostly to give people a way to change the "template" for their JSON response but I guess they can just subclass and override that? I was trying to provide functionality similar to body_template and text_template. I don't know if that's desired though.

@sigmavirus24
Contributor

@mmerickel @bertjwregeer any further thoughts on json_formatter or this PR?

@bertjwregeer
Member

I personally don't mind adding the json_formatter. I think it might be a good idea that allows customization and would be helpful in my own projects so I don't have to sub-class anything.

@bertjwregeer
Member

Unless I hear a complaint I'll merge this in the next day or so.

@bertjwregeer bertjwregeer added this to the Version 1.6 milestone Jan 18, 2016
@mmerickel
Member

LGTM, I just wanted to force someone to defend it before it goes in. Conforming to the analogous text_template etc is enough for me!

@sigmavirus24
Contributor

Yeah, we just can't use templates for JSON unfortunately. There are keywords to json.dumps that people may want to pass in the future too, but by default this should be enough. It might be fair, however, to ask people to subclass to pass those specifically. I don't need them, but I can imagine someone might, thoughts?

@bertjwregeer
Member

@sigmavirus24 If someone wants to pass particular keywords, then they can sub-class, otherwise we can keep going and ultimately WebOb will have full JSON parsing/validation/everything else ;-)

@sigmavirus24
Contributor

👌

@sjmc7
sjmc7 commented Jan 22, 2016

As an additional vote for this, it'd be really helpful for the openstack projects that use webob and generally work with JSON requests and responses.

@ttripp
ttripp commented Jan 22, 2016

As an OpenStack developer, I can definitely agree that this would help keep us from having to re-implement this code.

@bertjwregeer bertjwregeer merged commit bbca6e8 into Pylons:master Jan 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bertjwregeer
Member

As an OpenStack user, I don't want you guys re-implementing this code =)

@sigmavirus24 sigmavirus24 deleted the sigmavirus24:bug/209 branch Jan 26, 2016
@sigmavirus24
Contributor

Thanks @bertjwregeer! Let me know if I can help out with 1.6. It's clear a bunch of people would benefit from this. (I suspect @sjmc7 and @ttripp would also be willing to help out too.)

@bertjwregeer
Member

@sigmavirus24 appreciate that, I have tagged issues for 1.6 release with the milestone 1.6, they are visible here: https://github.com/Pylons/webob/milestones/Version%201.6

All of them are currently PR's that just need someone to verify A. that they fix the issue at hand, and B. that they don't cause any backwards incompatible changes (and if it does, is there a way to do it in a BW-compat way?).

Even something as simple as pulling them down, verifying they work correctly, and then adding a +1 to the PR would be very helpful!

@sjmc7
sjmc7 commented Jan 28, 2016

For what it's worth, i tested our service against the current master (bbca6e8) and everything seems to work as expected, though we don't use a lot of the features in that list. In particular, the change sigmavirus24 made seems to work as expected.

@sigmavirus24
Contributor

@sjmc7 can you pull the other changes in the 1.6 milestone and test your service against that all merged into master?

@sjmc7
sjmc7 commented Jan 28, 2016

Yep, I pulled the current master, not just this patch.

@sigmavirus24
Contributor

@sjmc7 what about merging the pull requests in that milestone to master in your clone (this has already been merged). You could at least verify those don't seem to break backwards compataibility.

@sjmc7
sjmc7 commented Jan 28, 2016

Ah, I'm sorry, i misunderstood. Been using gerrit for too long. I'll give them a try.

@bertjwregeer bertjwregeer added a commit that referenced this pull request Jan 28, 2016
@bertjwregeer bertjwregeer Add CHANGES for #230 and move HISTORY 6196215
@sjmc7
sjmc7 commented Jan 28, 2016

Ok, tested with all the patches in the 1.6 milestone and don't have any regressions. Thanks for setting me straight.

@bertjwregeer
Member

Awesome, good to know. I'm slowly going through them now and merging them, as well as doing the necessary work to document the changes.

@bertjwregeer
Member

@sjmc7 @sigmavirus24 Released 1.6.0a0, please test on projects and let me know if there are any issues :-).

Thanks for all the help guys!

@sjmc7
sjmc7 commented Jan 29, 2016

Done some manual testing and run our tests on 1.6.0a0, and didn't find any problems (though as i say, we don't exercise a huge number of features). Thanks both of you for getting this done!

@sigmavirus24
Contributor

I'll try to run some other project's tests against 1.6.0a0 this weekend. Thanks @bertjwregeer

@sigmavirus24
Contributor

So apparently the decision in this PR that decides on returning HTML or Plain text has caused some minor backwards incompatibility. Running tests with glance and 1.6.0a0 showed that our (overly specific) tests were receiving plain text instead of HTML in some cases. (I haven't debugged further to determine why though.)

Also @flaper87 should be able to let us know if he saw any other test failures.

@bertjwregeer
Member

@sigmavirus24 Before I ship 1.6.0, are there any changes that need to happen, or is this something that needs to be fixed in the Glance tests?

@sigmavirus24
Contributor

Both? So I'm not sure 1.6.0 should ship with a (minor) backwards incompatibility although I still haven't looked at the failing test cases that @flaper87 found. I also think that Glance's tests (as I hinted at above) are a bit too ... specific and are testing things they shouldn't be.

@sigmavirus24
Contributor

That said, if you're content with 1.6.0 having what seems like it might be a minor backwards incompatibility, you could always fix that with 1.6.1 if anyone else runs into trouble with it.

@mmerickel
Member

I personally think it's a little silly to assume that webob can never change the format of the bundled exceptions. The library provides ways to override the format among other things if that's mission-critical. Most tools will ignore the content anyway and focus on the status code. Anyway it seems to me the change is very minor and acceptable.

@sigmavirus24
Contributor

@mmerickel changing the format is one thing. Returning a totally different content-type is another, I'd think. Again, I'm only arguing this because this was clearly the result of my change. I'm not going to stop y'all from releasing 1.6.0 though.

@bertjwregeer
Member

The content-type returned should be based upon what the original requestor asked for. If this is not the case, then things need to be changed, but if it is correct currently and the tests are failing because WebOb is now arguably doing the correct thing, then that's something we need to take into account.

@flaper87
flaper87 commented Feb 2, 2016

I didn't see other tests break other than the ones I reported. Those tests, as @sigmavirus24 mentioned are extremely specific and they could be changed. @bertjwregeer When is the release scheduled? I'd appreciate a couple of days for us to fix them before the release.

@bertjwregeer
Member

Is the behaviour that WebOb has correct? That's what I want to make sure of first and foremost. If you guys are expecting a different response and that is broken with the new code, why is that the case? Is the Accept matching not accurate?

@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this pull request Feb 3, 2016
@mira-nomad @openstack-gerrit mira-nomad + openstack-gerrit Updated openstack/openstack
Project: openstack/glance  5e3c0cf8c9e31024d5dbde43728e3b5defb9f6a3

Change exception format checks in artifact tests

Some tests for glance artifacts make some assumptions about
exception format from WebOp library. That can cause issues if
exception format is changed (we faced with that here
Pylons/webob#230). So we need just check
if some info is present in exception message and don't make any
assumptions about exception format.

Change-Id: I284842de7786ab18bc892aa27fd7985896d4f4de
3b8138d
@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this pull request Feb 3, 2016
@mira-nomad @openstack-gerrit mira-nomad + openstack-gerrit Updated openstack/openstack
Project: openstack/glance  5e3c0cf8c9e31024d5dbde43728e3b5defb9f6a3

Change exception format checks in artifact tests

Some tests for glance artifacts make some assumptions about
exception format from WebOp library. That can cause issues if
exception format is changed (we faced with that here
Pylons/webob#230). So we need just check
if some info is present in exception message and don't make any
assumptions about exception format.

Change-Id: I284842de7786ab18bc892aa27fd7985896d4f4de
25b262a
@openstack-gerrit openstack-gerrit pushed a commit to openstack/glance that referenced this pull request Feb 3, 2016
@mira-nomad mira-nomad Change exception format checks in artifact tests
Some tests for glance artifacts make some assumptions about
exception format from WebOp library. That can cause issues if
exception format is changed (we faced with that here
Pylons/webob#230). So we need just check
if some info is present in exception message and don't make any
assumptions about exception format.

Change-Id: I284842de7786ab18bc892aa27fd7985896d4f4de
5e3c0cf
@bertjwregeer
Member

@FlaPer87 @sigmavirus24 @sjmc7 Are you guys okay with the changes moving forward?

I noticed that the tests that have changed were expecting HTML, when their accept headers stated they wanted JSON (which this change fixes for the better). Are we good to ship this?

@bertjwregeer bertjwregeer removed the backport label Feb 22, 2016
@flaper87

@bertjwregeer We are good to ship this, those tests have been fixed. Thanks a bunch for waiting on us and following-up! You just won a cake: 🍰

@sjmc7
sjmc7 commented Feb 24, 2016

Yep, we're good with it too, thanks!

@sigmavirus24
Contributor

:shipit:

@bertjwregeer
Member

Cheers! Will ship soon.

@ttripp
ttripp commented Feb 24, 2016

\O/

From: Bert JW Regeer <notifications@github.commailto:notifications@github.com>
Reply-To: Pylons/webob <reply@reply.github.commailto:reply@reply.github.com>
Date: Wednesday, February 24, 2016 at 2:44 PM
To: Pylons/webob <webob@noreply.github.commailto:webob@noreply.github.com>
Cc: Travis Tripp <travis.tripp@hp.commailto:travis.tripp@hp.com>
Subject: Re: [webob] Add ability to return a JSON body from an exception (#230)

Cheers! Will ship soon.


Reply to this email directly or view it on GitHubhttps://github.com/Pylons/webob/pull/230#issuecomment-188493646.

@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this pull request Mar 30, 2016
@openstack-gerrit Jenkins + openstack-gerrit Updated openstack/openstack
Project: openstack/python-novaclient  8c204ac1a27f7b645e151d67ee38a4c18dd2201b

Handle error response for webob>=1.6.0

WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.

Change-Id: If653a247d842786d2824b4b3a5c0cde1383ed7ab
Closes-Bug: #1559072
999ee4d
@openstack-gerrit openstack-gerrit pushed a commit to openstack/python-novaclient that referenced this pull request Mar 30, 2016
@mriedem mriedem Handle error response for webob>=1.6.0
WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.

Change-Id: If653a247d842786d2824b4b3a5c0cde1383ed7ab
Closes-Bug: #1559072
fa377e7
@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this pull request Mar 30, 2016
@openstack-gerrit Jenkins + openstack-gerrit Updated openstack/openstack
Project: openstack/python-novaclient  8c204ac1a27f7b645e151d67ee38a4c18dd2201b

Handle error response for webob>=1.6.0

WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.

Change-Id: If653a247d842786d2824b4b3a5c0cde1383ed7ab
Closes-Bug: #1559072
0134d5a
@bertjwregeer bertjwregeer added a commit that referenced this pull request Apr 14, 2016
@bertjwregeer bertjwregeer Change the default returned type to text/html
Currently if WebOb encounters a */* Accept header on an exception it
will return an application/json result. This leads to some interesting
results for crawlers and others that don't send an explicit HTTP Accept
and then end up receiving JSON instead of what was previously HTML.

This breaks B/W compat with issue #230.
6d5e028
@openstack-gerrit openstack-gerrit pushed a commit to openstack/python-novaclient that referenced this pull request Oct 5, 2016
@mriedem mriedem + Alexis Lee Handle error response for webob>=1.6.0
WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.

Change-Id: If653a247d842786d2824b4b3a5c0cde1383ed7ab
Closes-Bug: #1559072
70c35e6
@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this pull request Dec 11, 2016
@openstack-gerrit Jenkins + openstack-gerrit Updated openstack/openstack
Project: openstack/python-cinderclient  702988b8b94bc9112f62281b5d30ad70a16e142f

Handle error response for webob>=1.6.0

WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.Handle error response for
webob>=1.6.0

Change-Id: I7d589415aa024588faf77c8234ac026110f6c3cd
Closes-Bug: #1559072
cec4fbf
@openstack-gerrit openstack-gerrit pushed a commit to openstack/python-cinderclient that referenced this pull request Dec 11, 2016
Akira KAMIO Handle error response for webob>=1.6.0
WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.Handle error response for
webob>=1.6.0

Change-Id: I7d589415aa024588faf77c8234ac026110f6c3cd
Closes-Bug: #1559072
19befa6
@openstack-gerrit openstack-gerrit added a commit to openstack/openstack that referenced this pull request Dec 11, 2016
@openstack-gerrit Jenkins + openstack-gerrit Updated openstack/openstack
Project: openstack/python-cinderclient  702988b8b94bc9112f62281b5d30ad70a16e142f

Handle error response for webob>=1.6.0

WebOb change Pylons/webob#230 changed
the way in which the error response body is formatted such that
it's no longer a nested dict. So we have to handle both the
old convention of an error message key to the response body error
dict and the new way with just the error body dict.

This was reported upstream:

Pylons/webob#235

But given this was apparently implemented as a long-overdue change
in WebOb the behavior is not likely to change.Handle error response for
webob>=1.6.0

Change-Id: I7d589415aa024588faf77c8234ac026110f6c3cd
Closes-Bug: #1559072
c1232ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment