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

Ability to customize router URLs for custom actions, using url_path. #2010

Merged
merged 9 commits into from
Dec 19, 2014

Conversation

tanwanirahul
Copy link
Contributor

Creating a custom router just to change the method name in the URI is too verbose, specially if you have many custom actions in a ViewSet and you only to customize single method.

This patch would allow developers to customize the default method name with something else by passing a custom_method_name as a part of list_route and detail_route decorators.

@tomchristie
Copy link
Member

I can't see changing the methods being a standard, typical use-case.

@tomchristie tomchristie closed this Nov 3, 2014
@tanwanirahul
Copy link
Contributor Author

Cool.. the standard use case for me here was to convert _ to - (underscores to hyphen). For ex: I had a method sync_id for which I wanted to have a URI like prefix/sync-id.

@maryokhin
Copy link
Contributor

agree with @tanwanirahul, a bit annoying have to install drf-extensions just to get normal URL names

@tomchristie
Copy link
Member

Someone probably needs to walk through a simple example on this?

@maryokhin
Copy link
Contributor

I am not sure what you mean.
What me and @tanwanirahul meant, was that it is probably a common use case to want URLs like /users/1/request-password-reset/ instead of /users/1/request_password_reset/.
http://chibisov.github.io/drf-extensions/docs/#controller-endpoint-name

@tomchristie
Copy link
Member

Okay, understood.
"method name" confused me.
Fair request, but I'm a bit reluctant to do much tweaking of the routers atm, but worth re-assessing when poss.

@tanwanirahul
Copy link
Contributor Author

@maryokhin Yes, agree. Its a pain to use extensions for a very trivial tasks like this one.
@tomchristie thanks for re-consideration. I believe, many people might have a need for the same.

@tomchristie
Copy link
Member

Not going to promise that it's high on the list right now given getting 3.0 out the door, but certainly seems worth looking at at least.

@tanwanirahul
Copy link
Contributor Author

Cool..makes complete sense.

@maryokhin
Copy link
Contributor

Hi @tomchristie,

Are there any thoughts on this as of today? The reason I'm asking is because of the suspended state of the whole dynamic route thing. There are reports of routes not working properly in drf-extensions chibisov/drf-extensions#54, and the whole reason @action and @list are there in the first place is for supporting the use-case in this issue.

So it's unclear whether the effort should be spent on making drf-extensions more robust or just deprecating @action and @link and support fully the core @detail_route and @list_route.

@tomchristie
Copy link
Member

just deprecating @action and @link and support fully the core @detail_route and @list_route.

Confused by the question. @action and @link no longer exist, they are already deprecated.

Are there any thoughts on this as of today?

So are there any other examples other than selecting between this-kind-of-style and this_kind_of_style?

suspended state of the whole dynamic route thing.

I don't know what this means? What dynamic routes?

@maryokhin
Copy link
Contributor

Confused by the question. @action and @link no longer exist, they are already deprecated.

I meant the @action and @link decorators in drf-extensions, which mirrored the ones that DRF no longer has. This makes it a bit confusing now, what to do with them in general, because it would depend if DRF has this feature in core or not.

So are there any other examples other than selecting between this-kind-of-style and this_kind_of_style?

Not really, which is why I proposed in #1892 to just add dasherize_routes to Router init kwargs.

I don't know what this means? What dynamic routes?

I meant the routes that are created when decorating a viewset method with @list_route and @detail_route. I don't know what the correct naming for them is. I guess extra link and action routes as documentation calls it would be more correct.

@tomchristie
Copy link
Member

Not really, which is why I proposed in #1892 to just add dasherize_routes to Router init kwargs.

In which case I'd prefer to see a underscore-routers package or similar that includes alternate SimpleRouter and DefaultRouter classes, that use underscored styles. Eg.

class UnderscoreSimpleRouter(SimpleRouter):
    routes = [
        # List route.
         Route(
             url=r'^{prefix}{trailing_slash}$',
             mapping={
                 'get': 'list',
                 'post': 'create'
             },
             name='{basename}_list',
             initkwargs={'suffix': 'List'}
         ),
         # Dynamically generated list routes.
         # Generated using @list_route decorator
         # on methods of the viewset.
         DynamicListRoute(
             url=r'^{prefix}/{methodname}{trailing_slash}$',
             name='{basename}_{methodname}',
             initkwargs={}
         ),
         # Detail route.
         Route(
             url=r'^{prefix}/{lookup}{trailing_slash}$',
             mapping={
                 'get': 'retrieve',
                 'put': 'update',
                 'patch': 'partial_update',
                 'delete': 'destroy'
             },
             name='{basename}_detail',
             initkwargs={'suffix': 'Instance'}
         ),
         # Dynamically generated detail routes.
         # Generated using @detail_route decorator on methods of the viewset.
         DynamicDetailRoute(
             url=r'^{prefix}/{lookup}/{methodname}{trailing_slash}$',
             name='{basename}_{methodname}',
             initkwargs={}
         ),
     ]

Notice {methodnamehyphen} has become {methodname} throughout, and - has become _ in all the name parameters.

It's not terribly elegant, but it only needs to be done once.

I'd be against any further complexity in the router classes as they're already the least nice bit of API in rest framework so I've no problem with them continuing to only provide for a fairly restricted set of functionality.

@maryokhin
Copy link
Contributor

in all the name parameters

It probably doesn't matter, but I was talking about urls, not base names. Being able to use:

UserViewSet(ModelViewSet):
...
    @detail_route()
    def request_password_reset(...):
        ....

And get a url /users/2/request-password-reset/ instead of /users/2/request_password_reset/.

Which is why I felt that it belongs with the trailing_slash kwarg and that people will still keep requesting for it because implementing a whole router or using a third-party package for such a minor feature just won't feel right.

@tomchristie
Copy link
Member

The bottom line with third party packages is that they're not significantly any more work to implement (given @jpadilla's excellent work on the cookiecutter package) but are simply a good way to distribute maintainance responsibilities.

I don't have any interest in expanding the API set of the built-in routers - I know from experience that it'll only mean extra ongoing support cost to the project, which then negatively impacts our ability to build other new functionality or keep on to of bug reports.

By all means let's do this, but let someone else take it on in a separately installable package.

@tanwanirahul
Copy link
Contributor Author

@tomchristie Ability to change the url pattern (to make it different from method_name) would be as useful as ability to change the table names in django models. Sure we can do this with:

  1. CustomRouter: Which IMO is too verbose for doing this trivial thing
  2. Third party packages: Its always difficult to find such packages and having too many of them to support a single framework is too much IMO.

I wont be surprised if this request come up again.

@tmehlinger
Copy link

Your foreshadowing is appropriate, @tanwanirahul. I'm bringing this up again. 😄

I'm new to DRF and I'm really surprised this use case isn't supported, and I'm a little more surprised that there's so much resistance to it. I have a specific case where the limitations are causing me some frustration. Example:

router = routers.DefaultRouter()
router.register('things', ThingViewSet)

class ThingViewSet(ViewSet):
    @detail_route(methods=['GET'])
    def get_thing_widgets(self, request, pk=None):
        """
        I want this to handle /things/{pk}/widgets
        Instead it handles /things/{pk}/get-thing-widgets
        """

    @list_route()
    def get_all_widgets(self, request):
        """
        I want this to handle /things/widgets
        Instead it handles /things/get-all-widgets
        """

As far as I can tell, what I want isn't actually possible with DRF without going to great lengths with extensions and/or customized routers, which I think is a little silly because it's a fairly trivial case. What I really want is the ability to name my methods in such a way that they convey their purpose when looking at the code and have the URL be completely independent. @tanwanirahul's patch takes care of this with very little effort expended.

@tomchristie tomchristie reopened this Dec 19, 2014
@tomchristie
Copy link
Member

Okay, reopened. PR needs docs.

@tanwanirahul
Copy link
Contributor Author

@tomchristie Hmmm, I would take that on me. Will add by EOD today.

@maryokhin
Copy link
Contributor

If we accept this format, then we should also think about the naming of the decorator kwarg. custom_method_name is not very descriptive for someone who has no idea about the internals of the router, maybe url/route or endpoint (like drf-extensions)?

@tomchristie
Copy link
Member

👍

@tomchristie tomchristie changed the title Ability to customize method names without creating a custom router Ability to customize router URLs for custom actions, using url_path. Dec 19, 2014
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

5 participants