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

Switch API Documentation to DRF-YASG #3133

Merged
merged 3 commits into from Aug 6, 2019
Merged

Switch API Documentation to DRF-YASG #3133

merged 3 commits into from Aug 6, 2019

Conversation

@rajadain
Copy link
Member

rajadain commented Aug 1, 2019

Overview

The Django Upgrade in #3123 broken Django Rest Swagger, a project that is no longer maintained. We switch to the recommended (by the author of Django Rest Swagger) alternative drf-yasg for the API generation instead.

This new tool doesn't use the YAML documentation previously there, instead relying on OpenAPI Schema decoration. The documentation is converted to this new format, and some supplemental instruction is added as well.

Connects #3124

Demo

Screen Shot 2019-07-30 at 18 30 51-fullpage

Testing Instructions

  • Check out this branch and reprovision your app VM to install the new dependencies

    $ vagrant reload --provision app
    
  • Go to :8000/api/docs and ensure you see the Swagger UI, and not an error page

  • Compare the values with production https://modelmywatershed.org/api/docs/ and ensure that the information there has been faithfully captured

  • Ensure there are no typos or confusing sentences in the opening instructions near the header

@rajadain rajadain added the WPF label Aug 1, 2019
@rajadain rajadain requested a review from mmcfarland Aug 1, 2019
@mmcfarland

This comment has been minimized.

Copy link
Member

mmcfarland commented Aug 2, 2019

I generated a Token and authorized the docs with Token XXX-XXX but I get:

{
  "detail": "CSRF Failed: CSRF token missing or incorrect."
}
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Aug 2, 2019

This is very odd. I was able to reproduce the above error in the morning, but after restarting the VM I can no longer reproduce it. Now, I am able to make API requests without CSRF tokens:

$ echo '{"coordinates":[[[[-75.16468,39.97054],[-75.17086,39.95001],[-75.14888,39.95975],[-75.16468,39.97054]]]],"type":"MultiPolygon"}' | http --print HhBb POST :8000/api/analyze/land/ Accept:application/json Authorization:"Token d0faa35ccd57214e5496f7f50ce1fc4a07c00013"

POST /api/analyze/land/ HTTP/1.1
Accept: application/json
Accept-Encoding: gzip, deflate
Authorization: Token d0faa35ccd57214e5496f7f50ce1fc4a07c00013
Connection: keep-alive
Content-Length: 128
Content-Type: application/json
Host: localhost:8000
User-Agent: HTTPie/1.0.2

{
    "coordinates": [
        [
            [
                [
                    -75.16468,
                    39.97054
                ],
                [
                    -75.17086,
                    39.95001
                ],
                [
                    -75.14888,
                    39.95975
                ],
                [
                    -75.16468,
                    39.97054
                ]
            ]
        ]
    ],
    "type": "MultiPolygon"
}

HTTP/1.1 200 OK
Allow: POST, OPTIONS
Connection: keep-alive
Content-Encoding: gzip
Content-Type: application/json
Date: Fri, 02 Aug 2019 19:19:07 GMT
Location: /api/jobs/c9643a13-3194-499c-9419-779fccfb58c6/
Server: nginx
Transfer-Encoding: chunked
Vary: Accept-Encoding
Vary: Accept, Cookie, Origin

{
    "job": "c9643a13-3194-499c-9419-779fccfb58c6",
    "status": "started"
}

Will try a couple of other workflows to see if I can reproduce it again.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Aug 5, 2019

According to https://drf-yasg.readthedocs.io/en/1.0.2/readme.html?highlight=csrf#c-swagger-settings-and-redoc-settings, the Swagger UI adds an X-CSRFToken header only when USE_SESSION_AUTH is enabled. This header is required for CSRF verification. We turned USE_SESSION_AUTH off because our API works off Token Authentication, not Session Authentication (which is in place only for the web app, not for API users).

With USE_SESSION_AUTH turned on, we do get the X-CSRFToken header (and successful requests), but also the Django Login / Logout button:

image

Our options now are:

  1. Leave the Django Login / Logout button in the interface enabled
    • Optionally add some text in the intro instructing the user to ignore it
  2. Override the core swagger-ui.html template in DRF-YASG to try and hide that button, even when USE_SESSION_AUTH is turned on https://drf-yasg.readthedocs.io/en/1.0.2/custom_ui.html
  3. Add a @csrf_exempt decorator to our API views so they can be queried without CSRF protection. They will still need Session / Token Authentication to work, and will still be logged.

I think doing 1 is cheapest, but 3 is most graceful, if we can accept those views not being CSRF protected. Thoughts?

@mmcfarland

This comment has been minimized.

Copy link
Member

mmcfarland commented Aug 5, 2019

I agree with your assessment. Given that we have the CSRF protection currently enabled, it doesn't seem like a great trade-off to lose that in order to get a marginally easier to work with API doc page. I suggest we go with 1 and add some small text to instruct users to it's use (or non-use, as it were), as you suggest.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Aug 5, 2019

Alright, ready for another look. Updated the settings and added a line to ignore Django Login in ad1a60f. Also added an empty space at the end of each sentence in the description so adding lines in the future doesn't affect existing ones.

Copy link
Member

mmcfarland left a comment

Looks good, submitted several jobs through the API docs with my token.

@mmcfarland mmcfarland assigned rajadain and unassigned mmcfarland Aug 6, 2019
rajadain added 3 commits Aug 1, 2019
The Django Rest Swagger version we were previously using was
not compatible with Django 1.11. The most recent version is,
but has a completely different API. Furthermore, it is now
abandoned and no longer actively maintained.

We instead switch to using DRF-YASG, an actively maintained
project which also uses a new API, and supports a newer UI
for Swagger.

It also supports ReDoc, a different API documentation tool,
which we can use in the future if needed.
As part of the new style of Swagger document generation,
DRF-YASG uses these OpenAPI schema definitions instead of
the YAML style used previously.

Unfortunately, specifying a request_body in the decorator
marks it as required in the generated documentation. This
is countered via instruction to the contrary in the header
section.
As was done for other cases in 107d4a9.
@rajadain rajadain force-pushed the tt/drf-yasg branch from ad1a60f to c927fad Aug 6, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Aug 6, 2019

Thanks for the detailed testing! Squashed the fixup. Merging now.

@rajadain rajadain merged commit 5e6f4fb into develop Aug 6, 2019
@rajadain rajadain deleted the tt/drf-yasg branch Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.