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

Feature/make swagger json links browsable [OSF-6163] #6099

Conversation

NyanHelsing
Copy link
Contributor

@NyanHelsing NyanHelsing commented Aug 4, 2016

Purpose

Make any links in JSON responses in Swagger docks clickable.
Causes docs to open/scroll to the pertinent response.

Changes

Modify swagger-ui.js to make links in JSON clickable/browsable
Upgrade Swagger spec to 2.0 -- OpenAPI compatibility
CoreAPI compatibility
Upgrade DRF to 3.4
DRS to 2.0.4
update some OSF files to use unicode strings by default

Side effects

Swagger docs render two separate endpoints as if they were one;
node-file-list and node-file-detail as well as registration-file-list and registration-file-detail appear as though they were a single endpoint. This is due to the fact that the way django renders uritemplates from router regexes cannot account for certain aspects of regular expressions. Both file-list and file-detail are operable from the operation 'try it out' panel in the swagger docs.

Ticket

https://openscience.atlassian.net/browse/OSF-6163

@@ -19,8 +20,16 @@
url(r'^applications/', include('api.applications.urls', namespace='applications')),
url(r'^collections/', include('api.collections.urls', namespace='collections')),
url(r'^comments/', include('api.comments.urls', namespace='comments')),
url(r'^docs/', include('rest_framework_swagger.urls')),
url(r'^nodes/', include('api.nodes.urls', namespace='nodes')),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See L12 and please keep this list alphabetized (unless the new version of DRS auto-alphabetizes regardless if this list order, in which remove L12).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRS does alphabetize on the actual API -> Core api transition, though something weird happens between there and rendering on the documentation page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorted out the alphabetization thing.

@NyanHelsing
Copy link
Contributor Author

NyanHelsing commented Aug 23, 2016

Not sure what I'm missing. All tests run fine locally... I think there's something I haven't committed somehow.

@brianjgeiger
Copy link
Collaborator

======================================================================
FAIL: test_get_absolute_url (tests.test_models.TestNode)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/CenterForOpenScience/osf.io/tests/test_models.py", line 1625, in test_get_absolute_url
    .format(settings.API_DOMAIN, self.node._id)
AssertionError: 'http://localhost:8000/nodes/v5q9y/' != 'http://localhost:8000/v2/nodes/v5q9y/'

Did you change anything in there that you shouldn't have? Such as settings.API_DOMAIN? And are you running the api tests?

@NyanHelsing
Copy link
Contributor Author

I do have API_DOMAIN set to something different, only in local.py defaults is set to localhost:8000, which seems correct.

Huh. api settings is different.

@brianjgeiger
Copy link
Collaborator

Based on the test failures, I'm guessing that the new version of DRF is causing problems. Did you invoke test_api or otherwise run the api tests?

from django.conf.urls import url

from api.nodes import views
from website import settings

#nodeProviderDetailView = views.NodeProviderDetailView.as_view()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No commented out lines, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not certain I remember what I was trying here...

@brianjgeiger
Copy link
Collaborator

What happens when the next version of django-rest-swagger or the swagger-ui comes out?

}
)
mock_auth.return_value = self.user, mock_cas_resp
res = self.app.get(self.url, headers={'Authorization': 'Bearer {}'.format(token.token_id)})
res = self.app.get(self.url, headers={'Authorization': str('Bearer {}'.format(token.token_id))})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the chain of event that lead to these changes? The need for unicode_literals? The new version of DRF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new version of DRF requires unicode rather than bytestrings, so it was easier to change strings to unicode strings with the import unicode literals. A few things need to be explicitly bytestrings, like the (context?) dicts that are created from the headers here.

@brianjgeiger
Copy link
Collaborator

Pass done. 🐧 Most stuff in this pass is simple, except for the one big question.

@NyanHelsing
Copy link
Contributor Author

NyanHelsing commented Sep 1, 2016

Regarding what happens when DRS and swagger-ui get updated; the code in swagger-ui just need to gets inserted at the right spot. The code inserted is contiguous, and should be able to be placed in one location. as long as too much doesn't change all should be fine. The codebase here (barring the swagger-ui js) is relatively small, so it's fairly straightforward to understand what they may have changed. the big difference in the new version of DRS was updating the version of the spec to openapi from swagger.

@brianjgeiger
Copy link
Collaborator

Part of why I ask about the swagger-ui is because:

screen shot 2016-09-01 at 4 34 14 pm

So if you're adding the whole file plus the patch in one go, it's very difficult to see what's happening. When we talked at the beginning of the summer, you described this as a monkey-patch, which is generally a runtime patch rather than the whole file. If you haven't put your code directly into swagger-ui, then ideally it should be being brought in by another mechanism that doesn't involve a file put directly in /static/js/, and then you can patch that as a smaller diff?

If we have to maintain a modification to a static file, then we'll want to fork the repo and then make changes there, then import it with one build tool or another.

And if we have to be super careful about updating DRS / swagger-ui, it would be good to include a note next to where someone might do that update describing what would need to happen during that upgrade.

@NyanHelsing
Copy link
Contributor Author

At the beginning of the summer this was a monkey patch, but this new version isn't as nice to play around with. I can fork swagger-ui.

@brianjgeiger
Copy link
Collaborator

It's Autumn Cleaning time, so I'm closing this PR. Feel free to re-open when development becomes active again.

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.

3 participants