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

@dtkav dtkav 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 January 25, 2019 00:57
@dtkav
Copy link
Contributor Author

dtkav 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
Copy link
Member

Bump CI

@codecov-io
Copy link

codecov-io 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
@asvetlov
Copy link
Member

Thanks!

asvetlov pushed a commit that referenced this pull request Jun 11, 2019
(cherry picked from commit b80fec6)

Co-authored-by: Daniel Grossmann-Kavanagh <[email protected]>
webknjaz pushed a commit that referenced this pull request Jun 11, 2019
…. (#3835)

(cherry picked from commit b80fec6)

Co-authored-by: Daniel Grossmann-Kavanagh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants