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

OpenAPI Schema Generation #223

Merged
merged 67 commits into from Jan 10, 2021
Merged

OpenAPI Schema Generation #223

merged 67 commits into from Jan 10, 2021

Conversation

dhaval-mehta
Copy link
Collaborator

@dhaval-mehta dhaval-mehta commented Mar 15, 2020

Hi @auvipy,

This fixes: #219, #237

You requested to wait for 3.12 Release. I will write code in such a way that it will be compatible will 3.12 Release.

This will close: #186 and #219

@auvipy
Copy link
Collaborator

auvipy commented Mar 16, 2020

OK

@dhaval-mehta
Copy link
Collaborator Author

dhaval-mehta commented Mar 19, 2020

Hi, @auvipy Whenever you are free time, please review code quality.
Now, I will work on Testcases and Documentation.

I do not know how to generate schema for models.GeometryField & models.GeometryCollectionField. Please provide guidance for the schema generation for these fields.

@auvipy
Copy link
Collaborator

auvipy commented Mar 19, 2020

@sir-sigurd would you mind sharing your insight on this? I saw you contribute a lot on geodjango front :)

@dhaval-mehta dhaval-mehta changed the title OpenAPI Schema Generation For Filters OpenAPI Schema Generation Apr 3, 2020
@compacq
Copy link

compacq commented Apr 4, 2020

#````````

Sent with GitHawk

@auvipy auvipy self-requested a review April 8, 2020 16:54
@nemesifier nemesifier added this to In progress in OpenWISP Priorities for next releases via automation Nov 10, 2020
@nemesifier nemesifier moved this from In progress to Ready for review/testing in OpenWISP Priorities for next releases Nov 10, 2020
@dhaval-mehta
Copy link
Collaborator Author

why?

These commits are no longer required. #248 fixes PostgreSQL integration problem.

@dhaval-mehta
Copy link
Collaborator Author

@auvipy Gentle reminder. I have merged the latest master.

OpenWISP Priorities for next releases automation moved this from Ready for review/testing to In progress Dec 30, 2020
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great progress @dhaval-mehta!

I left some comments below.

Anyone else here using/testing this who would like to leave feedback? @jayvdb @auvipy

README.rst Outdated Show resolved Hide resolved
tests/settings.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I guess the only way to get feedback on this feature is to publish it and see what happens, @dhaval-mehta will you stick around to help us maintain it?

@dhaval-mehta
Copy link
Collaborator Author

I guess the only way to get feedback on this feature is to publish it and see what happens, @dhaval-mehta will you stick around to help us maintain it?

Sure. It will be my pleasure to maintain this project.

@auvipy auvipy merged commit 4be23ef into openwisp:master Jan 10, 2021
OpenWISP Priorities for next releases automation moved this from In progress to Done Jan 10, 2021
@nemesifier
Copy link
Member

@dhaval-mehta your work is now released! I expect feedback from users of this library in the coming months.
I sent you an invite to have write access to this repository in the hope that you can help us maintain this feature as feedback from users comes in.

Pushes to master are restricted to admins, but pushes to other branches are allowed 👍.

@dhaval-mehta
Copy link
Collaborator Author

I will ask my network to test this as well. It will help us to improve the schema generation quickly.

@dhaval-mehta
Copy link
Collaborator Author

@dhaval-mehta your work is now released! I expect feedback from users of this library in the coming months.
I sent you an invite to have write access to this repository in the hope that you can help us maintain this feature as feedback from users comes in.

Pushes to master are restricted to admins, but pushes to other branches are allowed 👍.

Thank you so much for providing me this opportunity. It would be my pleasure to contribute to this repo.

@christiaan-dockbite
Copy link

Requiring users to set REST_FRAMEWORK.DEFAULT_SCHEMA_CLASS to a custom GIS specific class will limit the utility of this. The default DRF schema generator is rarely used for serious projects because there are so many unsolved problems in it. Most medium to large projects will use drf-yasg, or similar (including drf-spectacular which is new but quite good), and they use their own mechanism, either requiring that DEFAULT_SCHEMA_CLASS is set to their own class, or they ignore DEFAULT_SCHEMA_CLASS altogether. And many projects need to sub-class DRF's AutoSchema to add their own behaviour. I've tried to do a WIP for drf-spectacular to use the standard AutoSchema inheritance at tfranzel/drf-spectacular#46 , but it has has some curly problems.

Keeping GeoFeatureAutoSchema is ok, but it would be better as a thin wrapper only, using functionality that exists outside of the class.

i.e. the schemas and functions that are embedded here inside this class should be in the module namespace, so they can be used with other generators. The code in this custom AutoSchema only uses self. to reference the new code below -- it doesnt rely on any of the inner workings of DRF AutoSchema.

@dhaval-mehta Is there any way now that the schema generation is compatible with other schema generators, such as drf-spectacular? If not, is there a way we can make it compatible?

@dhaval-mehta
Copy link
Collaborator Author

dhaval-mehta commented Sep 24, 2021

Requiring users to set REST_FRAMEWORK.DEFAULT_SCHEMA_CLASS to a custom GIS specific class will limit the utility of this. The default DRF schema generator is rarely used for serious projects because there are so many unsolved problems in it. Most medium to large projects will use drf-yasg, or similar (including drf-spectacular which is new but quite good), and they use their own mechanism, either requiring that DEFAULT_SCHEMA_CLASS is set to their own class, or they ignore DEFAULT_SCHEMA_CLASS altogether. And many projects need to sub-class DRF's AutoSchema to add their own behaviour. I've tried to do a WIP for drf-spectacular to use the standard AutoSchema inheritance at tfranzel/drf-spectacular#46 , but it has has some curly problems.
Keeping GeoFeatureAutoSchema is ok, but it would be better as a thin wrapper only, using functionality that exists outside of the class.
i.e. the schemas and functions that are embedded here inside this class should be in the module namespace, so they can be used with other generators. The code in this custom AutoSchema only uses self. to reference the new code below -- it doesnt rely on any of the inner workings of DRF AutoSchema.

@dhaval-mehta Is there any way now that the schema generation is compatible with other schema generators, such as drf-spectacular? If not, is there a way we can make it compatible?

This implementation is compatible with all libraries which are compatible with DRF schema generation.
As per my knowledge, implementation of drf-spectacular is not compatible with DRF's schema generation. Hence I think, there is no way to make it compatible with drf-spectacular.

tfranzel/drf-spectacular#38 This PR is open. This can bring support for open API schema generation for drf-spectacular.

@auvipy
Copy link
Collaborator

auvipy commented Sep 25, 2021

drf-specticular need to upgrade its code to drf openAPI 3.0+

"style": "form",
"explode": False,
}
)

Choose a reason for hiding this comment

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

This needs to return the params. It is throwing an error in drf spectacular

Copy link
Collaborator

Choose a reason for hiding this comment

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

this implementation is not based on DRF spectacular btw

Choose a reason for hiding this comment

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

@auvipy yes but check all implementations of get_schema_operation_parameters, they all return data. This is the only one that does not. Settings params inside of get_schema_operation_parameters and then not returning it also does not make sense since no one can access these parameters, since they are not set on self or returned. It is a get function, which should return something. In this case the params.

Copy link
Collaborator

Choose a reason for hiding this comment

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

are you up for making a contribution? I will review it

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

Successfully merging this pull request may close these issues.

Add support for OpenAPI Schema Generation
8 participants