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

Mark extra actions which corresponds to the same URL #4920

Closed
theosotr opened this issue Feb 22, 2017 · 11 comments · Fixed by #5605
Closed

Mark extra actions which corresponds to the same URL #4920

theosotr opened this issue Feb 22, 2017 · 11 comments · Fixed by #5605
Assignees

Comments

@theosotr
Copy link
Contributor

Imagine that I have two extra actions that corresponds to the same url (e.g. like create() with list() which corresponds to the name url i.e. ^{prefix}/{name}/ but with different HTTP methods).

from rest_framework import viewsets
from rest_framework.decorators import detail_route

class MyViewSet(viewsets.ViewSet):

    @detail_route(methods=['get'], url_path='foo', url_name='foo_get')
    def foo_get(self, request, pk=None):
        pass

    @detail_route(methods=['head'], url_path='foo', url_name='foo_head')
    def foo_head(self, request, pk=None):
        pass

I want to use the DefaultRouter class to construct url patterns rather than creating urls on my own like follows:

extra_actions = MyView.as_view({'head': 'foo_head', 'get': 'foo_get'})

The problem here is that the _get_dynamic_routes() method of SimpleRouter class create a namedtuple (Route) for every extra action, and therefore only the first extra action is taken into account. So, the fist call succeeds whereas the second call returns HTTP 405 METHOD_NOT_ALLOWED.

# This call woks.
curl -X get http:localhost:8000/api/products/1/foo/

# This returns 405.
curl -X head http:localhost:8000/api/products/1/foo/ 

All in all, I would expect that one Route should be created by the _get_dynamic_routes() with the following mapping (Remeber that I want these extra actions to correspond to the same url!):

mapping = {
    'get': 'foo_get',
    'head': 'foo_head',
}
@xordoquy
Copy link
Collaborator

That's an interesting idea.

@rpkilby
Copy link
Member

rpkilby commented Feb 22, 2017

Is there a reason to have the two separate url patterns? You could also simulate this with the following:

class MyViewSet(viewsets.ViewSet):

    @detail_route(methods=['get', 'head'], url_path='foo', url_name='foo')
    def foo(self, request, pk=None):
        if request.method == 'get':
            return self.foo_get(request, pk)
        if request.method == 'head':
            return self.foo_head(request, pk)

    def foo_get(self, request, pk):
        pass

    def foo_head(self, request, pk):
        pass

foo() essentially becomes a mini dispatch() method.

@tomchristie
Copy link
Member

As per @rpkilby.

@theosotr
Copy link
Contributor Author

@tomchristie @rpkilby It is not what I expect, because in the above code there is one action named foo and it supports two HTTP methods (either you make a head or a get call, the action is the same).

# The value of self.action is `foo`.
action = self.action

What I desire is to have two actions corresponding to the same url, and the action is changed based on the HTTP method (like get -> list and post -> create). It is something that is not supported by the SimpleRouter class.

@tomchristie
Copy link
Member

Actually that's kinda fair, tho it's not obv. if we'd do better to just accept this as one of the limitations of viewsets or cater for this case better. It'd get slightly complicated in that you'd also still be able to have multiple methods for an particular action. Would certainly be will to review a pull request for this on it's own merits.

@xordoquy
Copy link
Collaborator

It'd get slightly complicated in that you'd also still be able to have multiple methods for an particular action.

One would argue that's already the case with list / create and retrieve / update / delete.

@tomchristie tomchristie reopened this Feb 23, 2017
@tomchristie
Copy link
Member

Reopening for further consideration.

@kevin-brown
Copy link
Member

This is the same as #2820, #3743, and #4264.

I will note that either way the url_name would have to be the same since you can't define the same URL pattern in Django and give it multiple names. You also can't have it point to multiple methods, so we'd have to generate some extra method on the fly to handle the routing.

@theosotr
Copy link
Contributor Author

In my opinion, #2820 is on the right direction.

For the reasons I explained above, I believe that this feature is not trivial, and indexing by url patterns makes code consistent.

@dsaradini
Copy link

Just a quick note; The mini dispatch() solution does not handle schema documentation.

If we want different documentation from the retrieve/get and update/put actions, the @detail_route needs 2 different methods.

Currently it does not handle schema documentation as described here: http://www.django-rest-framework.org/api-guide/schemas/#schemas-as-documentation

@rpkilby rpkilby self-assigned this Dec 5, 2017
@rpkilby
Copy link
Member

rpkilby commented May 16, 2018

Hi all. I've implemented this in #5605. Current usage looks like:

class MyViewSet(ViewSet):

    @action(detail=False)
    def example(self, request, **kwargs):
        """Base description."""

    @example.mapping.post
    def create_example(self, request, **kwargs):
        """Description for creation."""

Per @kevin-brown's comment:

I will note that either way the url_name would have to be the same since you can't define the same URL pattern in Django and give it multiple names.

Method mapping intentionally does not accept any arguments, so you would be unable to run into this.

Lastly, method mapping works as expected with schemas/documentation.

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

Successfully merging a pull request may close this issue.

6 participants