Feature/openapi doc #26
Conversation
Add /schema and /redoc to generate docs on-the-fly. Add AutoSchema and tags for each viewset to properly categorize API endpoints. Update DRF to 3.12 which includes some enhancements for generating OpenAPI schema automatically. Add additional dependencies for autodocs and versioning of API docs.
…docs. Add some TODOs for missing descriptions. Add some limits on timezones and altitude.
…oints. By default, the DjangoFilterBackend uses the field name for the description in the OpenAPI schema. For clarity, allow these to be overridden at the ViewSet level by providing some basic annotations and passing them to a CustomViewSchema, which will add them in when generating the filter parameters.
Add doc generation endpoints to top-level urls, as it's not hardware-specific. Add versioning support to auto-docs.
By default github actions checkout shallow clones a repo, which does not include much of the git metadata that setuptools-scm needs to generate version numbers and work correctly. Also add a better description for the head of the API docs.
configdb/hardware/models.py
Outdated
| timezone = models.IntegerField() | ||
| timezone = models.IntegerField( | ||
| help_text='Offset from UTC in hours', | ||
| validators=[MinValueValidator(-12), MaxValueValidator(12)] |
There was a problem hiding this comment.
-12 to +12 seemed like the correct range?
There was a problem hiding this comment.
yeah that seems appropriate
There was a problem hiding this comment.
Actually surprisingly it looks like the range is -12 to +14 https://en.wikipedia.org/wiki/List_of_UTC_time_offsets This article has some timezone fun facts and it shows locations that go beyond +12 https://jakubmarian.com/5-weird-things-you-didnt-know-about-time-zones/
There was a problem hiding this comment.
Unless this timezone field is not meant to represent real timezones but rather be more representative of longitude? Not sure we actually use this field anywhere though
configdb/hardware/models.py
Outdated
| lat = models.FloatField(default=0.0, help_text='Site latitude in decimal degrees') | ||
| long = models.FloatField(default=0.0, help_text='Site longitude in decimal degrees') | ||
| elevation = models.IntegerField( | ||
| validators=[MinValueValidator(0), MaxValueValidator(100000)], |
There was a problem hiding this comment.
Made this 0-100,000. Can change it when we put a telescope on the moon, I guess?
There was a problem hiding this comment.
This might not even be worth saying since generally telescopes are placed at higher elevation... but technically there are places on Earth that are below 0 meters in elevation https://en.wikipedia.org/wiki/List_of_places_on_land_with_elevations_below_sea_level The lowest is the Dead Sea, at about -430 meters.
There was a problem hiding this comment.
Some day I'll take a telescope + OCS to badwater basin in death valley (-86m) 😉 https://en.wikipedia.org/wiki/Death_Valley
Maybe we should give some wiggle room, just to cover the extents of the terrestrial earth
configdb/hardware/api_views.py
Outdated
| class InstrumentViewSet(FilterableViewSet): | ||
| custom_filter_annotations=[{'name': 'science_cameras', 'description': 'Set of science camera codes on the instrument', | ||
| 'type': 'Array of strings'}, | ||
| {'name': 'autoguider_camera', 'description': 'Autoguider code for the autoguider camera on the instrument'}] |
There was a problem hiding this comment.
This is an example where the InstrumentFilter includes a couple of fields (science_cameras, autoguider_camera) in the fields Meta attribute of the filterset, but doesn't include them as explicit filters. Here they are being overridden with some better descriptions and type annotations, and then passed to the schema generator class to override what DRF does by default.
jnation3406
left a comment
jnation3406
left a comment
There was a problem hiding this comment.
looks good, just a few comments. I think I'll have to see if I can figure out how to import the generated .html into a .md file in the github pages repo. Otherwise maybe we'll need to add a step to the github workflow to move the .html into a .md and append some text to the beginning of the file.
configdb/hardware/api_views.py
Outdated
| """ | ||
| def __init__(self, custom_filter_annotations=None, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.custom_filter_annotations = custom_filter_annotations |
There was a problem hiding this comment.
I think there might be a way to do this without passing in the field name/description pairs. You said for the meta fields that they would end up with no description, so could we just iterate over the parameters, and any without a description, we check that fields name against the model and use its help_text for the description if that field exists in the model? That way you override this get_filter_parameters method still, but don't need to duplicate the help text or pass anything into this schema generator?
There was a problem hiding this comment.
Yep good idea. They end up with the description simply being the meta field name, so we can check on that. That's much cleaner.
configdb/hardware/models.py
Outdated
| timezone = models.IntegerField() | ||
| timezone = models.IntegerField( | ||
| help_text='Offset from UTC in hours', | ||
| validators=[MinValueValidator(-12), MaxValueValidator(12)] |
There was a problem hiding this comment.
yeah that seems appropriate
configdb/hardware/models.py
Outdated
| lat = models.FloatField(default=0.0, help_text='Site latitude in decimal degrees') | ||
| long = models.FloatField(default=0.0, help_text='Site longitude in decimal degrees') | ||
| elevation = models.IntegerField( | ||
| validators=[MinValueValidator(0), MaxValueValidator(100000)], |
There was a problem hiding this comment.
what if aliens on non-earth planets want to use the OCS?
| return self.code | ||
|
|
||
|
|
||
| class ConfigurationTypeProperties(BaseModel): |
There was a problem hiding this comment.
should add a verbose_name_plural to this one as well, it's current plural has two "s" at the end
configdb/hardware/models.py
Outdated
|
|
||
| class ModeType(BaseModel): | ||
| id = models.CharField(max_length=200, primary_key=True) | ||
| id = models.CharField(max_length=200, primary_key=True, help_text='Mode type ID') |
There was a problem hiding this comment.
This is technically a string name/code of the mode type. It is intended to be a "code" or "id" I guess in that we don't want spaces in it. Maybe just call it the "Mode type" rather than "Mode type ID"?
configdb/hardware/serializers.py
Outdated
| camera_type_id = serializers.IntegerField(write_only=True) | ||
| optical_element_groups = OpticalElementGroupSerializer(many=True, read_only=True) | ||
| camera_type = CameraTypeSerializer(read_only=True, help_text='Camera type') | ||
| camera_type_id = serializers.IntegerField(write_only=True, help_text='Unique ID that corresponds to this camera\'s type') |
There was a problem hiding this comment.
This field is the db model id for the camera_type (so a number, not a string)
configdb/hardware/serializers.py
Outdated
| state = StateField() | ||
| autoguider_camera = CameraSerializer(read_only=True, help_text='Autoguider camera for this instrument') | ||
| autoguider_camera_id = serializers.IntegerField(write_only=True, | ||
| help_text='Unique ID for the autoguider camera belonging to this instrument') |
There was a problem hiding this comment.
Same as above, this is the database model id number
configdb/hardware/serializers.py
Outdated
| help_text='Science cameras that belong to this instrument') | ||
| science_cameras_ids = serializers.PrimaryKeyRelatedField(write_only=True, many=True, | ||
| queryset=Camera.objects.all(), source='science_cameras', | ||
| help_text='Unique IDs for the science cameras belonging to this instrument') |
configdb/hardware/serializers.py
Outdated
| telescope = serializers.HyperlinkedRelatedField(view_name='telescope-detail', read_only=True, | ||
| help_text='Telescope this instrument belongs to') | ||
| telescope_id = serializers.IntegerField(write_only=True, | ||
| help_text='Unique ID for the telescope that this instrument belongs to') |
configdb/hardware/serializers.py
Outdated
| queryset=Camera.objects.all(), source='science_cameras', | ||
| help_text='Unique IDs for the science cameras belonging to this instrument') | ||
| instrument_type = InstrumentTypeSerializer(read_only=True, help_text='Instrument type') | ||
| instrument_type_id = serializers.IntegerField(write_only=True, help_text='Unique ID for the instrument type of this instrument') |
…iated model. By default, DRF simply substitutes the field name in for the description if the filter field isn't explicitly defined in the Filter.
…stem/configdb into feature/openapi-doc
This adds autogenerating docs to the ConfigDB project.
Most of the code changes are to better annotate the auto-generated docs. I'll add some comments to illustrate this.
Basically, the deal is:
GET endpoints' query parameters are driven by the filter fields, as expected. If there are any filters that aren't explicitly listed in the FilterSet but are present in the Meta
fieldsattribute, these will be documented but the annotations are not very useful - and it will not be auto-populated by the model attribute's help text, even if the field corresponds directly to a model field. Frustrating! I added a mechanism for us to override these by way of a custom Schema generator. Basically, list out all of your fields that DRF isn't annotating to your liking in the view, pass this to the the schema generator, and the default annotations will be overridden if the field names match what's in your list. I'll add some comments in the PR to illustrate how this works.POST/PUT/PATCH/etc... docs are driven primarily by serializers as you'd expect. If there are other fields not listed in the serializer explicitly but included in the Meta fields attribute, their help text will be populated from the model's help text. So I've added help text to all of the serializers and all of the models, just to be consistent and cover our bases.
Random thing I found - in the admin interface the
InstrumentCategorymodel is pluralized toInstrumentCategorys. Addingverbose_name_plural = "Instrument categories"to the InstrumentCategory's Meta class can override that - this change is live at http://configdb-dev.lco.gtn/admin/'Also added some min, max values to some of the serializers. In the docs, the integer fields would default their range from -MAXINT to +MAXINT which is silly. Figured we could validate on some of these. I'll point these out in some comments
My main concern is that these are annotated accurately and correctly. Please correct me if any of them are way off-base!
The latest build of the documentation is live on my test github pages site, https://mgdaily.github.io/ocs-gh-pages/docs/configdb.html