Do not use translated path by william-gr · Pull Request #3051 · aio-libs/aiohttp

william-gr · 2018-06-04T17:57:59Z

What do these changes do?

Allow redirect to work when using url encoded paths and normalize path middleware, e.g.

/foo/bar%2Fbar will redirect to /foo/bar%2Fbar/ instead of /foo/bar/bar/

Are there changes in behavior for the user?

Not as far as I am aware, since its a bug fix.

codecov-io · 2018-06-04T18:15:45Z

Codecov Report

Merging #3051 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #3051   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files          42       42           
  Lines        7743     7743           
  Branches     1349     1349           
=======================================
  Hits         7597     7597           
  Misses         51       51           
  Partials       95       95

Impacted Files	Coverage Δ
aiohttp/web_middlewares.py	`100% <100%> (ø)`	⬆️

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 e641373...b45080c. Read the comment docs.

webknjaz · 2018-06-04T18:18:58Z

Please fill out the PR template, so that it would be easier to understand why and what you are fixing.

asvetlov · 2018-06-05T12:48:00Z

Sorry, I don't follow what is failed and what is expected behavior.

Unit test and changelog entry are required.

webknjaz · 2018-06-05T14:48:46Z

https://raw.githubusercontent.com/aio-libs/aiohttp/master/.github/PULL_REQUEST_TEMPLATE.md

william-gr · 2018-06-05T18:08:55Z

@asvetlov @webknjaz PR updated and summary changed to follow template.

Let me know if something is missing.

Thanks

Redirect would fail in case we have encoded url, e.g. /foo/bar%2Fbar -> /foo/bar/bar/

william-gr · 2018-06-07T15:58:03Z

Ping. Just want to make sure there isn't anything missing here. I will be travelling the next few days and want to leave it ready for a review.

asvetlov · 2018-06-07T19:29:10Z

aiohttp/web_middlewares.py

                resolves, request = await _check_request_resolves(
                    request, path)
                if resolves:
-                    raise redirect_class(request.path + query)


I wonder if request.path can be replaced with request.raw_path

Thats the same as path, no? request.raw_path is being used above to populate paths_to_check.

No. It is the percent encoded canonical path representation.

And thats exactly the issue I am solving here, and its passing all the tests so I dont get it.

So replace the call please

william-gr · 2018-06-08T11:19:56Z

@asvetlov call updated. Thanks for the helpful explanation as of why.

asvetlov · 2018-06-09T09:53:20Z

thanks

lock · 2019-10-28T04:04:05Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

william-gr force-pushed the issues/normalize_raw branch from 1e4914a to 8e7590a Compare June 5, 2018 18:06

Do not use translated path

b6fd755

Redirect would fail in case we have encoded url, e.g. /foo/bar%2Fbar -> /foo/bar/bar/

william-gr force-pushed the issues/normalize_raw branch from 8e7590a to b6fd755 Compare June 5, 2018 18:10

asvetlov reviewed Jun 7, 2018

View reviewed changes

Use request.raw_path as mandated by @asvetlov

b45080c

asvetlov approved these changes Jun 9, 2018

View reviewed changes

asvetlov merged commit 4b54daf into aio-libs:master Jun 9, 2018

william-gr deleted the issues/normalize_raw branch June 12, 2018 16:04

ghost mentioned this pull request Aug 26, 2018

aiohttp versions available: 3.4.0 Harmon758/Harmonbot#301

Closed

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Uh oh!

Conversation

william-gr commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Uh oh!

codecov-io commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

webknjaz commented Jun 4, 2018

Uh oh!

asvetlov commented Jun 5, 2018

Uh oh!

webknjaz commented Jun 5, 2018

Uh oh!

william-gr commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

william-gr commented Jun 7, 2018

Uh oh!

asvetlov Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

william-gr Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

asvetlov Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

william-gr Jun 7, 2018

Choose a reason for hiding this comment

Uh oh!

asvetlov Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

william-gr commented Jun 8, 2018

Uh oh!

asvetlov commented Jun 9, 2018

Uh oh!

lock bot commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

william-gr commented Jun 4, 2018 •

edited

Loading

codecov-io commented Jun 4, 2018 •

edited

Loading

william-gr commented Jun 5, 2018 •

edited

Loading