Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OpenAPI Schema Generation #223
Changes from 64 commits
d8cf934
a448b13
0859877
bce22da
75343a0
93b0f14
9f94293
c7421e4
69bc672
7b99c1e
563ce0d
9df1f4f
b0ff15c
99bf8cf
255f160
5598003
b830a74
2b7f862
e3f1494
540a7c4
2a8d1b0
f9ab386
60682e7
bbf392d
d2895d8
14ceb6b
af8773a
a94a7bf
dac80da
4a47129
f1a0a2b
d137946
2bdba3d
73bda51
e3cb217
02f34ca
1c2e457
3e7966f
e374ed4
01c1e92
59d346a
11430b4
331f764
1aefe46
8780ea6
8de2e18
27443e8
4c1dc11
e31c72d
f6ea625
fb01e25
83d3dd7
1d551ba
644c379
6abd208
1f14ee5
917b490
b1c30e6
0009d2a
5af7366
218fec5
c9ffd55
438527c
f890c91
d61949d
88d0b12
d0f8848
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_
. I think it is a clear instruction to others, not to use these methods. Also, this is an unreviewed code. I have already requested @auvipy to review this code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no comment on DRF master. I expected released versions of DRF to be usable, and in this case it isnt. Usually the problem is that it falls apart when an installable app using DRF isnt 'perfect', which is 80%+ of the time, and often they are slowly maintained, so it will be a long time before they will work - usually it is exceptions all over the place.
While drf-yasg is openapi 2.0, it is usable for most people, and they dont care much about v2 vs v3
I am not saying that drf-gis shouldnt provide a AutoSchema class. I just hope that the core functionality of this PR is outside of the AutoSchema class, and the AutoSchema class is a thin wrapper. Especially the schema definitions, which are first class entities that drf-gis should expose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have noted down your points. @auvipy Eagerly waiting for your review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually waiting for drf 3.12 release and celery 4.4.3[due by 5th of may]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @auvipy for the quick reply.
I have added a lot of extra code to make it compatible with DRF 3.10 and 3.11.
Should I remove that code and make schema generation only compatible with DRF 3.12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DRF 3.10 can be removed without any hesitation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like
GeometryCollection
isnt being covered by this PR. It usesgeometries
rather thangeometry
, and I dont see "geometries" mentioned anywhere in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @dhaval-mehta . Still no "geometries"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayvdb man, I am running very busy. I have just merged the latest code in my branch and resolved merge conflicts.
I will sure look into your suggestion but it may take 2-3 months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2-3 months? Do you mean after this PR is merged?
Unless you have tests passing that include "geometries", it is very likely that merging this PR will cause problems for anyone using
GeometryCollection
in their gis data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhaval-mehta can you clear this concern please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayvdb Added support for GeometryCollectionField and GeometryField
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks incorrect.
_map_field
is a method from DRF, and the argfield
is not a serializer. I have to wonder how a serializer is appearing here.Anyway,
_map_field
should handleif isinstance(field, GeometryField):
.I've added a commit jayvdb@85983bc with some of the things that seem needed for that.
I needed this when using spatialite, which may be a bit different under the cover than the postgres driver.
I have a travis build https://travis-ci.org/github/jayvdb/django-rest-framework-gis/jobs/682327850 and
test_point_field_inner_geo_list_serializer
might be a real breakage caused by my changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at line no 364 of rest_framework/schemas/openapi.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line no 369 of
rest_framework/schemas/openapi.py
will handle this case automatically.