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

WIP: Change normalize_path_middleware redirect from 301 -> 308 #3579

Merged
merged 1 commit into from Jun 11, 2019

Conversation

@dtkav
Copy link
Contributor

commented Jan 25, 2019

Redirect path normalization with 308 Permanent Redirect rather than 301 Moved Permanently.

The default redirect class is currently HTTPMovedPermanently, which works for GET and HEAD requests, but does not work well for other HTTP methods. Clients will typically handle a 301 response by changing the verb to GET on redirect.

Related RFCs:

  • RFC 2616 section-10.3.2 stated that changing the method was an implementation error
  • RFC 7231 formally allowed changing the method on 301/302/303.
  • RFC 7538 introduced 308 Permanent Redirect to work as 301 was originally intended.

Related aiohttp client behavior:
#3082 - clients being redirected POST -> GET (closed as wontfix)
#2134 - aiohttp client implementation of 308 Permanent Redirect

Are there changes in behavior for the user?

Users will no longer lose POST data during a redirect caused by a missing trailing slash.

Related issue number

#3578

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@dtkav dtkav requested a review from asvetlov as a code owner Jan 25, 2019

@dtkav dtkav force-pushed the dtkav:master branch from 6892d31 to a72e3b8 Jan 25, 2019

@dtkav dtkav requested a review from webknjaz as a code owner Jan 25, 2019

@dtkav

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

Regarding unit tests, I can imagine a couple potential paths:

  1. Leave as is, this is a default configuration change
  2. Check that we get a 308 redirect explicitly
  3. Add a functional test that ensures POST works correctly with a path redirect (tests client and server)

Let me know what you think! I'll leave this as a WIP until I have a path forward for testing.

@asvetlov asvetlov closed this May 13, 2019

@asvetlov asvetlov reopened this May 13, 2019

@asvetlov

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Bump CI

@codecov-io

This comment has been minimized.

Copy link

commented May 13, 2019

Codecov Report

Merging #3579 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3579      +/-   ##
==========================================
+ Coverage    97.9%   97.93%   +0.02%     
==========================================
  Files          43       43              
  Lines        8560     8569       +9     
  Branches     1378     1375       -3     
==========================================
+ Hits         8381     8392      +11     
  Misses         74       74              
+ Partials      105      103       -2
Impacted Files Coverage Δ
aiohttp/web_middlewares.py 100% <100%> (ø) ⬆️
aiohttp/streams.py 98.2% <0%> (-0.52%) ⬇️
aiohttp/web_request.py 96.99% <0%> (-0.45%) ⬇️
aiohttp/http_websocket.py 98.62% <0%> (-0.03%) ⬇️
aiohttp/web_app.py 98.85% <0%> (-0.03%) ⬇️
aiohttp/tracing.py 100% <0%> (ø) ⬆️
aiohttp/cookiejar.py 100% <0%> (ø) ⬆️
aiohttp/__init__.py 100% <0%> (ø) ⬆️
aiohttp/web_protocol.py 92.76% <0%> (+0.12%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c145bd8...a72e3b8. Read the comment docs.

@asvetlov asvetlov merged commit b80fec6 into aio-libs:master Jun 11, 2019

3 of 5 checks passed

WIP Title contains "WIP"
Details
license/cla Contributor License Agreement is not signed yet.
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 100% of diff hit (target 97.9%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@asvetlov

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Thanks!

asvetlov added a commit that referenced this pull request Jun 11, 2019
[3.5] Change normalize_path_middleware redirect from 301 -> 308 (#3579).
(cherry picked from commit b80fec6)

Co-authored-by: Daniel Grossmann-Kavanagh <kavanagh.daniel@gmail.com>
webknjaz added a commit that referenced this pull request Jun 11, 2019
[3.5] Change normalize_path_middleware redirect from 301 -> 308 (#3579)…
…. (#3835)

(cherry picked from commit b80fec6)

Co-authored-by: Daniel Grossmann-Kavanagh <kavanagh.daniel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.