Fix bug in route name validation (#4691) #4692

gorogoroumaru · 2020-04-14T06:39:19Z

The register_resource function in aiohttp/web_urldispatcher.py had a bug in route name validation.
It validated each part of the splitted route name whether it's a python keyword.
I fixed the function to validate route name before splitting it and added a new error message.

codecov-io · 2020-04-14T06:44:50Z

Codecov Report

Merging #4692 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #4692      +/-   ##
==========================================
- Coverage   97.60%   97.58%   -0.02%     
==========================================
  Files          43       43              
  Lines        8920     8931      +11     
  Branches     1406     1407       +1     
==========================================
+ Hits         8706     8715       +9     
- Misses         95       96       +1     
- Partials      119      120       +1

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`98.79% <33.33%> (-0.30%)`	⬇️
aiohttp/web.py	`99.05% <0.00%> (+<0.01%)`	⬆️
aiohttp/client_reqrep.py	`97.04% <0.00%> (+0.03%)`	⬆️

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 16a49c1...83f1ea2. Read the comment docs.

webknjaz · 2020-04-14T16:27:44Z

aiohttp/web_urldispatcher.py

+                raise ValueError('Incorrect route name {!r}, '
+                                 'python keywords cannot be used '
+                                 'for route name'.format(name))


FYI this could be an f-string

Suggested change

raise ValueError('Incorrect route name {!r}, '

'python keywords cannot be used '

'for route name'.format(name))

raise ValueError(f'Incorrect route name {name!r}, '

'python keywords cannot be used '

'for route name')

webknjaz · 2020-04-14T16:28:40Z

aiohttp/web_urldispatcher.py

@@ -977,9 +977,13 @@ def register_resource(self, resource: AbstractResource) -> None:
        name = resource.name

        if name is not None:
+            if keyword.iskeyword(name):


Should we trim whitespaces first?

Suggested change

if keyword.iskeyword(name):

if keyword.iskeyword(name.strip()):

webknjaz

Hey @gorogoroumaru, Thanks for the PR!

This looks reasonable but to be accepted we need a regression test and a change fragment to be included in the PR. Also, please complete the PR template https://github.com/aio-libs/aiohttp/blob/master/.github/PULL_REQUEST_TEMPLATE.md.

gorogoroumaru · 2020-04-15T00:48:53Z

Thanks!
I applied your suggested change.

webknjaz · 2020-04-16T00:18:59Z

Great, waiting for the test, then!

gorogoroumaru · 2020-04-20T11:53:33Z

Test completed!

webknjaz

Currrently this PR contains unrelated changes, please rebase it on top of origin/master.

tests/test_web_urldispatcher.py

webknjaz

The PR needs rebasing so that unrelated changes made by someone else don't show up here.

webknjaz · 2020-04-28T12:44:15Z

tests/test_web_urldispatcher.py

+
+    wrong_name = "for"
+    resource = PlainResource(url.raw_path, name=wrong_name)
+    with pytest.raises(ValueError):


You have to use match= arg as show @ #4692 (comment)

webknjaz · 2020-04-28T12:46:11Z

tests/test_web_urldispatcher.py

+
+    wrong_name = "for"
+    resource = PlainResource(url.raw_path, name=wrong_name)
+    with pytest.raises(ValueError):
+        router.register_resource(resource)


This part is a separate test and so it must be in a separate test function.

webknjaz · 2020-04-28T12:48:03Z

.github/ISSUE_TEMPLATE/config.yml

@@ -1,29 +1,12 @@
 # Ref: https://help.github.com/en/github/building-a-strong-community/configuring-issue-templates-for-your-repository#configuring-the-template-chooser
 blank_issues_enabled: false  # default: true
 contact_links:
- name: 🔐 Security bug report 🔥


Things like this mustn't be a part of the PR.

Sorry, I wrongly operated git command and included someone else's commit to my PR.

# Conflicts: # tests/test_web_urldispatcher.py

gorogoroumaru · 2020-04-29T00:28:58Z

I finished rebasing PR and applying suggested changes.

asvetlov · 2020-10-15T16:23:17Z

Thanks for the PR.
I've ported your changes to a little bit more correct #5051

check if the route name is a python keyword ,before splitting it

7d0e11e

gorogoroumaru requested a review from asvetlov as a code owner April 14, 2020 06:39

webknjaz reviewed Apr 14, 2020

View reviewed changes

webknjaz requested changes Apr 14, 2020

View reviewed changes

Add change log

120d69d

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 15, 2020

gorogoroumaru and others added 9 commits April 17, 2020 09:46

Add a test

fd97f85

Drop redundant items from issue template types

77e9933

Add a test

534ce1a

Add a test

c95357e

Add a test

795affa

Add a test

7f00085

Add a test

f14584b

Add a test

918d921

Add a test

324d3c5

gorogoroumaru requested a review from webknjaz April 19, 2020 00:16

webknjaz requested changes Apr 20, 2020

View reviewed changes

webknjaz reviewed Apr 20, 2020

View reviewed changes

tests/test_web_urldispatcher.py Outdated Show resolved Hide resolved

gorogoroumaru added 3 commits April 28, 2020 14:21

fixed test

2ee1263

fixed test

8d7721f

fixed test

4cde2e2

webknjaz requested changes Apr 28, 2020

View reviewed changes

webknjaz reviewed Apr 28, 2020

View reviewed changes

gorogoroumaru added 3 commits April 28, 2020 22:36

issue4691 bugfix

02c7d94

fix gen.py check_changes.py config.yml

cb16705

Merge remote-tracking branch 'origin/issue4691' into issue4691

83f1ea2

# Conflicts: # tests/test_web_urldispatcher.py

asvetlov closed this Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in route name validation (#4691) #4692

Fix bug in route name validation (#4691) #4692

gorogoroumaru commented Apr 14, 2020

codecov-io commented Apr 14, 2020 •

edited

Loading

webknjaz Apr 14, 2020

This comment was marked as resolved.

webknjaz Apr 14, 2020

webknjaz left a comment

gorogoroumaru commented Apr 15, 2020

webknjaz commented Apr 16, 2020

gorogoroumaru commented Apr 20, 2020

webknjaz left a comment

webknjaz left a comment

webknjaz Apr 28, 2020

webknjaz Apr 28, 2020

webknjaz Apr 28, 2020

gorogoroumaru Apr 28, 2020

gorogoroumaru commented Apr 29, 2020

asvetlov commented Oct 15, 2020

	if keyword.iskeyword(name):
	if keyword.iskeyword(name.strip()):

Fix bug in route name validation (#4691) #4692

Fix bug in route name validation (#4691) #4692

Conversation

gorogoroumaru commented Apr 14, 2020

codecov-io commented Apr 14, 2020 • edited Loading

Codecov Report

webknjaz Apr 14, 2020

Choose a reason for hiding this comment

This comment was marked as resolved.

webknjaz Apr 14, 2020

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

gorogoroumaru commented Apr 15, 2020

webknjaz commented Apr 16, 2020

gorogoroumaru commented Apr 20, 2020

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz left a comment

Choose a reason for hiding this comment

webknjaz Apr 28, 2020

Choose a reason for hiding this comment

webknjaz Apr 28, 2020

Choose a reason for hiding this comment

webknjaz Apr 28, 2020

Choose a reason for hiding this comment

gorogoroumaru Apr 28, 2020

Choose a reason for hiding this comment

gorogoroumaru commented Apr 29, 2020

asvetlov commented Oct 15, 2020

codecov-io commented Apr 14, 2020 •

edited

Loading