Skip to content

initial implementation of OAS 3.0 generateschema #689

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

initial implementation of OAS 3.0 generateschema #689

wants to merge 11 commits into from

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Aug 12, 2019

Fixes #604
(replaces #669)

Description of the Change

Extends DRF >= 3.10's generateschema to produce a jsonapi-formatted OAS schema document.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@n2ygk n2ygk requested a review from sliverc August 12, 2019 21:00
@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 13, 2019

I found a mistake in the tox.ini/.travis changes -- commit coming soon as I finish testing locally.

@codecov
Copy link

codecov bot commented Aug 13, 2019

Codecov Report

Merging #689 into master will decrease coverage by 2.7%.
The diff coverage is 76.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   95.96%   93.26%   -2.71%     
==========================================
  Files          54       58       +4     
  Lines        2728     3161     +433     
==========================================
+ Hits         2618     2948     +330     
- Misses        110      213     +103
Impacted Files Coverage Δ
...ork_json_api/management/commands/generateschema.py 0% <0%> (ø)
example/settings/dev.py 100% <100%> (ø) ⬆️
rest_framework_json_api/schemas/openapi.py 72.64% <72.64%> (ø)
example/tests/unit/test_filter_schema_params.py 75.86% <75.86%> (ø)
example/tests/test_openapi.py 96.05% <96.05%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c6cceb...78e92eb. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 13, 2019 •
edited
Loading

Codecov Report

Merging #689 into master will decrease coverage by 0.05%.
The diff coverage is 95.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   95.97%   95.92%   -0.06%     
==========================================
  Files          54       59       +5     
  Lines        2735     3138     +403     
==========================================
+ Hits         2625     3010     +385     
- Misses        110      128      +18
Impacted Files Coverage Δ
example/tests/test_filters.py 100% <ø> (ø) ⬆️
example/tests/integration/test_pagination.py 100% <ø> (ø) ⬆️
example/settings/dev.py 100% <ø> (ø) ⬆️
.../tests/integration/test_non_paginated_responses.py 100% <ø> (ø) ⬆️
example/urls_test.py 100% <ø> (ø) ⬆️
example/tests/test_relations.py 100% <ø> (ø) ⬆️
example/tests/test_views.py 100% <100%> (ø) ⬆️
example/tests/snapshots/snap_test_openapi.py 100% <100%> (ø)
rest_framework_json_api/django_filters/backends.py 100% <100%> (ø) ⬆️
example/tests/unit/test_filter_schema_params.py 100% <100%> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd8b535...18c677b. Read the comment docs.

@n2ygk n2ygk mentioned this pull request Aug 13, 2019
Closed
@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 14, 2019

@arielpontes I'd appreciate it if you could test this. Just add something like git+https://github.com/n2ygk/django-rest-framework-json-api.git@openapi_schema to your project's requirements.txt and then:

$ pip install -Ur requirements.txt
$ python manage.py generateschema >myschema.yaml

You can test the schema with something like swagger-ui-watcher:

$ npm install -g swagger-ui-watcher
$ swagger-ui-watcher myschema.yaml

Ignore the "DELETE operations cannot have a requestBody." messages.

sliverc
sliverc reviewed Aug 15, 2019
View reviewed changes
Copy link
Member

@sliverc sliverc left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for working on this.

This is quite a massive PR and very hard to review, so we need to make it smaller to get started. All in all when looking through it I have a feeling not all code is jsonapi specific.

I haven't dived into how generateschema works exactly so correct me if my suggestions do not seem to be valid.

Suggestions:

  • use snapshottest to simplify large asserts in tests
  • we could remove support for DRF 3.9 in master to make things simpler here. What do you think? (I don't see that now 3.10 is released and in DJA release 3.0.0 we remove almost all DRF releases why we should continue supporting 3.9)
  • oauth and rest_condition code resp. security code in general isn't jsonapi specification right? If so I rather we delete it from this PR. We can then still discuss at a later point where the code should best go.

If you have other ideas how we could start integrating this change with small PRs please feel free to comment. Above suggestions should at least make it simpler.

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 16, 2019

@sliverc Thanks for the the feedback.

I had intended to implement OAS here first before moving portions of the implementation that are commonly useful (like securitySchemes and security objects) to DRF PRs as Tom has indicated he wants to wait to see what implementations happen before making too many more changes there. I can look at doing that now and, in general, shrinking this into a more manageable PR.

Thanks for the snapshottest pointer.

Removing DRF 3.9 support sounds good. Should we make a 3.0 feature branch here or just drop 3.9 from the master branch?

@sliverc
Copy link
Member

sliverc commented Aug 16, 2019

Let's simply drop DRF 3.9 support from master in another PR

@andrew-snek
Copy link

andrew-snek commented Aug 19, 2019 •
edited
Loading

Getting this when trying to run 'generateschema':

Traceback (most recent call last):
  File "./manage.py", line 24, in <module>
    execute_from_command_line(sys.argv)
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "/code/.venv3/lib/python3.6/site-packages/rest_framework/management/commands/generateschema.py", line 40, in handle
    schema = generator.get_schema(request=None, public=True)
  File "/code/.venv3/lib/python3.6/site-packages/rest_framework_json_api/schemas/openapi.py", line 290, in get_schema
    paths = self.get_paths(None if public else request)
  File "/code/.venv3/lib/python3.6/site-packages/rest_framework_json_api/schemas/openapi.py", line 340, in get_paths
    operation = view.schema.get_operation(path, method, action)
TypeError: get_operation() takes 3 positional arguments but 4 were given

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 19, 2019

Getting this when trying to run 'generateschema':

Traceback (most recent call last):
  File "./manage.py", line 24, in <module>
    execute_from_command_line(sys.argv)
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/code/.venv3/lib/python3.6/site-packages/django/core/management/base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "/code/.venv3/lib/python3.6/site-packages/rest_framework/management/commands/generateschema.py", line 40, in handle
    schema = generator.get_schema(request=None, public=True)
  File "/code/.venv3/lib/python3.6/site-packages/rest_framework_json_api/schemas/openapi.py", line 290, in get_schema
    paths = self.get_paths(None if public else request)
  File "/code/.venv3/lib/python3.6/site-packages/rest_framework_json_api/schemas/openapi.py", line 340, in get_paths
    operation = view.schema.get_operation(path, method, action)
TypeError: get_operation() takes 3 positional arguments but 4 were given

Please share your INSTALLED_APPS from settings.py. I think this is because 'rest_framework_json_api' is not before 'rest_framework' per the updated documentation

@andrew-snek
Copy link

andrew-snek commented Aug 20, 2019 •
edited
Loading

Thanks for answering, but nope, my INSTALLED_APPS seem to be alright:

INSTALLED_APPS = [
    'django.contrib.admin',
    'django.contrib.auth',
    'django.contrib.contenttypes',
    'django.contrib.sessions',
    'django.contrib.messages',
    'django.contrib.staticfiles',
    'rest_framework_json_api',
    'rest_framework',
    # 'django_filters',
    # 'drf_yasg',
    'myapp'
]

If I put it in the wrong order, as you described, there's no error and it's just ignored. My REST_FRAMEWORK settings portion is straight from the linked docs, both serializers and views are imported from rest_framework_json_api.

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 20, 2019

@andrew-snek Sorry, I missed documenting a step. Now add here

Basically you need to make sure the DJA version of AutoSchema is used.

@andrew-snek
Copy link

No need to be sorry, you're doing this for free and I'm grateful.

Now a question that technically goes beyond the scope of this pull request, but I think will be useful to many people: by chance, do you have any ideas how to make this work with drf-yasg - a package which autogenerates web ui for the schema? I see you've subclassed the schema generator in this PR, and now stuff like this fails with 403; permission_classes which allow everything seem to be ignored:

from rest_framework import permissions
from drf_yasg.views import get_schema_view
from drf_yasg import openapi

schema_view = get_schema_view(
   openapi.Info(
      title="My API",
      default_version='v1',
   ),
   public=True,
   permission_classes=(permissions.AllowAny,),
)

from rest_framework_json_api.renderers import JSONRenderer
from rest_framework_json_api.schemas.openapi import SchemaGenerator
class CustomSchemaView(schema_view):
    generator_class = SchemaGenerator
    renderer_classes = (JSONRenderer,)
    permission_classes = (permissions.AllowAny,)

url_patterns = [
    url(r'^swagger/$', CustomSchemaView.with_ui('swagger', cache_timeout=0), name='schema-swagger-ui'),
    #...
]

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 21, 2019 •
edited
Loading

@andrew-snek Sorry, I didn't even know drf-yasg was a thing but I see it is limited to Swagger 2 (OAS 2.0): "Generate real Swagger/OpenAPI 2.0 specifications..."

This generateschema code generates OAS 3.0, specifically extending the support in DRF to document the JSON:API format for requests and responses.

See an example of using this stuff (in an early pre-release form) here. This looks to be doing the same thing as drf-yasg.

auvipy
auvipy reviewed Aug 21, 2019
View reviewed changes
Copy link
Contributor

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

great work! it will be in v3.0?

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 21, 2019

great work! it will be in v3.0?

@auvipy Still needs some more stuff cleaned up per @sliverc -- specifically pulling the security and securitySchemes out and submitting those to DRF where they belong.

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 22, 2019

@sliverc I've rebased. Can you take another look please?

auvipy
auvipy reviewed Aug 22, 2019
View reviewed changes
n2ygk added 6 commits August 22, 2019 15:02
@n2ygk
implement missing DjangoFilterBackend.get_schema_operation_parameters()
4a70020
@n2ygk
tests and improvements for related fields
4e9b43c
@n2ygk
add trailing /$ to urlpatterns
29eb776 
- works around a bug in django.contrib.admindocs.utils.replace_named_groups that fails to replace a named group if there's no trailing /
- only make the change to urls.py; urls_test.py has a bunch of tests that expect the / to be missing.
@n2ygk
add trailing / to urls_test as well
d43f7ca
@n2ygk
fix _expand_relationships to actually do that
e126b62
@n2ygk
add a TODO for SerializerMethodRelatedField schema
2e54524
@arielpontes
Copy link
Contributor

Hi, sorry I disappeared. I'm trying to run generateschema now, I got some errors and reading the thread I fixed some of them, also some were in my code (getting stuff from self.request in some views was breaking, so I added some getattrs and it did the job. I'm still getting this, however:

Traceback (most recent call last):
  File "manage.py", line 30, in <module>
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.7/site-packages/django/core/management/__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.7/site-packages/django/core/management/__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.7/site-packages/django/core/management/base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.7/site-packages/django/core/management/base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.7/site-packages/rest_framework/management/commands/generateschema.py", line 40, in handle
    schema = generator.get_schema(request=None, public=True)
  File "/usr/local/lib/python3.7/site-packages/rest_framework/schemas/openapi.py", line 64, in get_schema
    paths = self.get_paths(None if public else request)
  File "/usr/local/lib/python3.7/site-packages/rest_framework/schemas/openapi.py", line 47, in get_paths
    operation = view.schema.get_operation(path, method)
  File "/usr/local/lib/python3.7/site-packages/rest_framework_json_api/schemas/openapi.py", line 466, in get_operation
    parameters += self._get_filter_parameters(path, method)
  File "/usr/local/lib/python3.7/site-packages/rest_framework/schemas/openapi.py", line 187, in _get_filter_parameters
    parameters += filter_backend().get_schema_operation_parameters(self.view)
  File "/usr/local/lib/python3.7/site-packages/rest_framework_json_api/django_filters/backends.py", line 135, in get_schema_operation_parameters
    for res in super(DjangoFilterBackend, self).get_schema_operation_parameters(view):
AttributeError: 'super' object has no attribute 'get_schema_operation_parameters'

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 26, 2019 via email

@arielpontes
Copy link
Contributor

arielpontes commented Aug 27, 2019 •
edited
Loading

@n2ygk I've updated django-filters from 2.1.0 to 2.2.0 and made a few other changes and the generateschema command is now working :)

I'm still getting some errors in the Swagger UI though. This one appears multiple times:

😱 Could not render ParameterRow, see the console.

In the terminal where I ran the swagger-ui-watcher command, I don't see anything. In the UI page I see a red error section at the top of the page with a lot of error messages, too many to post here, but the first few look like this:

Errors
Hide
 
Schema error should NOT have additional properties
additionalProperty: Loading
Jump to line 0
Schema error at paths['/api/v1/user-profiles/'].put
should have required property 'responses'
missingProperty: responses
Jump to line 0
Schema error at paths['/api/v1/organizations/{organization_id}/members/{id}/'].put
should have required property 'responses'
missingProperty: responses
Jump to line 0
Schema error at paths['/api/v1/organizations/invites/{id}/'].put
should have required property 'responses'
missingProperty: responses
Jump to line 0
Schema error at paths['/api/v1/organizations/{id}/'].put
should have required property 'responses'
missingProperty: responses
Jump to line 0
Schema error at paths['/api/v1/documents/{id}/'].put
should have required property 'responses'
missingProperty: responses
Jump to line 0

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 27, 2019 via email

@n2ygk
Copy link
Contributor Author

n2ygk commented Aug 30, 2019

Per #697 I'll be rebasing this to undo 29eb776 and d43f7ca.

sliverc
sliverc requested changes Aug 31, 2019
View reviewed changes
Copy link
Member

@sliverc sliverc left a comment

Choose a reason for hiding this comment

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

@n2ygk
The PR is also still quite large so I haven't had the time to really look at all the details. I have done a first review though so you can keep working on it till I have more time at hand to do a more thorough review.

requirements-development.txt
@@ -15,3 +15,6 @@ recommonmark==0.6.0
Sphinx==2.1.2
sphinx_rtd_theme==0.4.3
twine==1.13.0
coreapi==2.3.3
Copy link
Member

Choose a reason for hiding this comment

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

let's keep this file alphabetically ordered.

rest_framework_json_api/django_filters/backends.py
for res in super(DjangoFilterBackend, self).get_schema_operation_parameters(view):
if 'name' in res:
res['name'] = 'filter[{}]'.format(res['name']).replace('__', '.')
result.append(res)
Copy link
Member

Choose a reason for hiding this comment

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

to create a new list but one line above to simply change a existing dictionary seems to be a bit odd.

I think it is properly easier especially as it is a super class call anyway to do something like this:

results = super(DjangoFilterBackend, self).get_schema_operation_parameters(view)
for result in results:
  if 'name' in res:
    result['name'] = 'filter[{}]'.format(result['name']).replace('__', '.')
return results

tox.ini
@@ -10,6 +10,8 @@ deps =
django22: Django>=2.2,<2.3
drf310: djangorestframework>=3.10.2,<3.11
drfmaster: https://github.com/encode/django-rest-framework/archive/master.zip
coreapi>=2.3.1
Copy link
Member

Choose a reason for hiding this comment

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

no need to add this here. Below requirements-development.txt will be installed

Copy link
Contributor

Choose a reason for hiding this comment

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

drf is going to drop coreapi

rest_framework_json_api/schemas/openapi.py
'type': 'object',
'additionalProperties': True
},
'datum': {
Copy link
Member

Choose a reason for hiding this comment

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

what is this?

requirements-development.txt
@@ -15,3 +15,6 @@ recommonmark==0.6.0
Sphinx==2.1.2
sphinx_rtd_theme==0.4.3
twine==1.13.0
coreapi==2.3.3
Copy link
Member

Choose a reason for hiding this comment

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

what is coreapi used for?

Copy link
Contributor

Choose a reason for hiding this comment

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

core api is deprecated

rest_framework_json_api/schemas/openapi.py
'components': JSONAPI_COMPONENTS,
}

return {**schema, **self.openapi_schema}
Copy link
Member

Choose a reason for hiding this comment

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

is it not easier to call super and simply add components to the returned result?

rest_framework_json_api/schemas/openapi.py
Extend DRF's SchemaGenerator to implement jsonapi-flavored generateschema command
"""
def __init__(self, *args, **kwargs):
self.openapi_schema = {}
Copy link
Member

Choose a reason for hiding this comment

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

DRF doesn't provide this - why do we introduce it in DJA?

@n2ygk n2ygk mentioned this pull request Sep 5, 2019
Closed
@n2ygk n2ygk mentioned this pull request Oct 11, 2019
Closed
1 task
@auvipy
Copy link
Contributor

auvipy commented Jan 17, 2020

hope to see this in soon

@keyz182
Copy link
Contributor

keyz182 commented Feb 13, 2020

@n2ygk - Will you be able to address the issues raised by @sliverc ? If not, we need this, so happy to take over.

@n2ygk
Copy link
Contributor Author

n2ygk commented Feb 13, 2020 via email

@keyz182
Copy link
Contributor

keyz182 commented Feb 13, 2020

Please do take over!
…
On Thu, Feb 13, 2020 at 5:30 AM Kieran Evans @.***> wrote: @n2ygk <@n2ygk> - Will you be able to address the issues raised by @sliverc <@sliverc> ? If not, we need this, so happy to take over. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#689?email_source=notifications&email_token=ABBHS55K5XITFUFQV3WDTGTRCUONZA5CNFSM4ILE7AL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELUHBJQ#issuecomment-585658534>, or unsubscribe <github.com/notifications/unsubscribe-auth/ABBHS5ZOKDJHT4O2K3E2ZMTRCUONZANCNFSM4ILE7ALQ> .

Can do. Any notes to pass on?
The main issue I see with the output currently is that these don't resolve -

      - $ref: '#/components/parameters/include'
      - $ref: '#/components/parameters/fields'
      - $ref: '#/components/parameters/sort'

Will also need to bring things in line with the latest release of this lib.

@n2ygk
Copy link
Contributor Author

n2ygk commented Feb 13, 2020 via email

@n2ygk
Copy link
Contributor Author

n2ygk commented Feb 13, 2020 via email

@keyz182
Copy link
Contributor

keyz182 commented Feb 13, 2020 via email

@keyz182 keyz182 mentioned this pull request Feb 20, 2020
Merged
5 tasks
@sliverc
Copy link
Member

sliverc commented Mar 8, 2020

Closing in favor of #772

@sliverc sliverc closed this Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auvipy auvipy auvipy left review comments

@sliverc sliverc sliverc requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support DRF 3.10+ generateschema (OAS 3)
6 participants
@n2ygk @sliverc @andrew-snek @arielpontes @auvipy @keyz182