Allow multiple @detail_route and/or @list_route for the same URL

[andriy-s]

Currently, if I need an ad hoc route to handle several HTTP methods I'm expected to define a single method in a view set, and then explicitly check for the value of the request.method to decide what code path to choose.

Example. Lets assume I have a UserViewSet similar to the one from the DRF's documentation. I'd like a single sub-URL to be used for both setting and resetting (deleting) the user's password. Currently, I can do this like follows:

class UserViewSet(ModelViewSet):
    ...

    @detail_route(methods=['post', 'delete'])
    def password(self, request, pk=None):
        if request.method == 'POST':
            # set password
            ...
        else:
            # reset (delete) password
            ...

This Pull Request allows to define several decorated viewset methods with the same url_path but different methods, and lets DRF route requests to these methods automatically based on both URL the HTTP method, just like with predefined list,get,update etc. viewset methods. In the client side code this will look like follows:

class UserViewSet(ModelViewSet):
    ...

    @detail_route(methods=['post'], url_path='password')
    def set_password(self, request, pk=None):
        # set password
        ...

    @detail_route(methods=['delete'], url_path='password')
    def reset_password(self, request, pk=None):
        # reset (delete) password
        ...

[jpadilla]

I'm not sure how much value in general this would bring into core at the moment. In this case I think this would do better as custom router with this functionality released as a third party package.

[tomchristie]

Our routing here currently follows Django's style.
While the PR makes sense it's not something I'm inclined to accept - seconding @jpadilla that this sort of thing should remain as a third party package if needed.

[andriy-s]

Tom, José,

thank you for your replies.

Sorry, but I do not understand what Django's style Tom is referring to here. Please correct me if I'm wrong, but Django itself does not have automatic routing. Or does it? And by adding the possibility to have ViewSet method per HTTP method per ad-hoc URL path, this PR was intended to make handling of ad-hoc routes in ViewSet more consistent with standard ViewSet methods (and consistency is a good thing, isn't it?) Which, in turn, allows to make client code using DRF a bit cleaner (more code paths separation, less indent required), which is also a good thing.

Anyways, I estimated the possibility to make this an independent out-of-core router. And while making it in-core required a fairly small amount of code changes, making it outside of the core will required a significant amount of code duplication, which I'd like to avoid, if at all possible. To make implementing this kind of change in an out-of-core package easier, I'd like to suggest the following change to the DRF core.

There's a _get_dynamic_routes() function defined inside the SimpleRouter.get_routes() method. I'd like to turn this _get_dynamic_routes() function into a protected method of SimpleRouter, so I can easily override it in a class derived from either SimpleRouter or DefaultRouter.

Please let me know if this kind of change is acceptable. I'll submit another PR if it is.

Thank you in advance,
Andriy.

[cancan101]

I am 👍 on this PR. There are additional benefits of splitting out the method such as having different permission classes on each method (see #2708).

[cancan101]

Any way we can re open this PR for discussion?

I think that having separate python methods would also help tools like django-rest-swagger in generating docs.

[cancan101]

@tomchristie Any possibility of reopening this for discussion?

[kevin-brown]

Just to put it out there, I am the third 👎 on this PR because I don't see this as being something that needs to be in the core, and I see it introducing more confusion on top of the already confusing @detail_route and @list_route decorators.

Please correct me if I'm wrong, but Django itself does not have automatic routing. Or does it?

Django enforces the idea of having a single view for a url route. This is something that I agree with Django REST framework maintaining, as another who is used to Django views can safely assume that a route will have a single view.

There's a _get_dynamic_routes() function defined inside the SimpleRouter.get_routes() method. I'd like to turn this _get_dynamic_routes() function into a protected method of SimpleRouter, so I can easily override it in a class derived from either SimpleRouter or DefaultRouter.

Please let me know if this kind of change is acceptable. I'll submit another PR if it is.

This is something that I wouldn't necessarily be opposed to, but I wouldn't mind hearing some input from @tomchristie or @jpadilla about it.

The alternative implementation that I was going to suggest for setting up detail routes with different method calls was

class UserViewSet(ModelViewSet):
    ...

    @detail_route(methods=['post', 'delete'])
    def password(self, request, pk=None):
        if request.method == 'POST':
            return self.ret_password(request, pk=pk)
        else:
            return self.reset_password(request, pk=pk)

    def set_password(self, request, pk=None):
        # set password
        ...

    def reset_password(self, request, pk=None):
        # reset (delete) password
        ...

At least this will make it more obvious what the password route is trying to do, especially for someone who is inheriting your code base and is trying to learn how Django REST framework relates to Django.

[cancan101]

While changing _get_dynamic_routes to be protected to allow more easy subclassing in third party package, that would be a decent step forward, I do think there is value in bringing this functionality into core.

The nice thing about breaking up the separate actions as separate methods is that there is now space to attach separate annotations to the methods. This is important to accomplish #2062 and for tools like django-rest-swagger to determine what serializers are in use by the method.

[xordoquy]

I'm also -0 for this change.
My feeling is this change makes code less readable.
I'd either do as @kevin-brown suggested or write a class decorator that would create the per action routing code.

[educolo]

At this momento without this functionality I have to choose between a working function or a documented function with django rest swagger 2. The new schema doesn't render more than one verb per function in detail and list routes.