-
Notifications
You must be signed in to change notification settings - Fork 202
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
Changes from all 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,3 +3,4 @@ django-filter>=2.0 | |
contexttimer | ||
# QA checks | ||
openwisp-utils[qa]~=0.6 | ||
packaging~=20.4 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
import warnings | ||
|
||
from django.contrib.gis.db import models | ||
from rest_framework.schemas.openapi import AutoSchema | ||
from rest_framework.utils import model_meta | ||
|
||
from rest_framework_gis.fields import GeometrySerializerMethodField | ||
from rest_framework_gis.serializers import ( | ||
GeoFeatureModelListSerializer, | ||
GeoFeatureModelSerializer, | ||
) | ||
|
||
|
||
class GeoFeatureAutoSchema(AutoSchema): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Thank you @auvipy for the quick reply. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DRF 3.10 can be removed without any hesitation! |
||
COORDINATES_SCHEMA_FOR_POINT = { | ||
"type": "array", | ||
"items": {"type": "number", "format": "float"}, | ||
"example": [12.9721, 77.5933], | ||
"minItems": 2, | ||
"maxItems": 3, | ||
} | ||
|
||
COORDINATES_SCHEMA_FOR_LINE_STRING = { | ||
"type": "array", | ||
"items": COORDINATES_SCHEMA_FOR_POINT, | ||
"example": [[22.4707, 70.0577], [12.9721, 77.5933]], | ||
"minItems": 2, | ||
} | ||
|
||
GEO_FIELD_TO_SCHEMA = { | ||
models.PointField: { | ||
"type": {"type": "string", "enum": ["Point"]}, | ||
"coordinates": COORDINATES_SCHEMA_FOR_POINT, | ||
}, | ||
models.LineStringField: { | ||
"type": {"type": "string", "enum": ["LineString"]}, | ||
"coordinates": COORDINATES_SCHEMA_FOR_LINE_STRING, | ||
}, | ||
models.PolygonField: { | ||
"type": {"type": "string", "enum": ["Polygon"]}, | ||
"coordinates": { | ||
"type": "array", | ||
"items": {**COORDINATES_SCHEMA_FOR_LINE_STRING, "minItems": 4}, | ||
"example": [ | ||
[0.0, 0.0], | ||
[0.0, 50.0], | ||
[50.0, 50.0], | ||
[50.0, 0.0], | ||
[0.0, 0.0], | ||
], | ||
}, | ||
}, | ||
} | ||
|
||
GEO_FIELD_TO_SCHEMA[models.GeometryField] = { | ||
'type': {'type': 'string'}, | ||
'coordinates': { | ||
'oneOf': [ # If you have custom subclass of GeometryField, Override `oneOf` property. | ||
GEO_FIELD_TO_SCHEMA[models.PointField], | ||
GEO_FIELD_TO_SCHEMA[models.LineStringField], | ||
GEO_FIELD_TO_SCHEMA[models.PolygonField], | ||
], | ||
'example': GEO_FIELD_TO_SCHEMA[models.PolygonField]['coordinates'][ | ||
'example' | ||
], | ||
}, | ||
} | ||
|
||
MULTI_FIELD_MAPPING = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @jayvdb Added support for GeometryCollectionField and GeometryField |
||
models.PointField: models.MultiPointField, | ||
models.LineStringField: models.MultiLineStringField, | ||
models.PolygonField: models.MultiPolygonField, | ||
models.GeometryField: models.GeometryCollectionField, | ||
} | ||
|
||
for singular_field, multi_field in MULTI_FIELD_MAPPING.items(): | ||
GEO_FIELD_TO_SCHEMA[multi_field] = { | ||
"type": {"type": "string", "enum": [multi_field.geom_class.__name__]}, | ||
"coordinates": { | ||
"type": "array", | ||
"items": GEO_FIELD_TO_SCHEMA[singular_field]["coordinates"], | ||
"example": [ | ||
GEO_FIELD_TO_SCHEMA[singular_field]["coordinates"]["example"] | ||
], | ||
}, | ||
} | ||
|
||
def _map_geo_field(self, serializer, geo_field_name): | ||
field = serializer.fields[geo_field_name] | ||
if isinstance(field, GeometrySerializerMethodField): | ||
warnings.warn( | ||
"Geometry generation for GeometrySerializerMethodField is not supported." | ||
) | ||
return {} | ||
|
||
model_field_name = geo_field_name | ||
|
||
geo_field = model_meta.get_field_info(serializer.Meta.model).fields[ | ||
model_field_name | ||
] | ||
try: | ||
return self.GEO_FIELD_TO_SCHEMA[geo_field.__class__] | ||
except KeyError: | ||
warnings.warn( | ||
"Geometry generation for {field} is not supported.".format(field=field) | ||
) | ||
return {} | ||
|
||
def map_field(self, field): | ||
if isinstance(field, GeoFeatureModelListSerializer): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks incorrect. Anyway, 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Line no 369 of |
||
return self._map_geo_feature_model_list_serializer(field) | ||
|
||
return super().map_field(field) | ||
|
||
def _map_geo_feature_model_list_serializer(self, serializer): | ||
return { | ||
"type": "object", | ||
"properties": { | ||
"type": {"type": "string", "enum": ["FeatureCollection"]}, | ||
"features": { | ||
"type": "array", | ||
"items": self.map_serializer(serializer.child), | ||
}, | ||
}, | ||
} | ||
|
||
def _map_geo_feature_model_serializer(self, serializer): | ||
schema = super().map_serializer(serializer) | ||
|
||
geo_json_schema = { | ||
"type": "object", | ||
"properties": {"type": {"type": "string", "enum": ["Feature"]}}, | ||
} | ||
|
||
if serializer.Meta.id_field: | ||
geo_json_schema["properties"]["id"] = schema["properties"].pop( | ||
serializer.Meta.id_field | ||
) | ||
|
||
geo_field = serializer.Meta.geo_field | ||
geo_json_schema["properties"]["geometry"] = { | ||
"type": "object", | ||
"properties": self._map_geo_field(serializer, geo_field), | ||
} | ||
schema["properties"].pop(geo_field) | ||
|
||
if serializer.Meta.auto_bbox or serializer.Meta.bbox_geo_field: | ||
geo_json_schema["properties"]["bbox"] = { | ||
"type": "array", | ||
"items": {"type": "number"}, | ||
"minItems": 4, | ||
"maxItems": 4, | ||
"example": [12.9721, 77.5933, 12.9721, 77.5933], | ||
} | ||
if serializer.Meta.bbox_geo_field in schema["properties"]: | ||
schema["properties"].pop(serializer.Meta.bbox_geo_field) | ||
|
||
geo_json_schema["properties"]["properties"] = schema | ||
|
||
return geo_json_schema | ||
|
||
def map_serializer(self, serializer): | ||
dhaval-mehta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if isinstance(serializer, GeoFeatureModelListSerializer): | ||
return self._map_geo_feature_model_list_serializer(serializer) | ||
|
||
if isinstance(serializer, GeoFeatureModelSerializer): | ||
return self._map_geo_feature_model_serializer(serializer) | ||
|
||
return super().map_serializer(serializer) |
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