Skip to content

Allow Django 2.x path usage in routers #6148

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

Closed
1 task done
sevdog opened this issue Aug 28, 2018 · 15 comments
Closed
1 task done

Allow Django 2.x path usage in routers #6148

sevdog opened this issue Aug 28, 2018 · 15 comments
Milestone

Comments

@sevdog
Copy link
Contributor

sevdog commented Aug 28, 2018

Checklist

  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.

Proposal

In Django 2.x the new path (and re_path) were introduced to simplify URL routing.

It could be nice to have the ability to use path to simplify route annotation (aka @action ).

This however may break compatibility with Django 1.x which should be tested.

@rpkilby
Copy link
Member

rpkilby commented Aug 28, 2018

Can you give an example of what the expected usage would look like?

@sevdog
Copy link
Contributor Author

sevdog commented Aug 29, 2018

As of now we may write an action with url_path using a raw regex.

# UUID
@action(url_path=r'(?P<myuuid>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})')
# slug
@action(url_path=r'(?P<myslug>[-a-zA-Z0-9_]+)')
# other regex (to match html simple tag)
@action(url_path=r'(?P<myvar></?[a-zA-Z0-9]+>')

With path syntax it should look like:

# UUID
@action(url_path='<uuid:myuuid>`)
# slug
@action(url_path='<slug:myslug>')

# other regex (to match html simple tag)
# NOTE: the path convertert `html-tag` should be registered via `register_converter`
@action(url_path='<html-tag:myvar>'

Other possible examples on path usage can be found here.

In the end, because django.urls.url may be deprecated in favor of django.urls.re_path a conditional import (try-except) should be used for this function to guarantee compatibility.

@timworx
Copy link

timworx commented Sep 17, 2018

To add to this, it there are other args that would be useful with routers (whether specified on the router or view).

One I currently have a need for is custom path converter.

If you were able to use the path route syntax to specify the url param used for detail lookups (e.g. <int:my_lookup_kwarg> then it would be possible to use custom path converters as well.

@rpkilby
Copy link
Member

rpkilby commented Sep 17, 2018

I haven't spent too much time looking into it, but it should be possible for users to implement this as a custom router. You would need to do two things:

  • Override the routes list to use a path compatible URL syntax instead of regexs.
  • Override get_urls to construct routes with path instead of url/path_re.

No changes to the @action decorator would be necessary.

@liavkoren
Copy link

Hi @rpkilby. I ran into this issue last night and it took more than an hour to figure out why my url wasn't being resolved correctly. Because all you get is the django debug page saying, 'I can't resolve this url, here are the patterns I tried', there's no obvious failure point to dig into, and no obvious place inside my code to insert a break point.

I realize it's More Work for the DRF maintainers, but at the very least it should be recognized that DRF ideally should be fully compatible with the default Django path formatting. Otherwise this can result in extremely confusing bugs.

@xordoquy
Copy link
Collaborator

it should be recognized that DRF ideally should be fully compatible with the default Django path formatting

This is addressed by the issue still being opened.
We are looking forward PR to improve the compatibility here.

@chgad
Copy link

chgad commented Sep 6, 2019

Today I was confronted with the same issue as @likeon. Is it possible to note it in the docs like somewhere here untill any changes towards path formatting are made?

@tomchristie tomchristie added this to the 3.11 Release milestone Nov 7, 2019
@hmpf
Copy link

hmpf commented Nov 11, 2020

I ran into a cousin of this: switching from manually made paths and APIViews to ViewSets lead to a lot of 405s until I realized the viwsets didn't use typed paths, so that endpoint/<pk> would find both integer and string <pk>s, making it necessary to change the order of urls and the order viewset are appended to routers.. (and testing for correct response codes everywhere.)

I've started work on a router using paths, where the type can be explicitly defined.

@sevdog
Copy link
Contributor Author

sevdog commented Nov 11, 2020

I ran into a cousin of this: switching from manually made paths and APIViews to ViewSets lead to a lot of 405s until I realized the viwsets didn't use typed paths, so that endpoint/<pk> would find both integer and string <pk>s, making it necessary to change the order of urls and the order viewset are appended to routers.. (and testing for correct response codes everywhere.)

I've started work on a router using paths, where the type can be explicitly defined.

@hmpf There is already a PR about this: #6789

@jourdanrodrigues
Copy link

Is this still a thing to be solved by #6789?

@tomchristie tomchristie modified the milestones: 3.13 Release, 3.14 Jan 10, 2022
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@auvipy
Copy link
Member

auvipy commented Nov 21, 2022

Is this still a thing to be solved by #6789?

yes

@auvipy auvipy removed the stale label Jan 3, 2023
@auvipy auvipy closed this as completed Jan 12, 2023
@tu-pm
Copy link

tu-pm commented Apr 9, 2024

Thanks @sevdog @auvipy for making this happened.

I was also working on another solution for this problem with DRF version 3.13.1 before looking at #6789 and found out the change in PR is pretty radical: instead of having different URL Path formats for different ViewSets, we are modifying this behavior per-router.

This will cause problems if we want to use lookup_value_regex in some ViewSets and lookup_value_converter in some other ViewSets within the same router.

Do we have a good reason for this approach? 'Cause I think we can do it more flexibly and with less changes by just overriding get_lookup_regex like this:

    def get_lookup_regex(self, viewset, lookup_prefix=''):
        lookup_value_regex = getattr(viewset, 'lookup_value_regex', None)
        lookup_value_converter = getattr(viewset, 'lookup_value_converter', None)
        assert not lookup_value_regex or not lookup_value_converter, (
            'lookup_value_regex and lookup_value_converter are mutually exclusive'
        )
        if lookup_value_converter:
            base_regex = '<{lookup_value}:{lookup_prefix}{lookup_url_kwarg}>'
            lookup_field = getattr(viewset, 'lookup_field', 'pk')
            lookup_url_kwarg = getattr(viewset, 'lookup_url_kwarg', None) or lookup_field
            return base_regex.format(
                lookup_prefix=lookup_prefix,
                lookup_url_kwarg=lookup_url_kwarg,
                lookup_value=lookup_value_converter
            )

        return super().get_lookup_regex(viewset, lookup_prefix)

@sevdog
Copy link
Contributor Author

sevdog commented Apr 9, 2024

@tu-pm you can see the discussion in the PR from here for this behaviour.

@tu-pm
Copy link

tu-pm commented Apr 9, 2024

@sevdog

I see, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests