Add path_pattern property to routes and resources #2993

pposca · 2018-05-11T14:00:52Z

The path_pattern is the raw path used to add a new route. For example,
/foo/bar/{name:\d+}.

Add path_pattern property to AbstractRoute and AbstractResource
Add path pattern implementation to PlainResource, DynamicResource,
PrefixResource, StaticResource, ResourceRoute and SystemRoute.
Add tests

What do these changes do?

Add path_pattern property to routes and resources.
The path_pattern is the raw path used to add a new route. For example, /bar/{name:\d+}.

Are there changes in behavior for the user?

The public API of Route and Resource has now a new property path_name to expose directly the raw path used to add a new route. Significant values have been selected for each implementation of the abstract classes.

Related issue number

2968

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

asvetlov · 2018-05-11T14:09:57Z

Thank for the PR.
We have 2 options: use regex pattern or formatter string.
The first is stricter, the second looks nicer: /foo/bar/{name}.

I have no personal opinion.
@aio-libs/aiohttp-committers what do you think?

kxepal · 2018-05-11T14:29:55Z

I'm for formatted string, or, actually, placeholders.

Regexps are more powerful, but with their power they move part of data validation on router side. Say, /user/:id vs /user/{id:\d+} - placeholder knowns nothing about what ids are valid and delegates their check to handler side. Regular expression tries to ensure that they are numeric ones. This may sounds sweet, but you still would do int(id) in handler and if that id is not valid you still would like to throw some exception back.

Also, I would like to note, that /foo/bar/{baz} syntax not quite common. The /foo/bar/:baz would be a bit more wide used.

pposca · 2018-05-11T15:21:29Z

As far as I've understood, regex patterns only exist in DynamicResource. If other resources return a string and DynamicResource returns a regex, it looks a bit like a broken API, inconsistent. It seems more logic to me that all resources return a string.

On the other hand, I think regex is more useful than formatted string, as /user/{id:\d+} provides more information than /user/{id}, and this is why I chose to return exactly the same string it is used when calling add_routes., whether it is a normal string (PlainResource) or a regex pattern string (DynamicResource)

PD: Sorry if I've misunderstood something. It is the first time I dive into aiohttp code.

asvetlov · 2018-05-11T15:28:23Z

@jettify could you help to make a decision?
It is your request anyway.

codecov-io · 2018-05-18T13:22:50Z

Codecov Report

Merging #2993 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2993      +/-   ##
==========================================
+ Coverage   98.09%   98.09%   +<.01%     
==========================================
  Files          42       42              
  Lines        7711     7718       +7     
  Branches     1345     1345              
==========================================
+ Hits         7564     7571       +7     
  Misses         51       51              
  Partials       96       96

Impacted Files	Coverage Δ
aiohttp/web_urldispatcher.py	`99.11% <100%> (+0.01%)`	⬆️
aiohttp/web_app.py	`99.1% <0%> (-0.01%)`	⬇️

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 af53fe0...5ff1917. Read the comment docs.

jettify · 2018-05-20T11:19:11Z

Sorry for late reply, was out of town. My use case is tracing system [1] where canonical name can be used for grouping spans, this also useful for metrics subsystems too to count number of requests per specific route, using path pattern as a key instead of full actual path (with user is for example).

For this purpose I think formatted string is better since it is easier to read.

[1] openzipkin/zipkin#1874

asvetlov

@pposca please update the PR:

Rename path_pattern to canonical (I don't like long complex names).
Use formatting template instead of regex.

pposca · 2018-05-21T12:17:13Z

@asvetlov , do you prefer a 2nd commit with the update or that I amend the first commit?

asvetlov · 2018-05-21T12:20:18Z

Don't care, PRs are squashed on merging anyway.

asvetlov

The documentation update is required.
The property audience is not very wide, perhaps changing web_reference.rst is enough.

asvetlov · 2018-05-22T20:25:58Z

aiohttp/web_urldispatcher.py

@@ -131,6 +140,15 @@ def handler(self):
    def name(self):
        """Optional route's name, always equals to resource's name."""

+    @property
+    @abc.abstractmethod
+    def canonical(self):


Should we have route.canonical?
route.resource.canonical should be enough.
route.name exists for historical reasons, routes were added before the resource invention.

The issue was asking for a route property, that's why I added it.

I will change it and I will update the docs (I completely forgot them...)

asvetlov · 2018-05-23T06:06:38Z

aiohttp/web_urldispatcher.py

@@ -457,6 +478,10 @@ def __init__(self, prefix, directory, *, name=None,
                        'HEAD': ResourceRoute('HEAD', self._handle, self,
                                              expect_handler=expect_handler)}

+    @property
+    def canonical(self):
+        return self._prefix + str(self._directory)


Looks like there is an error here.
Canonical URL should contain the prefix only, the directory is a local path.
It never returns in URLs but url_for() constructs a return value from the prefix and a relative to the directory path.

Ok, I will remove it from StaticResource, as canonical is already the prefix in PrefixResource and StaticResource will inherit the property.

asvetlov

Everything is perfect, the last neat is left.

asvetlov · 2018-05-23T18:54:13Z

docs/web_reference.rst

+
+      Read-only *canonical path* associate with the resource. For example
+      ``/path/to`` or ``/path/{to}``
+


.. versionadded:: 3.3

asvetlov · 2018-05-23T18:54:27Z

docs/web_reference.rst

+   .. attribute:: canonical
+
+      Read-only *canonical path* associate with the resource. Returns the path
+      used to create the PlainResource. For example ``/path/to``


versionadded

asvetlov · 2018-05-23T18:54:39Z

docs/web_reference.rst

+
+      Read-only *canonical path* associate with the resource. Returns the
+      formatter obtained from the path used to create the DynamicResource.
+      For example, from a path ``/get/{num:^\d+}``, it returns ``/get/{num}``


versionadded

asvetlov · 2018-05-23T18:54:48Z

docs/web_reference.rst

+
+      Read-only *canonical path* associate with the resource. Returns the prefix
+      used to create the StaticResource. For example ``/prefix``
+


versionadded

asvetlov · 2018-05-23T18:55:05Z

docs/web_reference.rst

+      Read-only *canonical path* associate with the resource. Returns the
+      prefix used to create the PrefixedSubAppResource.
+      For example ``/prefix``
+


versionadded

The canonical is the path used to add a new route. For example, /foo/bar/{name}. For DynamicResource, canonical exposes the formatter. - Add canonical property to AbstractResource - Add canonical implementation to DynamicResource, PrefixResource and StaticResource. - Add tests - Add docs Closes aio-libs#2968

asvetlov · 2018-05-27T10:18:58Z

Well done!
Thank you for patience.

pposca · 2018-05-27T10:20:31Z

My pleasure. Really happy to help.

jettify · 2018-06-09T19:51:51Z

Thanks @pposca @asvetlov ! I have already added this property to aiozipkin integration (aio-libs/aiozipkin#138)

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

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

asvetlov requested changes May 21, 2018

View reviewed changes

pposca force-pushed the issue_2968 branch from 1f449e1 to 35b5704 Compare May 21, 2018 13:19

asvetlov reviewed May 22, 2018

View reviewed changes

pposca force-pushed the issue_2968 branch from 35b5704 to e84d6ed Compare May 22, 2018 22:27

asvetlov requested changes May 23, 2018

View reviewed changes

pposca force-pushed the issue_2968 branch from e84d6ed to d43cd9b Compare May 23, 2018 09:14

asvetlov reviewed May 23, 2018

View reviewed changes

pposca force-pushed the issue_2968 branch from d43cd9b to 5ff1917 Compare May 24, 2018 09:38

asvetlov approved these changes May 27, 2018

View reviewed changes

asvetlov merged commit 0e11b36 into aio-libs:master May 27, 2018

pposca deleted the issue_2968 branch June 9, 2018 20:01

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add path_pattern property to routes and resources #2993

Add path_pattern property to routes and resources #2993

pposca commented May 11, 2018

asvetlov commented May 11, 2018

kxepal commented May 11, 2018

pposca commented May 11, 2018

asvetlov commented May 11, 2018

codecov-io commented May 18, 2018 •

edited

Loading

jettify commented May 20, 2018

asvetlov left a comment

pposca commented May 21, 2018

asvetlov commented May 21, 2018

asvetlov left a comment

asvetlov May 22, 2018

pposca May 22, 2018

asvetlov May 23, 2018

pposca May 23, 2018

asvetlov left a comment

asvetlov May 23, 2018

asvetlov May 23, 2018

asvetlov May 23, 2018

asvetlov May 23, 2018

asvetlov May 23, 2018

asvetlov commented May 27, 2018

pposca commented May 27, 2018

jettify commented Jun 9, 2018

lock bot commented Oct 28, 2019


		Read-only canonical path associate with the resource. For example
		``/path/to`` or ``/path/{to}``


		Read-only canonical path associate with the resource. Returns the prefix
		used to create the StaticResource. For example ``/prefix``

Add path_pattern property to routes and resources #2993

Add path_pattern property to routes and resources #2993

Conversation

pposca commented May 11, 2018

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

asvetlov commented May 11, 2018

kxepal commented May 11, 2018

pposca commented May 11, 2018

asvetlov commented May 11, 2018

codecov-io commented May 18, 2018 • edited Loading

Codecov Report

jettify commented May 20, 2018

asvetlov left a comment

Choose a reason for hiding this comment

pposca commented May 21, 2018

asvetlov commented May 21, 2018

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov commented May 27, 2018

pposca commented May 27, 2018

jettify commented Jun 9, 2018

lock bot commented Oct 28, 2019

codecov-io commented May 18, 2018 •

edited

Loading