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

Update and fix Accept* headers handling #338

Merged
merged 51 commits into from Nov 20, 2017

Conversation

Projects
None yet
4 participants
@whiteroses
Copy link
Contributor

whiteroses commented Aug 29, 2017

This PR is for the work on the Accept, Accept-Charset and Accept-Encoding headers, and any further work on the Accept-Language header, following on from this merged PR.

whiteroses added some commits Aug 29, 2017

Rewrite AcceptCharset class, and add docs and tests.
From the old AcceptCharset.parse: 'ISO-8859-1' was a default charset in
early versions of HTTP/1.1, but this is no longer the case
(https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Charset)

whiteroses added some commits Aug 29, 2017

Improve AcceptLanguageValidHeader.basic_filtering.
The main change here is to simplify the handling of duplicate ranges in
the header. The RFC does not specify what to do when the same range
appears in the header more than once, possibly with different qvalues
and giving conflicting information. Previously, we chose which of the
possibly conflicting versions of the range to use for matching by first
checking if there was one with q=0, which would take priority; if one
with q=0 was not found, we chose the one with the highest qvalue.

Having given it a lot more thought, it now seems clear that this kind of
special handling is not necessary -- the simplest way to handle it is to
take the first appearance of the range in the header (from the left) and
simple use that for matching.
@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Sep 12, 2017

I haven't forgotten about this! Will take a look tomorrow. Thanks @whiteroses!

@whiteroses

This comment has been minimized.

Copy link
Contributor

whiteroses commented Sep 13, 2017

Thanks @bertjwregeer! I've been working on Pylons/pyramid#1264, but the documentation changes are coming!

@@ -1859,131 +5102,3 @@ def fdel(request):
pass

return property(fget, fset, fdel, textwrap.dedent(doc))


class MIMEAccept(Accept):

This comment has been minimized.

@bertjwregeer

bertjwregeer Oct 30, 2017

Member

We should keep this around, even if it is just a shim that calls the new classes/methods. MIMEAccept is pretty heavily used by various other libraries/tools that depend on WebOb.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Oct 30, 2017

MIMEAccept = Accept

May be acceptable if the API hasn't changed.

@whiteroses

This comment has been minimized.

Copy link
Contributor

whiteroses commented Nov 5, 2017

I think it would need to be

MIMEAccept = AcceptValidHeader

but the API hasn't changed (other than the new acceptable_offers method) — is that ok? The other libraries/tools don't use MIMENilAccept, NilAccept or NoAccept directly, do they? (These classes were not documented.)

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Nov 5, 2017

Since they were not documented, I'm less worried. MIMEAccept on the other hand was heavily used.

Your suggestion of MIMEAccept = AcceptValidHeader would be fine by me.

@whiteroses

This comment has been minimized.

Copy link
Contributor

whiteroses commented Nov 6, 2017

Ok, I've made the change.

(Have been wanting to work on the documentation changes and other follow-up work from GSoC, but have been really busy — may have more time in the second half of this month and in December though.)

@bertjwregeer bertjwregeer merged commit 503f4f8 into Pylons:master Nov 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryanpetrello

This comment has been minimized.

Copy link
Contributor

ryanpetrello commented Jan 29, 2018

I know I'm a bit late to the party on this one, and I applaud you all for the work you've done improving Accept header support here.

I work in open source, and I don't mean to be critical or diminish the work that you all are doing (many of you as volunteers, which I also understand). I know that it can be difficult - and sometimes impossible - to improve software without some breaking API changes. I do wish, however, that this sort of backwards-incompatible change could have gone into a major version release instead of 1.7 -> 1.8 (though admittedly this could be a matter of semantics, depending what the webob team considers "major versions"), or at the very least involved some form of deprecation period (maybe it did, and I'm not paying enough attention in the right places?)

Hunting down a bug I was encountering related to this change was fairly painful and surprising, and for now, I've ended up pinning to an earlier version of webob until I can update my code to work with these changes 😢

@stevepiercy

This comment has been minimized.

Copy link
Member

stevepiercy commented Jan 29, 2018

@ryanpetrello You can watch GitHub repos that affect your software so that you can receive notifications of updates. If that is too noisy, we also make announcements via Google Group pylons-discuss, and Twitter. And there is a well-maintained changelog on PyPI and WebOb's docs.

If you have suggestions for how to mitigate your pain and surprise apart from the foregoing options, please let us know.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jan 29, 2018

@ryanpetrello I put out a call for testing for 1.8, including finding out what was broken. I can't test all software that relies on WebOb. What in these changes caused you pain? What bug did it cause? If I knew what it was that broke, I may be able to find a way to provide a shim or the necessary compatibility to allow your code to continue to function.

Also, WebOb like Pyramid and many other Pylons Project projects do not use semantic versioning. And 1.7 -> 1.8 may deprecate public API's (which should at least warn about the upcoming deprecation). This part of the code base was some of the most hairy and difficult to make a transition without potential breaking changes, but as far as I know the public/documented API did not change (other than having a deprecation warning on them), internals are allowed to be changed (and have) as needed, if you rely on reading the WebOb source code and grabbing internal values we can't continue to support that.

@whiteroses

This comment has been minimized.

Copy link
Contributor

whiteroses commented Jan 30, 2018

@ryanpetrello We had many discussions at the time about what to change and where to keep backward compatibility (e.g. under this PR), and tried to keep any changes to the documented public API to a minimum (it's been a while, but I can't actually remember any significant changes to the documented API). You can see we've put in quite a few deprecation warnings for future changes that haven't happened yet. If there's an oversight or mistake on my part, please let us know and I'll (with @bertjwregeer's agreement) do my best to fix.

@ryanpetrello

This comment has been minimized.

Copy link
Contributor

ryanpetrello commented Jan 30, 2018

Hey @whiteroses @bertjwregeer - thanks for the replies. It looks like you all did a great job broadcasting these coming changes to the community, and that I just wasn't paying attention in the right channels. I've since subscribed to some of the resources you've mentioned and will pay closer attention in the future.

One specific aspect of this change seems to be tripping up code like this:

getattr(req.accept, 'header_value', '*/*')

It looks like this no longer behaves the same way for AcceptInvalidHeader and AcceptNoHeader as it did before with webob.acceptparse.MIMEAccept; now it evaluates to None (prior to 1.8 req.accept.header_value would equal "*/*" for requests with missing Accept headers), and that's what was causing issues for me.

internals are allowed to be changed (and have) as needed, if you rely on reading the WebOb source code and grabbing internal values we can't continue to support that.

I think, in the end, this is what it comes down to - you all made a change to the underlying implementation - something that's not considered part to the public API, and I had code that was relying on it. I think I've worked out a way to support what I'm doing in both webob 1.8 and prior, so I'm good to go on my end; thanks @bertjwregeer for offering to help me work it out.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jan 30, 2018

Excellent. I was actually just now looking through Pecan since I saw the pin commit come by, I assume you've also found the docs for the new classes in 1.8 here:

https://docs.pylonsproject.org/projects/webob/en/1.8-branch/api/webob.html#webob.acceptparse.AcceptValidHeader.acceptable_offers

In the case of no header being provided, the order that the server wishes to respond in will be taken:

https://docs.pylonsproject.org/projects/webob/en/1.8-branch/api/webob.html#webob.acceptparse.AcceptNoHeader.acceptable_offers

This allows the server to very easily even in the case of no accept header or */* to select the appropriate offer using:

clientaccepts = req.accept.acceptable_offers(['text/html', 'application/json'])

To get back a list of tuples.

Unless there is anything major, I will be releasing 1.8 within the next day or two. Since you mentioned you have a work-around that works for both 1.8 and older, I won't let this hold up the publishing of the new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment