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

StaticResource only matches folder-like URL prefix #5534

Merged
merged 10 commits into from Oct 30, 2021

Conversation

danking
Copy link
Contributor

@danking danking commented Mar 10, 2021

What do these changes do?

I think this fixes #5250.

A static resource like:

routes.static('/foo', '/www/foo')

currently matches /foobar but returns 404. This PR ensures that aforementioned static route does not match /foobar

Are there changes in behavior for the user?

If a user expected /foobar to load the file /www/foo/ar, this PR will violate their expectation.

Related issue number

Fixes #5250

Checklist

I'm not aware of any docs that need to change, but I didn't tick that box because I am not certain.

  • 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."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Mar 10, 2021
@Dreamsorcerer
Copy link
Member

The fact that /foobar is translated to /ar seems surprising, so I'd be inclined to say that it's unlikely that people are depending on this behaviour. Though it could be possible that someone uses paths like /static-filename...

I wonder if the prefix argument should be renamed to path though, to avoid confusion if we do change it...

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #5534 (519539b) into master (9220703) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5534   +/-   ##
=======================================
  Coverage   93.31%   93.31%           
=======================================
  Files         102      102           
  Lines       30313    30325   +12     
  Branches     2723     2724    +1     
=======================================
+ Hits        28287    28299   +12     
  Misses       1849     1849           
  Partials      177      177           
Flag Coverage Δ
unit 93.24% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiohttp/web_urldispatcher.py 97.55% <100.00%> (+<0.01%) ⬆️
tests/test_web_urldispatcher.py 98.14% <100.00%> (+0.07%) ⬆️

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 9220703...519539b. Read the comment docs.

@danking
Copy link
Contributor Author

danking commented Mar 23, 2021

OK, I’m fairly certain this is ready for review. Again, I don’t think the docs need to change because I think of this as a bug fix, but I’m not sure.

@Dreamsorcerer
Copy link
Member

@webknjaz Thoughts on this? Seems to resolve some surprising behaviour.
But, looks like it also adds a little overhead, which I assume is running on every request.

Maybe a more efficient solution would be to ensure that that static resource is always at the end of the list, so it's the last one checked? This sounds reasonable to me, as a web server should be handling static assets on a production system where performance matters, therefore the static resource would never be useful.

CHANGES/5250.bugfix Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

@Dreamsorcerer I'm not sure about that. Making the priority assumptions should probably be on the user. Besides, this would be a breaking change for people who used to rely on the order.

CHANGES/5250.bugfix Outdated Show resolved Hide resolved
@asvetlov asvetlov enabled auto-merge (squash) October 30, 2021 15:43
@asvetlov asvetlov merged commit 9c7f3d3 into aio-libs:master Oct 30, 2021
@patchback
Copy link
Contributor

patchback bot commented Oct 30, 2021

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/9c7f3d37fd9ad2aea4d7f382062d52dca9d2bd89/pr-5534

Backported as #6179

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Oct 30, 2021
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
(cherry picked from commit 9c7f3d3)
asvetlov added a commit that referenced this pull request Oct 30, 2021
…like URL prefix (#6179)

* StaticResource only matches folder-like URL prefix (#5534)

Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
(cherry picked from commit 9c7f3d3)

* fix

Co-authored-by: Dan King <daniel.zidan.king@gmail.com>
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

router.add_static is greedy
4 participants