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

Use str as default path converter #9066

Merged
merged 1 commit into from Aug 16, 2023
Merged

Conversation

sevdog
Copy link
Contributor

@sevdog sevdog commented Aug 10, 2023

Description

Improves #6789 by using str as default path converter instead of path.

This because re-reading the docs the more appropriate to use is str.

This because its regex is [^/]+ which, among converters, is the most similar to the default used by router ([^/.]+).

@@ -144,7 +144,7 @@ def __init__(self, trailing_slash=True, use_regex_path=True):
self._url_conf = re_path
else:
self._base_pattern = '<{lookup_value}:{lookup_prefix}{lookup_url_kwarg}>'
self._default_value_pattern = 'path'
self._default_value_pattern = 'str'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any tests to verify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here they are: I have added the corner case in which there may be some patterns which may be ambigouus.

Thus using the <path:var> could cause problems, since it is too greedy, and having the latest registered pattern to never match if there is any sub-match.

@auvipy
Copy link
Member

auvipy commented Aug 15, 2023

str - Matches any non-empty string, excluding the path separator, '/'. This is the default if a converter isn’t included in the expression.
vs
path - Matches any non-empty string, including the path separator, '/'. This allows you to match against a complete URL path rather than a segment of a URL path as with str.

can you weigh in a bit more in the context?

@auvipy auvipy added this to the 3.15 milestone Aug 15, 2023
@sevdog
Copy link
Contributor Author

sevdog commented Aug 16, 2023

Consider the test viewset UrlPathViewSet.

The url-patterns produced by the router for this are going to be:

# using <path:pk>
^list/(?P<kwarg>[0-9]+)/\\Z
^(?P<pk>.+)/detail/(?P<kwarg>[0-9]+)/\\Z
^(?P<pk>.+)/detail/(?P<kwarg>[0-9]+)/detail/(?P<param>[0-9]+)/\\Z

# using <str:pk>
^list/(?P<kwarg>[0-9]+)/\\Z
^(?P<pk>[^/]+)/detail/(?P<kwarg>[0-9]+)/\\Z
^(?P<pk>[^/]+)/detail/(?P<kwarg>[0-9]+)/detail/(?P<param>[0-9]+)/\\Z

The second pattern with path is ambiguous (since it is also going to match also every URL which also matches the third) while with str it is not.

Since the router follows this sequence to build patterns:

  1. default list view
  2. dynamic list view
  3. default detail view
  4. dynamic detail view

The problem arises when there are both default detail and any action detail. Since the first pattern (prefix/<path:pk>/ -> ^prefix/(?P<pk>.+)/\\Z) is also going to match against the any other detail actions defined (prefix/<path:pk>/action/ -> ^prefix/(?P<pk>.+)/action/\\Z) because the patterns are matched in the order in which they appears in the url configuration and because those patterns overlap.

An other way to avoid such confusion would be to change the order of routes in SimpleRouter.routes by moving the more generics to the bottom of the list (thus the latest to be evaluated/tried). Registering first dynamic actions and then default one should avoid any confusion (referencing the list before the order should be 4, 2, 3, 1).

As final note the path converter may be too greedy and may cause useless queries on the database by trying to access non-exinstent objects, since that regex takes literally everithing that it encounters in the URI.

We may also register a custom path converter for rest-framework to match the pattern [^/.]+

@auvipy auvipy merged commit 5c07060 into encode:master Aug 16, 2023
6 of 7 checks passed
@sevdog sevdog deleted the path-converter branch August 18, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants