Add SimplePathRouter #6789

sevdog · 2019-07-06T15:33:56Z

Description

Add SimplePathRouter to handle Django 2.x path converters (refs #6148).

tomchristie · 2019-07-08T12:27:02Z

Milestoning for review. Thanks!

sevdog · 2019-07-08T16:20:03Z

It may be improved a bit if support for Django 1.x gets dropped (ie: some bad if will be removed).

rpkilby · 2019-07-08T17:55:20Z

I'm thinking this should be pushed to the 3.11 release, as that is when we'll likely drop Django 1.11

tomchristie · 2019-07-09T10:37:28Z

@rpkilby Good call

tests/test_routers.py

rest_framework/routers.py

sevdog · 2019-07-13T13:47:05Z

@rpkilby I have updated with CRUD tests and removed Django 1.11 compact. Tell me if everything is fine for you.

auvipy

can you skip tests for Django 1.11 for this PR? dj1.11 seems to be supported for some more time.

sevdog · 2019-11-21T20:51:40Z

can you skip tests for Django 1.11 for this PR? dj1.11 seems to be supported for some more time.

I have also added an assertion in SimplePathRouter.get_urls to raise an assertion error if path is not defined.

sevdog · 2019-11-22T11:20:38Z

Ok... seems that pytest.mark.skipif has not worked as expected. I will recheck it as soon as I can.

auvipy

can you rebase plz?

sevdog · 2019-12-05T20:26:58Z

Rebased.

tomchristie · 2019-12-12T13:40:41Z

We've not dropped 1.11 on this pass, since its LTS.

Wondering if we might just outright switch the behavior on the next release - I'd rather we didn't have two different have two different class implementations here.

sevdog · 2019-12-12T13:55:13Z

@tomchristie you are right that a single-class implementation would be better.

I think that it can be done since the difference between those two are just two methods (get_urls and get_get_lookup_*). Those methods could be guided with some initialization param in class constructor.

I will try to refactor the code to have a single class.

tomchristie · 2019-12-12T14:42:18Z

When we drop 1.11 I'd actually probably be okay with us just completely switching to .path, so long as there's some kind of way for users to stick with the older behavior.

There's not really any great options for us here.

sevdog · 2022-10-18T08:36:12Z

@auvipy I have rebased the branch with current master. Let me know if any other change is needed.

auvipy · 2022-10-18T09:32:52Z

the workflow need to be triggered

camuthig · 2022-10-27T17:31:29Z

I am playing around with this PR a bit, and in my case the major reason I am trying to move to path over regular expressions is to get custom converter behaviors for the URL arguments. I have a function that transforms my model IDs as they come into and out of the API layer, and I have implemented this as a custom converter to do something like <encoded:pk> instead of <int:pk> to automatically decode the IDs before they reach any of my code.

I am able to make this work with the code in this PR by setting lookup_value_regex = "encoded" on my views. However, since I am not defining a regex here, the usage feels a little unnatural. The same behavior would apply to standard converters, like uuid.

This isn't a blocker, since it seems to be working just fine, but figured it was worth bringing up while it is still in PR. If there is an alternative way to approach these custom converters, let me know, and I will give that a try too.

sevdog · 2022-10-28T07:10:47Z

@camuthig you pointed out an interesting use case. I could change the code to use an "alternative" attribute with a more readable name to solve it, but this feels quite a mock-fix IMO.

Preserve backwards compatibility and handle this use case in an elegant way is a bit hard.

I would like to go back to my first implementation where there were 2 base router classes: one for regex and one for path, so there is no chance of mixing code and name would be quite expressive (but the project owner clearly expressed he prefers to have a single class).

The other way I see is to rework the current implementation in order to handle the choice between regex or path on a per view basis and not globally on the router, the problem here is to handle Route and DynamicRoute url attribute (which is currently a way orrible hack also in this implementation, since there is no guarantee that route url templates do not contains any other regex-language characters other than an initial ^ and a final $).

@tomchristie what do you think would fit better?

auvipy · 2022-11-21T13:33:09Z

Let us focus on the actual issue first. we are going to make the SimplRouter allowing new path based routing. should we also do the same for the DefaultRouter?

sevdog · 2022-11-21T14:32:06Z

@auvipy, do you mean this line?

django-rest-framework/rest_framework/routers.py

Line 342 in 751808c

root_url = re_path(r'^$', view, name=self.root_view_name)

Apart from that DefaultRouter should inherit from SimpleRouter

auvipy · 2022-11-21T14:40:18Z

yeah, OK. I got commit access recently. so want to prioritize this one and draw a conclusion very soon to get this merged. of course considering all the natural edge cases are ironed out

docs/api-guide/routers.md

auvipy · 2022-11-21T15:49:56Z

tests/test_routers.py

 notes_router = SimpleRouter()
 notes_router.register(r'notes', NoteViewSet)

+notes_path_router = SimpleRouter(use_regex_path=False)


can we add another test case which use DeafultRouter please?

It should not be a problem to replicate that test also with the other router.

rest_framework/routers.py

docs/api-guide/routers.md

Co-authored-by: Éric <merwok@netwok.org>

sevdog · 2022-11-23T09:08:59Z

I have added a small change to address the semantic issue pointed by @camuthig.

auvipy · 2022-11-23T12:36:36Z

that's great. do you see any other documentation changes needed for this?

auvipy · 2022-11-23T12:54:11Z

docs/api-guide/routers.md


    class MyModelViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet):
        lookup_field = 'my_model_id'
        lookup_value_regex = '[0-9a-f]{32}'

+    class MyPathModelViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet):
+        lookup_field = 'my_model_uuid'
+        lookup_value_converter = 'uuid'


can we use better name for lookup_value_converter?any better suited name instead of converter?

I used "converter" since django calls these elements path converters, maybe we can use the full expanded name to have lookup_value_pathconverter (or lookup_value_path_converter).

Any suggestion is welcome.

I would inclined to @tomchristie for a better insight here as I'm bit confused about naming

what about just lookup_value_path? this alighned with the name lookup_value_regex

I think that it would be better to use the same naming of django.

In this case it was named "converter" thus an attribute which should define one should be named after that. In this way it is easier to find references to what this element does.

OK I am going to merge this, we can improve it if needed

See encode/django-rest-framework#6789

sevdog force-pushed the path-router branch from 67caa27 to a29275f Compare July 6, 2019 15:48

tomchristie added this to the 3.10 Release milestone Jul 8, 2019

tomchristie modified the milestones: 3.10 Release, 3.11 Release Jul 9, 2019

rpkilby requested changes Jul 9, 2019

View reviewed changes

tests/test_routers.py Show resolved Hide resolved

tests/test_routers.py Outdated Show resolved Hide resolved

rest_framework/routers.py Outdated Show resolved Hide resolved

sevdog force-pushed the path-router branch 2 times, most recently from 7a17941 to c4c09ef Compare November 5, 2019 19:05

This was referenced Nov 7, 2019

Drop Django 1.11 support #7045

Closed

proposal: DJA DefaultRouter django-json-api/django-rest-framework-json-api#467

Closed

auvipy requested changes Nov 21, 2019

View reviewed changes

sevdog force-pushed the path-router branch from c4c09ef to a4e68d6 Compare November 21, 2019 20:49

sevdog force-pushed the path-router branch from a4e68d6 to 386dbbb Compare November 22, 2019 19:30

auvipy requested changes Dec 5, 2019

View reviewed changes

sevdog force-pushed the path-router branch 2 times, most recently from 17af0e1 to c1348cf Compare December 5, 2019 20:05

tomchristie modified the milestones: 3.11 Release, 3.12 Release Dec 12, 2019

sevdog force-pushed the path-router branch 2 times, most recently from b1d231f to 4696b19 Compare December 12, 2019 19:42

stale bot removed the stale label Oct 16, 2022

Allow usage of Django 2.x path in SimpleRouter

765d728

sevdog force-pushed the path-router branch from a8cbfca to 765d728 Compare October 17, 2022 13:34

auvipy added the Enhancement label Nov 21, 2022

Use path in Default router

1486e73

merwok reviewed Nov 21, 2022

View reviewed changes

docs/api-guide/routers.md Outdated Show resolved Hide resolved

auvipy requested changes Nov 21, 2022

View reviewed changes

merwok reviewed Nov 21, 2022

View reviewed changes

docs/api-guide/routers.md Outdated Show resolved Hide resolved

auvipy and others added 4 commits November 21, 2022 21:55

Update docs/api-guide/routers.md

a0ebb51

Co-authored-by: Éric <merwok@netwok.org>

Update docs/api-guide/routers.md

06a9244

Co-authored-by: Éric <merwok@netwok.org>

Add tests also for default router with path

ff69c69

Use a more relevant attribute for lookup when using path converters

4f232c7

auvipy reviewed Nov 23, 2022

View reviewed changes

auvipy approved these changes Jan 12, 2023

View reviewed changes

auvipy merged commit 2d19f23 into encode:master Jan 12, 2023

intgr mentioned this pull request Feb 7, 2023

[3.15] Add SimpleRouter use_regex_path argument typeddjango/djangorestframework-stubs#344

Merged

sevdog mentioned this pull request Aug 10, 2023

Use str as default path converter #9066

Merged

browniebroke mentioned this pull request Sep 10, 2024

Move path converter docs into a separate section #9524

Merged

krysal pushed a commit to WordPress/openverse that referenced this pull request Dec 12, 2024

Remove MediaViewSet's regex since upstream issue if fixed

c805e21

See encode/django-rest-framework#6789

krysal mentioned this pull request Dec 12, 2024

Remove MediaViewSet's lookup regex WordPress/openverse#5273

Merged

8 tasks

krysal pushed a commit to WordPress/openverse that referenced this pull request Dec 13, 2024

Remove MediaViewSet's regex since upstream issue if fixed

154d0b8

See encode/django-rest-framework#6789

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SimplePathRouter #6789

Add SimplePathRouter #6789

sevdog commented Jul 6, 2019 •

edited

Loading

tomchristie commented Jul 8, 2019

sevdog commented Jul 8, 2019

rpkilby commented Jul 8, 2019

tomchristie commented Jul 9, 2019

sevdog commented Jul 13, 2019

auvipy left a comment

sevdog commented Nov 21, 2019 •

edited

Loading

sevdog commented Nov 22, 2019

auvipy left a comment

sevdog commented Dec 5, 2019

tomchristie commented Dec 12, 2019

sevdog commented Dec 12, 2019

tomchristie commented Dec 12, 2019

sevdog commented Oct 18, 2022

auvipy commented Oct 18, 2022

camuthig commented Oct 27, 2022

sevdog commented Oct 28, 2022

auvipy commented Nov 21, 2022

sevdog commented Nov 21, 2022

auvipy commented Nov 21, 2022

auvipy Nov 21, 2022

sevdog Nov 21, 2022

sevdog commented Nov 23, 2022

auvipy commented Nov 23, 2022

auvipy Nov 23, 2022

sevdog Nov 23, 2022

auvipy Nov 23, 2022

auvipy Dec 2, 2022

sevdog Dec 2, 2022

auvipy Jan 12, 2023

Add SimplePathRouter #6789

Add SimplePathRouter #6789

Conversation

sevdog commented Jul 6, 2019 • edited Loading

Description

tomchristie commented Jul 8, 2019

sevdog commented Jul 8, 2019

rpkilby commented Jul 8, 2019

tomchristie commented Jul 9, 2019

sevdog commented Jul 13, 2019

auvipy left a comment

Choose a reason for hiding this comment

sevdog commented Nov 21, 2019 • edited Loading

sevdog commented Nov 22, 2019

auvipy left a comment

Choose a reason for hiding this comment

sevdog commented Dec 5, 2019

tomchristie commented Dec 12, 2019

sevdog commented Dec 12, 2019

tomchristie commented Dec 12, 2019

sevdog commented Oct 18, 2022

auvipy commented Oct 18, 2022

camuthig commented Oct 27, 2022

sevdog commented Oct 28, 2022

auvipy commented Nov 21, 2022

sevdog commented Nov 21, 2022

auvipy commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sevdog commented Nov 23, 2022

auvipy commented Nov 23, 2022

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

sevdog commented Jul 6, 2019 •

edited

Loading

sevdog commented Nov 21, 2019 •

edited

Loading