Fix StaticResource routes not resolving#12255
Fix StaticResource routes not resolving#12255BlindChickens wants to merge 2 commits intoaio-libs:masterfrom
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #12255 +/- ##
==========================================
- Coverage 99.11% 99.10% -0.01%
==========================================
Files 130 130
Lines 45432 45435 +3
Branches 2400 2399 -1
==========================================
+ Hits 45028 45030 +2
Misses 273 273
- Partials 131 132 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
BlindChickens
force-pushed
the
fix_static_resource_routes_not_resolving
branch
2 times, most recently
from
b7dc460 to
5628d5d
Compare
BlindChickens
force-pushed
the
fix_static_resource_routes_not_resolving
branch
from
5628d5d to
6b832fb
Compare
| resource = router.add_static( | ||
| "/st", pathlib.Path(aiohttp.__file__).parent, name="static" | ||
| ) | ||
| resource.add_route(hdrs.METH_POST, make_handler()) | ||
| mapping, allowed_methods = await resource.resolve( | ||
| make_mocked_request("POST", "/st/path") |
There was a problem hiding this comment.
Sorry, what is happening here?
It looks to me like you're trying to add a POST handler to a static resource. I can't understand how that makes any sense.
Can you please start by describing the issue you are supposedly fixing? Or the use case of this? As the documentation says, .add_static() is meant to emulate something similar to nginx static file handling, for use in local development only. Adding a POST handler makes no sense to me.
There was a problem hiding this comment.
I understand what you are saying. The POST in the test case would be confusing. But there are other methods, so I can change it to something else, if that would help.
But to me it seems that you feel this cannot be useful because of this: .add_static() is meant to emulate something similar to nginx static file handling.
So if that's the case, then there is no use keeping this open.
If not, if I change the test case to add any other method handler through the new interface, would it be acceptable?
There was a problem hiding this comment.
If not, if I change the test case to add any other method handler through the new interface, would it be acceptable?
I just don't understand what you're trying to achieve... A StaticResource should resolve to a file on the filesystem, why would you want to try adding a custom handler to it? It seems to me like you should be using a regular PlainResource through the router.add_post() etc. methods (https://docs.aiohttp.org/en/stable/web_reference.html#aiohttp.web.UrlDispatcher.add_routes).
There was a problem hiding this comment.
So I did this purely because we had a regression when updating past #9911, and when I investigated and saw that it also regressed something done in aiohttp-cors as indicated in the PR description of #9976.
aiohttp-cors accesses the protected self._routes here and relies on being able to add new methods
So I was like, oh, they also want to add methods, let's fix that.
Our stuff that broke is obviously legacy shenanigans and we need to change it. But for now I just added an extra line: self._allowed_methods.add("POST"), because you know stuff needs to keep moving.
There was a problem hiding this comment.
So I was like, oh, they also want to add methods, let's fix that.
OK, I understand now. I think we probably want to reevaluate the aiohttp-cors integration before deciding that we want to add this to the public API. So, we'll probably avoid merging this PR currently until we have time to evaluate that properly.
2 participants
What do these changes do?
Move the
_routes,_any_routeand_allowed_methodsproperties toAbstractResource.Move the
add_routeand register_routemethods to theAbstractResource`.Are there changes in behavior for the user?
They can now add a route method to a static resource without resorting to the internal properties.
Before #9911, and then subsequently #9976 you could have added a method to the internal
_routesproperty and it would resolve, afterwards, you also had to append the method to the internal_allowed_methodsproperty to resolve, except if the method you wanted to add wasOPTIONS, and you did it via theset_options_routemethod.Is it a substantial burden for the maintainers to support this?
Well I have added method higher up in the abstraction layer.
But the way it all looks to me, it seems it belonged there anyway.
Related issue number
Checklist
CONTRIBUTORS.txtCHANGES/foldername it
<issue_or_pr_num>.<type>.rst(e.g.588.bugfix.rst)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature: A new behavior, public APIs. That sort of stuff..deprecation: A declaration of future API removals and breakingchanges in behavior.
.breaking: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc: Notable updates to the documentation structure or buildprocess.
.packaging: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.