-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes #20365: Fix schema and field definitions for OpenAPI #20407
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
Fixes #20365: Fix schema and field definitions for OpenAPI #20407
Conversation
netbox/core/api/views.py
Outdated
|
|
||
|
|
||
| class BaseRQViewSet(viewsets.ViewSet): | ||
| class BaseRQViewSet(viewsets.GenericViewSet): |
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 not sure it makes sense to employ GenericViewSet here as it adds methods such as get_queryset() which aren't applicable, as the view doesn't use a model. Could we instead just add declare a get_serializer_class() method?
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.
Thanks for the thoughtful review!
You’re absolutely right.
My initial pass was to add get_serializer_class(), but the custom get_serializer() also expects get_serializer_context(). To avoid re‑implementing both, I tried GenericViewSet as a shortcut, though I agree it broadens the API surface unnecessarily.
I’ll revert the base class to viewsets.ViewSet and add minimal helpers for the serializer class/context locally.
Thanks again for the guidance!
netbox/core/api/views.py
Outdated
| @extend_schema(responses={200: OpenApiTypes.OBJECT}) | ||
| @extend_schema( | ||
| operation_id='core_background_tasks_retrieve_by_id', | ||
| parameters=[OpenApiParameter(name='pk', type=OpenApiTypes.STR, location=OpenApiParameter.PATH)], |
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.
Is this redundant to line 192 above? (Also, here we call the parameter pk but above it's named id.)
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.
Thanks for catching that!
You’re right on both points: the second annotation is redundant with line 192, and the parameter name mismatch (pk vs id) is my oversight. I’ll remove the duplicate, and standardize on id across the view methods (and the serializer’s HyperlinkedIdentityField) to match the route kwarg and keep the OpenAPI output consistent.
Appreciate the careful review!
Add `get_internal_type()` to custom field classes for Django compatibility, annotate path parameters and operation IDs for background endpoints, and provide serializer context on the RQ base viewset to clear schema warnings. Fixes netbox-community#20365
ae274a8 to
ff29107
Compare
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.
Thanks @pheus!
Fixes: #20365
get_internal_type()on custom fields for Django introspection.id/namepath params and add explicitoperation_ids for queues/workers/tasksget_serializer_class()andget_serializer_context()on the RQ base viewset