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

Support "extra actions" for routing in the HTML forms for the browsable API #2062

Closed
cancan101 opened this issue Nov 10, 2014 · 40 comments · Fixed by #5605
Closed

Support "extra actions" for routing in the HTML forms for the browsable API #2062

cancan101 opened this issue Nov 10, 2014 · 40 comments · Fixed by #5605
Assignees

Comments

@cancan101
Copy link
Contributor

Add support for creating HTML forms for the browsable API for "extra actions" for routing.

For example using the example from the linked page:

    @detail_route(methods=['post'])
    def set_password(self, request, pk=None):

creating a form for calling set_password. Likely this would require additional arguments passed to the detail_route decorator (such as the serializer used, PasswordSerializer).

See: https://groups.google.com/forum/#!topic/django-rest-framework/e75osh1Wcyg

@tomchristie tomchristie added this to the 3.2 Release milestone Nov 10, 2014
@tomchristie
Copy link
Member

This will probably be included as part of the 'admin-style browsable API'. This is all a little way off at the moment, right now so not going to get into specifics just yet.

@cancan101
Copy link
Contributor Author

Right now it looks like it just falls back to rendering the form for the view's base serializer which is a little odd.

@maryokhin
Copy link
Contributor

Setting something like @detail_route(serializer_class=MySerializer) would also allow to use self.get_serializer() normally in actions, because normally I find myself doing:

object = self.get_object()
context = self.get_serializer_context()
serializer = MySerializer(object, context=context)

Instead of:

object = self.get_object()
serializer = self.get_serializer(object) # context set automatically

This would actually be a very nice improvement for both the usability of the actions and the browsable API, because when I have detail_route(['POST']) it looks like the browsable API has a bug and is asking me to submit values for the instance itself, which is very confusing.

@xordoquy
Copy link
Collaborator

I'm about to close this issue as I feel the title doesn't match the content.
"Add support for creating HTML forms for the browsable API" is already possible. As per regular verbs, if you have an action with GET and POST, you'll have the associated form.
This only missing point being a link from the "main" page.

@cancan101
Copy link
Contributor Author

How does DRF know what Serializer to use to generate the form for extra actions?

@xordoquy
Copy link
Collaborator

They are included in the Response instance:

class BrowsableAPIRenderer(BaseRenderer):
    ....
    def get_raw_data_form(self, data, view, method, request):
        ...
        serializer = getattr(data, 'serializer', None)

@cancan101
Copy link
Contributor Author

I'm not sure I follow. Let's take this case:

    @detail_route(methods=['post'])
    def set_password(self, request, pk=None):

I assume that set_password is not called when rendering the correct form for the browsable API

@xordoquy
Copy link
Collaborator

@cancan101 if you have a post only, you won't get any browsable API anyway. This is the same as if you define a viewset that has a create but doesn't define a list.
This snippet has a nice form we you get it and can create the task upon post:

    @list_route(methods=['get', 'post'])
    def mine(self, request):
        queryset = self.filter_queryset(self.get_queryset().filter(owner=request.user))

        page = self.paginate_queryset(queryset)
        if page is not None:
            serializer = self.get_serializer(page, many=True)
            return self.get_paginated_response(serializer.data)

        serializer = self.get_serializer(queryset, many=True)
        return Response(serializer.data)

@cancan101
Copy link
Contributor Author

Ignoring the case of extra actions, I don't see why a form can't be generated even if there is no list. All of the information needed to do so should be available on the View.

Further I think it would be nice to support asymmetric Serializers (#2898).

@xordoquy
Copy link
Collaborator

@cancan101 if you don't have a get, you don't have a form representation

@cancan101
Copy link
Contributor Author

I understand, but that seems like a silly restriction.

@xordoquy
Copy link
Collaborator

@cancan101 What restriction are you referring to ?

@cancan101
Copy link
Contributor Author

That you need to have a GET endpoint in order to get a form for a POST endpoint.

@xordoquy
Copy link
Collaborator

@cancan101 how would a user fill a form without an initial get ?

@cancan101
Copy link
Contributor Author

What is needed from the initial GET in order for a user to fill a form when POSTing?

@xordoquy
Copy link
Collaborator

The user needs a form before he could post the data. This is a browsable API. As with every website, users get the page, fill the form and then post it.
I don't see why you would want to display a form if not for showing it within a get.

@xordoquy
Copy link
Collaborator

I slightly amended my workshop project to showcase this:
check https://github.com/xordoquy/workshop_drf and have a look at /task/mine/ for example.

@cancan101
Copy link
Contributor Author

Let's say I have an endpoint /company/<companyId>/users/ that I only want to allow POSTing to (perhaps because I already have a /users/<userId> for GETting).

Certainly in order to render the browseable API, the browser will GET that endpoint. I would still like some way to show a nice form with the fields in the POST without having to enable GETing.

@cancan101
Copy link
Contributor Author

DRF Swagger does handle this by introspecting the various actions on an end point and resolving the associated Serializer in each case. It then generates a UI based on the fields of that Serializer.

@xordoquy
Copy link
Collaborator

@cancan101 Your use case looks suspicious to me. You don't want to create through /company/<companyId>/users/if you already have /users/<userId> and in the case of different serializers you don't want to POST data from a serializer to another one.
It looks to me that the real request was to provide swagger a decorator that provides the serializer. Note swagger could provide its own there if you need more informations.
If several libraries do have the use of such feature, I will reconsider my opinion.

@cancan101
Copy link
Contributor Author

Perhaps the example I provided of users and companies was not the best case of POST without GET. For a better example, take a look at https://developer.github.com/v3/repos/merging/#perform-a-merge.

@smcoll
Copy link

smcoll commented May 13, 2015

@cancan101 my workaround for the time being (in DRF 2.4) for declaring the serializer class for a custom route such that it renders a relevant form in the browseable API looks like this:

@detail_route(methods=['post'])
def myaction(self, request, pk=None):
    # ...

def get_serializer_class(self):
    if self.request.path.endswith('/myaction'):
        return MyCustomSerializer
    return super(MyViewset, self).get_serializer_class()

@cancan101
Copy link
Contributor Author

Interesting. The solution in DRF3 (and maybe 2) is even simpler:

    def get_serializer_class(self):
        if self.action == 'myaction':
            return MyCustomSerializer
        return super(MyViewset, self).get_serializer_class()

no more hard coding paths.

@smcoll
Copy link

smcoll commented May 13, 2015

@cancan101 looking forward to that!

@cancan101
Copy link
Contributor Author

@smcoll Although I see this if GET is not one of the methods:

{
    "detail": "Method \"GET\" not allowed."
}

@smcoll
Copy link

smcoll commented May 13, 2015

@cancan101 with a json request, right? With the browseable API (and an HTTP request), i'm getting the customary form and documentation that DRF provides.

@cancan101
Copy link
Contributor Author

No I see that on the browseable API (using DRF3).

If I add GET as a method then the form renders correctly.

On Wed, May 13, 2015 at 2:34 PM smcoll notifications@github.com wrote:

@cancan101 https://github.com/cancan101 with a json request, right?
With the browseable API (and an HTTP request), i'm getting the customary
form and documentation that DRF provides.


Reply to this email directly or view it on GitHub
#2062 (comment)
.

@smcoll
Copy link

smcoll commented May 13, 2015

@cancan101 ok, then that must be a difference between the implementations of 2.4 and 3.0

@cancan101
Copy link
Contributor Author

I don't really understand why GET needs be support on the extra action for the POST form to work.

When I have @detail_route(methods=['post', 'get']) and If I call print self.action, self.request.method from within get_serializer_class , I see:

myaction GET
myaction POST
myaction POST

@cancan101
Copy link
Contributor Author

So I tracked down the issue in DRF3. It actually should work where the get_serializer is called with a request's method overridden to POST (so @xordoquy is actually not totally correct).

The issue is that even though the method is overwritten, the view's action is set to None. ie the logic used to map a request's path -> action DOES require for the GET endpoint to exist even though the browsable API does not.

@cancan101
Copy link
Contributor Author

There is actually an incredibly simple fix for this!

This line in override_method only sets the action if it is not currently None if we just remove that check, this problem is solved!

@tomchristie Any reason that override_method won't set the action on a the view if it is currently None?

I put up PR #2933.

@cancan101
Copy link
Contributor Author

@xordoquy I think if #2933 gets merged this issue is totally solved by using something like:

    def get_serializer_class(self):
        if self.action == 'myaction':
            return MyCustomSerializer
        return super(MyViewset, self).get_serializer_class()

or for the more hardcore:

    def get_serializer_class(self):
        if self.action == MyViewset.myaction.__name__:
            return MyCustomSerializer
        return super(MyViewset, self).get_serializer_class()

@tomchristie
Copy link
Member

We may get onto this at somepoint, but I don't see it as critical to 3.3.0. Dropping the milestone.

@rpkilby
Copy link
Member

rpkilby commented Aug 18, 2016

Not sure if I'm missing something, but overriding the serializer for an action does affect the browsable API correctly.

    @detail_route(methods=['post'],
                  url_path='change-password',
                  serializer_class=PasswordChangeSerializer)
    def change_password(self, request, pk):
        serializer = self.get_serializer(data=request.data)
        serializer.is_valid(raise_exception=True)
        ...

if you have a post only, you won't get any browsable API anyway. This is the same as if you define a viewset that has a create but doesn't define a list.

@xordoquy - not sure if this changed, but it works fine for us. The browsable api returns a 405, but the serializer form is rendered below the embedded response.

change-password

@tomchristie
Copy link
Member

Suggest we link to available actions rather than displaying them on the page itself, as described in #2934.

@merwok
Copy link
Contributor

merwok commented Feb 16, 2017

A simple workaround is to include links to the custom endpoints in your object representation. I have a model with a revision log, and its serialization has a link to itself, its parent resource and its revisions (a detail route that returns a list of records). In this case, hypermedia serves the client dev and serves me :)

It’s not perfect though: If I configure other methods than just GET for the custom detail view, the browsable API doesn’t show anything different than the regular object detail view.

@gabn88
Copy link
Contributor

gabn88 commented Jun 14, 2017

Just thinking out loud for a possible solution.

Ideally when you make a detail route like shown below (to set activities to done), with baseview = /activities/, detail = /activities/1/

  @detail_route(methods=['patch'])
    def set_done(self, request, pk=None):
        '''
        More info
        '''
        obj = self.get_object()  # Important, also does permission checking!!!
        done_by_pk = request.user.pk
        done_on = timezone.now()

        serializer = self.get_serializer(obj, data={'done_by':done_by_pk, 'done_on':done_on}, partial=True, context={'request': request})
        serializer.is_valid(raise_exception=True)
        self.perform_update(serializer)
        return Response(serializer.data)

When you go to the endpoint in the browsable api (/activities/), you would get an additional html

saying:

This view has a detail route: /set_done/.

More info (from the comment in the detail route)

. Or something like this.

When you go to the detail endpoint /activities/1/, you would see the detailed view (with the url etc), plus the additional url: {"set_done_url": "http.../activities/1/set_done/}. When then GETting to http.../activities/1/set_done/ you would see:

More info (from the comment in the detail route)

. And because the view already shows that only PATCH, OPTIONS is allowed the user of the API would have all necessary information.

What do you think?

@rpkilby
Copy link
Member

rpkilby commented Nov 18, 2017

Hi all. For anyone still tracking this issue, I've started #5605. It's an initial implementation, and your feedback would be greatly appreciated.

@gabn88
Copy link
Contributor

gabn88 commented Nov 18, 2017 via email

@rpkilby
Copy link
Member

rpkilby commented May 19, 2018

Hi all. Aside from documentation, #5605 is now ready for review. If you have the time, extra eyes would be appreciated.

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.

8 participants