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

Schemas should automatically respect API root location #4401

Closed
nick-allen opened this issue Aug 14, 2016 · 14 comments
Closed

Schemas should automatically respect API root location #4401

nick-allen opened this issue Aug 14, 2016 · 14 comments

Comments

@nick-allen
Copy link

Creating a schema should automatically provide any path prefixes to the registered router urls instead of defaulting to /.

It's possible to manually set this prefix using the DefaultRouter schema_url paramter, but this requires knowing where the router will be wired into Django URLs.

Steps to reproduce

Install django, rest framework, and coreapi. Configure normal Django boilerplate

Create a viewset

# viewsets.py

from rest_framework.viewsets import ModelViewSet
from rest_framework.serializers import ModelSerializer
from django.contrib.auth import get_user_model

User = get_user_model()


class UserSerializer(ModelSerializer):
    class Meta:
        model = User


class UserViewSet(ModelViewSet):

    queryset = User.objects.all()
    serializer_class = UserSerializer

Create a router with the schema_title argument to setup automatic CoreAPI renderer and register viewsets.
Include router URLs under a path not at root /

# urls.py
from django.conf.urls import url, include

from rest_framework.routers import DefaultRouter

from .viewsets import UserViewSet

v1_router = DefaultRouter(
    schema_title='Example API v1',
)

v1_router.register('users', UserViewSet)

urlpatterns = [
    url(r'^api/v1/', include(v1_router.urls, namespace='v1')),
]

Get schema document and verify url does not include API path prefix

$ curl http://localhost:8000/api/v1/?format=corejson
{
    "_type": "document",
    "_meta": {
        "title": "Example API v1"
    },
    "users": {
        "create": {
            "_type": "link",
            "url": "/users/",
            "action": "post",
            "encoding": "application/json",
            "fields": [
                {
                    "name": "password",
                    "required": true,
                    "location": "form"
                },
                {
                    "name": "last_login",
                    "location": "form"
                },
                {
                    "name": "is_superuser",
                    "location": "form",
                    "description": "Designates that this user has all permissions without explicitly assigning them."
                },
                {
                    "name": "username",
                    "required": true,
                    "location": "form",
                    "description": "Required. 30 characters or fewer. Letters, digits and @/./+/-/_ only."
                },
                {
                    "name": "first_name",
                    "location": "form"
                },
                {
                    "name": "last_name",
                    "location": "form"
                },
                {
                    "name": "email",
                    "location": "form"
                },
                {
                    "name": "is_staff",
                    "location": "form",
                    "description": "Designates whether the user can log into this admin site."
                },
                {
                    "name": "is_active",
                    "location": "form",
                    "description": "Designates whether this user should be treated as active. Unselect this instead of deleting accounts."
                },
                {
                    "name": "date_joined",
                    "location": "form"
                },
                {
                    "name": "groups",
                    "location": "form",
                    "description": "The groups this user belongs to. A user will get all permissions granted to each of their groups."
                },
                {
                    "name": "user_permissions",
                    "location": "form",
                    "description": "Specific permissions for this user."
                }
            ]
        },
        "destroy": {
            "_type": "link",
            "url": "/users/{pk}/",
            "action": "delete",
            "fields": [
                {
                    "name": "pk",
                    "required": true,
                    "location": "path"
                }
            ]
        },
        "list": {
            "_type": "link",
            "url": "/users/",
            "action": "get",
            "fields": [
                {
                    "name": "page",
                    "location": "query"
                }
            ]
        },
        "partial_update": {
            "_type": "link",
            "url": "/users/{pk}/",
            "action": "patch",
            "encoding": "application/json",
            "fields": [
                {
                    "name": "pk",
                    "required": true,
                    "location": "path"
                },
                {
                    "name": "password",
                    "location": "form"
                },
                {
                    "name": "last_login",
                    "location": "form"
                },
                {
                    "name": "is_superuser",
                    "location": "form",
                    "description": "Designates that this user has all permissions without explicitly assigning them."
                },
                {
                    "name": "username",
                    "location": "form",
                    "description": "Required. 30 characters or fewer. Letters, digits and @/./+/-/_ only."
                },
                {
                    "name": "first_name",
                    "location": "form"
                },
                {
                    "name": "last_name",
                    "location": "form"
                },
                {
                    "name": "email",
                    "location": "form"
                },
                {
                    "name": "is_staff",
                    "location": "form",
                    "description": "Designates whether the user can log into this admin site."
                },
                {
                    "name": "is_active",
                    "location": "form",
                    "description": "Designates whether this user should be treated as active. Unselect this instead of deleting accounts."
                },
                {
                    "name": "date_joined",
                    "location": "form"
                },
                {
                    "name": "groups",
                    "location": "form",
                    "description": "The groups this user belongs to. A user will get all permissions granted to each of their groups."
                },
                {
                    "name": "user_permissions",
                    "location": "form",
                    "description": "Specific permissions for this user."
                }
            ]
        },
        "retrieve": {
            "_type": "link",
            "url": "/users/{pk}/",
            "action": "get",
            "fields": [
                {
                    "name": "pk",
                    "required": true,
                    "location": "path"
                }
            ]
        },
        "update": {
            "_type": "link",
            "url": "/users/{pk}/",
            "action": "put",
            "encoding": "application/json",
            "fields": [
                {
                    "name": "pk",
                    "required": true,
                    "location": "path"
                },
                {
                    "name": "password",
                    "required": true,
                    "location": "form"
                },
                {
                    "name": "last_login",
                    "location": "form"
                },
                {
                    "name": "is_superuser",
                    "location": "form",
                    "description": "Designates that this user has all permissions without explicitly assigning them."
                },
                {
                    "name": "username",
                    "required": true,
                    "location": "form",
                    "description": "Required. 30 characters or fewer. Letters, digits and @/./+/-/_ only."
                },
                {
                    "name": "first_name",
                    "location": "form"
                },
                {
                    "name": "last_name",
                    "location": "form"
                },
                {
                    "name": "email",
                    "location": "form"
                },
                {
                    "name": "is_staff",
                    "location": "form",
                    "description": "Designates whether the user can log into this admin site."
                },
                {
                    "name": "is_active",
                    "location": "form",
                    "description": "Designates whether this user should be treated as active. Unselect this instead of deleting accounts."
                },
                {
                    "name": "date_joined",
                    "location": "form"
                },
                {
                    "name": "groups",
                    "location": "form",
                    "description": "The groups this user belongs to. A user will get all permissions granted to each of their groups."
                },
                {
                    "name": "user_permissions",
                    "location": "form",
                    "description": "Specific permissions for this user."
                }
            ]
        }
    }
}

Expected behavior

Using coreapi cli, use any actions following schema tutorial in docs (http://www.django-rest-framework.org/tutorial/7-schemas-and-client-libraries/)

Get schema document

$ coreapi get http://localhost:8000/api/v1/
<Example API v1 "http://localhost:8000/api/v1/">
    users: {
        create(password, username, [last_login], [is_superuser], [first_name], [last_name], [email], [is_staff], [is_active], [date_joined], [groups], [user_permissions])
        destroy(pk)
        list([page])
        partial_update(pk, [password], [last_login], [is_superuser], [username], [first_name], [last_name], [email], [is_staff], [is_active], [date_joined], [groups], [user_permissions])
        retrieve(pk)
        update(pk, password, username, [last_login], [is_superuser], [first_name], [last_name], [email], [is_staff], [is_active], [date_joined], [groups], [user_permissions])
    }

Run list action

$ coreapi action users list
[
    {
        "id": 1,
        "password": "!YOnSaMeJECJU0oeULffq6tp6TB3WAlTLTngqXBpT",
        "last_login": null,
        "is_superuser": false,
        "username": "user",
        "first_name": "",
        "last_name": "",
        "email": "",
        "is_staff": false,
        "is_active": true,
        "date_joined": "2016-08-14T18:10:37.880469Z",
        "groups": [],
        "user_permissions": []
    }
]

Actual behavior

Get schema document

$ coreapi get http://localhost:8000/api/v1/

<Example API v1 "http://localhost:8000/api/v1/">
    users: {
        create(password, username, [last_login], [is_superuser], [first_name], [last_name], [email], [is_staff], [is_active], [date_joined], [groups], [user_permissions])
        destroy(pk)
        list([page])
        partial_update(pk, [password], [last_login], [is_superuser], [username], [first_name], [last_name], [email], [is_staff], [is_active], [date_joined], [groups], [user_permissions])
        retrieve(pk)
        update(pk, password, username, [last_login], [is_superuser], [first_name], [last_name], [email], [is_staff], [is_active], [date_joined], [groups], [user_permissions])
    }

Run list action

$ coreapi action users list
<Error: Not Found>
    message: "
              <!DOCTYPE html>
              <html lang=\"en\">
              <head>
                <meta http-equiv=\"content-type\" content=\"text/html; charset=utf-8\">
                <title>Page not found at /users/</title>
                <meta name=\"robots\" content=\"NONE,NOARCHIVE\">
                <style type=\"text/css\">
                  html * { padding:0; margin:0; }
                  body * { padding:10px 20px; }
                  body * * { padding:0; }
                  body { font:small sans-serif; background:#eee; }
                  body>div { border-bottom:1px solid #ddd; }
                  h1 { font-weight:normal; margin-bottom:.4em; }
                  h1 span { font-size:60%; color:#666; font-weight:normal; }
                  table { border:none; border-collapse: collapse; width:100%; }
                  td, th { vertical-align:top; padding:2px 3px; }
                  th { width:12em; text-align:right; color:#666; padding-right:.5em; }
                  #info { background:#f6f6f6; }
                  #info ol { margin: 0.5em 4em; }
                  #info ol li { font-family: monospace; }
                  #summary { background: #ffc; }
                  #explanation { background:#eee; border-bottom: 0px none; }
                </style>
              </head>
              <body>
                <div id=\"summary\">
                  <h1>Page not found <span>(404)</span></h1>
                  <table class=\"meta\">
                    <tr>
                      <th>Request Method:</th>
                      <td>GET</td>
                    </tr>
                    <tr>
                      <th>Request URL:</th>
                      <td>http://localhost:8000/users/</td>
                    </tr>

                  </table>
                </div>
                <div id=\"info\">

                    <p>
                    Using the URLconf defined in <code>drfbug.urls</code>,
                    Django tried these URL patterns, in this order:
                    </p>
                    <ol>

                        <li>

                              ^api/v1/


                        </li>

                    </ol>
                    <p>The current URL, <code>users/</code>, didn't match any of these.</p>

                </div>

                <div id=\"explanation\">
                  <p>
                    You're seeing this error because you have <code>DEBUG = True</code> in
                    your Django settings file. Change that to <code>False</code>, and Django
                    will display a standard 404 page.
                  </p>
                </div>
              </body>
              </html>
              "
@tomchristie
Copy link
Member

It's possible to manually set this prefix using the DefaultRouter schema_url paramter, but this requires knowing where the router will be wired into Django URLs.

I'm not sure that we can automagically determine this in a nice way.

Setting the path explicitly is the right thing for you to do, at least for now.

Perhaps we'll revisit this at some point in the future.

@klada
Copy link

klada commented Oct 7, 2016

I would like to bring this up again, as the bug is not fixed.

The issue actually is that the URLs in the schema are wrong. Why is it not possible to simply use reverse() for generating the URLs in the schema? This way the URLs would always be correct. Everything else violates the DRY principle and is error-prone

@tomchristie
Copy link
Member

@klada - Be specific. There's a whole bunch of fixes in the incoming 3-5 branch. If you believe there's still an issue on that branch then show me an example case that we can use to help resolve things.

@tomchristie
Copy link
Member

tomchristie commented Oct 7, 2016

Okay, have read through this issue again.

Going to re-iterate this:

Setting the path explicitly is the right thing for you to do, at least for now.

Yes, that means making sure you know where it's located. Them's the breaks. I'll take another review of this prior to the 3.5 release, but I doubt that aspect is going to change.

@tomchristie tomchristie mentioned this issue Oct 7, 2016
26 tasks
@nick-allen
Copy link
Author

@tomchristie, to @klada's point, this does violate DRY. I think I gave a pretty detailed example case in the description, if there's something else you'd like to see, I'm happy to put something together.

As far as this not changing, is there something specific blocking this? I dug through the schema URL setup code, only issue I found was that I wasn't able to inspect the api_root dynamically if it wasn't provided due to when the URL setup was run.

@tomchristie
Copy link
Member

Okay, on review can confirm that this issue is resolved in the upcoming 3.5 release.

And you're right, the example was plenty specific enough.

There's a few further improvements that I can see are also required based on the example here, which I'll work towards resolving prior to the release.

@tomchristie tomchristie added this to the 3.5.0 Release milestone Oct 7, 2016
@tomchristie tomchristie reopened this Oct 7, 2016
@tomchristie
Copy link
Member

Reopening to leave as a placeholder for resolving schema layout with URL prefixes.

@tomchristie
Copy link
Member

tomchristie commented Oct 7, 2016

Resolved via 69b4acd (incoming in 3.5)

Thanks for having highlighted this & apologies for not having fully understood the issue on first pass!

tomchristie added a commit that referenced this issue Oct 10, 2016
* Start test case

* Added 'requests' test client

* Address typos

* Graceful fallback if requests is not installed.

* Add cookie support

* Tests for auth and CSRF

* Py3 compat

* py3 compat

* py3 compat

* Add get_requests_client

* Added SchemaGenerator.should_include_link

* add settings for html cutoff on related fields

* Router doesn't work if prefix is blank, though project urls.py handles prefix

* Fix Django 1.10 to-many deprecation

* Add django.core.urlresolvers compatibility

* Update django-filter & django-guardian

* Check for empty router prefix; adjust URL accordingly

It's easiest to fix this issue after we have made the regex.  To try
to fix it before would require doing something different for List vs
Detail, which means we'd have to know which type of url we're
constructing before acting accordingly.

* Fix misc django deprecations

* Use TOC extension instead of header

* Fix deprecations for py3k

* Add py3k compatibility to is_simple_callable

* Add is_simple_callable tests

* Drop python 3.2 support (EOL, Dropped by Django)

* schema_renderers= should *set* the renderers, not append to them.

* API client (#4424)

* Fix release notes

* Add note about 'User account is disabled.' vs 'Unable to log in'

* Clean up schema generation (#4527)

* Handle multiple methods on custom action (#4529)

* RequestsClient, CoreAPIClient

* exclude_from_schema

* Added 'get_schema_view()' shortcut

* Added schema descriptions

* Better descriptions for schemas

* Add type annotation to schema generation

* Coerce schema 'pk' in path to actual field name

* Deprecations move into assertion errors

* Use get_schema_view in tests

* Updte CoreJSON media type

* Handle schema structure correctly when path prefixs exist. Closes #4401

* Add PendingDeprecation to Router schema generation.

* Added SCHEMA_COERCE_PATH_PK and SCHEMA_COERCE_METHOD_NAMES

* Renamed and documented 'get_schema_fields' interface.
@tomchristie tomchristie mentioned this issue Oct 10, 2016
34 tasks
@klada
Copy link

klada commented Nov 10, 2016

This issue is still present in 3.5. Please see this demo project for a quick demonstration of the issue.

I have also included a README containing the current (wrong) and the expected corejson output.

@moorchegue
Copy link

As far as I'm concerned it's still present in the latest release. It's especially annoying when coreapi client is being used, because there's no way to add a prefix manually on the client side.

Is this considered not an issue anymore? Any hints for non-hacky solutions?

@carltongibson
Copy link
Collaborator

So there's a test added for this

69b4acd#diff-42ea969a60c167b848d4c4910d590060R294

I'd very happily look at a PR that included/added a failing example showing what the issue is meant to be.

@moorchegue
Copy link

Thanks for pointing out! I guess the test case I'm looking for is settings.BASE_URL = '/some/prefix/', which the test doesn't cover.

@carltongibson
Copy link
Collaborator

@moorchegue Does the url parameter on SchemaGenerator not give you what you need?

@moorchegue
Copy link

Oh, yes, it does. Thanks a lot!

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

No branches or pull requests

5 participants