Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

Group list_route with viewset base path #515

Closed
vladimirnani opened this issue Aug 12, 2016 · 15 comments
Closed

Group list_route with viewset base path #515

vladimirnani opened this issue Aug 12, 2016 · 15 comments

Comments

@vladimirnani
Copy link

I have PhotosViewSet with list and list_route upload action.
I would like it to see under same 'photos' group in the docs.
image

What i am doing wrong? Is it supported?

for method in self.get_allowed_methods(callback):
    key = self.get_key(path, method, callback)

in SchemaGenerator get_key seems to not take 'list route' actions into account.

@angvp
Copy link

angvp commented Aug 15, 2016

@marcgibbons I have done this: (with the invaluable help of @cyncyncyn), doing a class that extends from OpenAPIRenderer and writing our own add_customizations method, I think I can export this and alphabetically order (#512) in a way that works on this project with a setting or something, let me know if you would like to have a PR or if it's done now

@marcgibbons
Copy link
Owner

@vladimirnani The grouping of URLs is done outside this project. The grouping (known as tags in Swagger) is converted from CoreAPI document links, and is generated by Django REST Framework.

This his how the encoder works from CoreAPI to OpenAPI formats:
https://github.com/core-api/python-openapi-codec/blob/master/openapi_codec/encode.py#L25

I'd be interested in knowing how your endpoints are grouped as CoreJSON - we can identify if this is a DRF thing, or something on the OpenAPI codec.

@angvp Saw your message. I'm about to release some basic SwaggerUI customizations (i.e. sorting, etc.) which ship out of the box with Swagger for v2.0.4. The more "schema" based overrides I think are best if they are upstream (i.e. in DRF / CoreAPI documents)

@angvp
Copy link

angvp commented Aug 16, 2016

@marcgibbons Ok, so tagging is not following a good logic IMO, is just taking the last part of the url, instead of the first part, and it uses that as a "tag".

I've tried to understand the logic of python-openapi-codec and DRF and I gave up after failing at several attempts, so I just did this in OpenAPIRenderer.

Also naming of the methods it's just awful, and it's "corrected" in a way that works better for my purposes..

You can use the code below @vladimirnani while we wait @tomchristie's input on this issue (though he wasn't aware of it until this mention)

from rest_framework_swagger.renderers import OpenAPIRenderer

class MyAPIRenderer(OpenAPIRenderer):
    API_BASE_PATH = getattr(settings, 'API_BASE_PATH', '/api/v1/')
    APP_HOSTNAME = getattr(settings, 'APP_HOSTNAME', '')

def _get_url_splitted(self, url):
    splitted_url = url.replace(self.API_BASE_PATH, '').split('/')

    url_parts = [
        x for x in splitted_url
        if '{' not in x
    ]
    return filter(None, url_parts)

def add_customizations(self, data, renderer_context):
    super(MyAPIRenderer, self).add_customizations(data,
                                                  renderer_context)
    data['host'] = self.APP_HOSTNAME # not yet sure if you fixed this on 2.0.4
    tags = []
    for url, values in data['paths'].iteritems():
        methods = values.keys()
        for method in methods:
            splitted_url = self._get_url_splitted(url)
            data['paths'][url][method]['tags'] = [splitted_url[0], ]
            _operation_id = "{}_{}".format(method, '_'.join(splitted_url))

            data['paths'][url][method]['operationId'] = _operation_id
            if splitted_url[0] not in tags:
                tags.append(splitted_url[0])
    # sort alphabetically before 2.0.4
    # data['tags'] = [{'name': x} for x in sorted(tags)]

And then use this class instead OpenAPIRenderer on the decorators of schema_view

@marcgibbons
Copy link
Owner

@angvp host and sorting are now released in 2.0.4.

As for the overrides, this doesn't belong here. The CoreAPI is generated in the schema generator and is grouped here: https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/schemas.py#L187.

I think what's needed here is to remove anything from the path that is common to all the patterns (i.e. /api/v1/) before grouping. Otherwise, /api/ could potentially be considered a category / tag.

@vladimirnani
Copy link
Author

vladimirnani commented Aug 16, 2016

@marcgibbons right, it seems that all list_route are considered "new tags" while detail_route is grouped correctly. Ok will try to play around with schema.

@tomchristie
Copy link
Contributor

@vladimirnani Can you confirm against 3.4.4. Believe this was resolved in encode/django-rest-framework#4394

@angvp
Copy link

angvp commented Aug 18, 2016

@tomchristie I am using 3.4.4 and still doesn't work, even without the ugly hack on add_customizations ^ (which btw grew up a little bit more cause some endpoints were from ListCreateAPIView were returning a list with just post method -and not get method-).

@tomchristie
Copy link
Contributor

even without the ugly hack on add_customizations

Do you mean add_categories? That briefly existed in master, but I don't think it's any release.

The get_key you reference in the opening comment exists in 3.4.3.

In 3.4.4 we use get_action and get_category. Note that we do also have a test case for exactly this issue in 3.4.4: https://github.com/tomchristie/django-rest-framework/blob/3.4.4/tests/test_schemas.py#L95

@angvp
Copy link

angvp commented Aug 18, 2016

even without the ugly hack on add_customizations
Do you mean add_categories? That briefly existed in master, but I don't think it's any release.

@tomchristie Oh, sorry for the confusion, when I said add_customizations I was referring to the one that I wrote for achieving this (see 4th comment on the thread).

And with 3.4.3 the get_category method is still producing this issue .. I will try to create a sample project similar that the one that I have in order to see where's the issue and how to improve all the projects.

@tomchristie
Copy link
Contributor

And with 3.4.3 the get_category method is still producing this issue

Presume you meant 3.4.4 here, right?

I will try to create a sample project similar that the one that I have in order to see where's the issue and how to improve all the projects.

That's great thanks. If you're able to replicate the issue in the existing test suite that'd certainly drive resolution of this forward.

@vladimirnani
Copy link
Author

@tomchristie 3.4.4 resolved, all list_routes are grouped. Thanks!!!
fixed

@angvp
Copy link

angvp commented Aug 22, 2016

Ehh not for me.

Please do not close this @marcgibbons .

This is how it should be seen.
screen shot 2016-08-22 at 10 04 19 am

And I get that splitted like this:

screen shot 2016-08-22 at 10 06 09 am
screen shot 2016-08-22 at 10 06 31 am
screen shot 2016-08-22 at 10 06 44 am

Is that the intended behaviour @tomchristie ? because that kind of grouping to me seems wrong.

@tomchristie
Copy link
Contributor

@angvp - I don't think the style you're showing there is not how the default routers/viewsets would generate the URLs?

Normally I'd expect to see, this...

/check/{pk}/disk/
/check/{pk}/mem/

I do believe that we may still have some improvements to make for correctly generating tags against plain views and URLs, but believe that we'd still be generating the second style, rather than the first. (No clear way from that URL style to determine exactly which of the two you'd be expecting)

You may want to customize the get_category method on your SchemaGenerator class in order to get the behavior you want.

@angvp
Copy link

angvp commented Aug 22, 2016

@tomchristie I don't use routers/viewsets but using generics .. when you go to /category/subcategory you get a list of subcategory I don't want to use /subcategory directly and it seems wrong to me to don't support subcategories on APIs is common than I thought, so maybe we should move this issue directly to DRF then.

@tomchristie
Copy link
Contributor

Do that yup. It'll likely be a case of us providing explicit ways to get the layout you want when the default case isn't quite right, but either way we'll need to make that possible/easier.

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

No branches or pull requests

4 participants