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

Better support for ModelViewSet (through appropriate router attribution based on settings) - And fix incomplete API paths when recurrence is used #132

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dimitri-justeau
Copy link

@dimitri-justeau dimitri-justeau commented Aug 24, 2016

This pull request is an attempt to provide a better ModelViewSet support:

  • It solves this issue: ModelViewSet doesn't show request methods, instead it only show option button. The request methods where not shown because of the ApiEndpoint objects not being instantiated with a drf_router, which is necessary to know which http method is bound to which python method. What I did is adding the possibility to indicate in settings how the ApiDocumentation object should bind an ApiEndpoint to a router. There are 3 possibilities:
    1. Map a urls module with a router instance ("MODULE_ROUTERS" setting).
    2. Set a default router instance name that will be search in each urls module ("DEFAULT_MODULE_ROUTER" setting).
    3. Define a global default router instance ("DEFAULT_ROUTER" setting).
  • It also solves the same problem as described in this pull request: Deep recurrence displays invalid path. I did not see this pull request before starting working on this one, but it appeared obvious that is should be solved in this context because using ModelViewSet and routers involves, most of the time, recursion in the url resolving.

To achieve this, I slightly modified the ApiEndpoint model, adding a ApiNode class (which is a super class of ApiEndpoint). The API is then seen as a tree, with leaves being ApiEndpoint instances, that will be documented. The ancestors of an ApiEndpoint are inspected to build the full url, and also to know which drf_router should be used to get the http methods.

When an include pattern is used in a urls module, if the regex pattern is empty, all the urls included are shown as being part of the same group as the current urls module. If the regex is not empty, a new group is show, using this pattern: <parent_group_pattern>/<include_group_pattern>.

For instance, consider the following urls:

urlpatterns = [
    url(r'^login/$', views.LoginView.as_view(), name="login"),
    url(r'^register/$', views.UserRegistrationView.as_view(), name="register"),
    url(r'^register/$', views.UserRegistrationView.as_view(), name="register"),
    url(r'^reset-password/$', view=views.PasswordResetView.as_view(), name="reset-password"),
    url(r'^reset-password/confirm/$', views.PasswordResetConfirmView.as_view(), name="reset-password-confirm"),
    url(r'^user/profile/$', views.UserProfileView.as_view(), name="profile"),

    url(r'^viewset_test/', include(router.urls), name="user_viewset"),
]

with those urls being added to the project's url module as:

url(r'^accounts/', view=include('project.accounts.urls', namespace='accounts')),

The documentation will be generated in two sections: accounts, and accounts/viewset_test

If viewset_test were defined as url(r'^', include(router.urls), name="user_viewset"), the only generated documentation group would have been accounts with all the urls of viewset_test included in it.

Using this tree model, future improvement are also made possible, such as having different documentation sections and levels in the UI.

I can make changes to this pull request, or enhancements if necessary!

EDIT: Using it, I can see a lot of improvements that could be done. So the main idea of this PR is mainly about using this ApiNode / ApiEndpoint model. I don't know for the moment if there would be a way to automatically discover the router(s) associated to a urlpattern, but for sure it would be the perfect solution!

@dimitri-justeau
Copy link
Author

UPDATE: I made a PR to django-rest-framework (Transparently keep reference of routers in urls generated by routers) which would enable an easy and automatic discovery of routers when inspecting API endpoints. I also made the change in a branch of my drfdocs fork (https://github.com/dimitri-justeau/django-rest-framework-docs/tree/use-drf-resolvers-with-router-reference) in case the PR gets accepted, and it works like a charm!

@decko
Copy link

decko commented Sep 9, 2016

👍

@johndavidback
Copy link

Would love to see this merged. Anything I can do to help?

@BuckinghamIO
Copy link

Any further progress?

@makslevental
Copy link

makslevental commented Sep 6, 2017

bump?

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

Successfully merging this pull request may close these issues.

None yet

5 participants