-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactoring for improved Open API & typing support (allows: automatic schema generation) #20
base: main
Are you sure you want to change the base?
Conversation
- add/fix/improve openapi support - add typing - fix serializer assignment which resulted in DRF error warnings with swagger - separated utils from views and serializers - disabled caching functionality (was not working reliably and i would rather cache api with redis directly) - remove absolute url from serializer, because i see no use in them (at least not for my projects)
Modularized serializers into distinct files for improved clarity and maintainability. Refactored placeholder caching, language, and page handling utilities into dedicated modules. Adjusted settings and base views to align with the new structure.
…w_page Needed because nested objects are also checked but are PageContent
should be added in parent/global urlpatterns
Reviewer's Guide by SourceryThis pull request refactors the REST API to enhance Open API support and improve type safety. The changes reorganize view classes, serializers, permissions, and utilities following common Django patterns. The refactoring introduces explicit serializer assignments, type hints, and modularized components to facilitate automatic schema generation and frontend type usage. Updated API Views Class DiagramclassDiagram
class BaseAPIView {
<<abstract>>
+get()
}
class LanguageListView {
+get(request: Request): Response
+@extend_schema
}
LanguageListView --|> BaseAPIView
class PageTreeListView {
+get(request, language): Response
+permission: IsAllowedLanguage
}
PageTreeListView --|> BaseAPIView
class PageDetailView {
+get(request: Request, language: str, path: str): Response
+permission: CanViewPage
}
PageDetailView --|> BaseAPIView
class PlaceholderDetailView {
+get(request: Request, language: str, content_type_id: int, object_id: int, slot: str): Response
+permission: CanViewPageContent
}
PlaceholderDetailView --|> BaseAPIView
Updated API Serializers Class DiagramclassDiagram
class BasePageSerializer {
+to_representation()
}
class PageMetaSerializer {
+to_representation()
+children: List
}
PageMetaSerializer --|> BasePageSerializer
class PageContentSerializer {
+to_representation()
}
PageContentSerializer --|> BasePageSerializer
class PlaceholderRenderer {
+render_placeholder(placeholder, context, language, use_cache)
+render_plugins(placeholder, language, context)
}
class PlaceholderSerializer {
+__init__(request: Request, placeholder, language: str)
}
PlaceholderSerializer ..> PlaceholderRenderer : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @metaforx - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a brief explanation of the purpose and structure of each serializer to improve maintainability.
- It would be helpful to include a section on error handling and how the API deals with invalid requests or data.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
language=language, | ||
use_cache=True, | ||
) | ||
if request.GET.get("html", False): |
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.
suggestion (bug_risk): Handling of the 'html' query parameter deserves closer attention.
Since GET parameters are strings, checking for 'html' using a truthy evaluation might be error‐prone. Consider normalizing the value (for example, comparing against 'true' case‐insensitively) to ensure consistent behavior.
if request.GET.get("html", False): | |
if request.GET.get("html", "").strip().lower() == "true": |
as_tree is not integrated, could pages-list be printed flat when as_tree = false? @classmethod63 | def many_init(cls, request, instances, *args, **kwargs): |
- Remove drf-spectacular import and extend_schema decorator
An opinionated refactoring of the REST API, not adding functionality beside improved Open API support. Allows automated schema generation using tools like
openapi-typescript-codegen
. This allows automatic use of types in frontend using Typescript.Improved readability, but that's really a personal preference.
self.request = self.context.get("request")
API-Schema & Documentation
https://github.com/tfranzel/drf-spectacular
Follow installation guidelines on GitHub, only a few instructions have to be added to settings.py
Use in Frontend
Use Automatic Endpoints from Schema
https://nuxt-open-fetch.vercel.app/ (Nuxt specific)
Validate Data from Schema/Types
https://zod.dev/
Needs: What's missing & Bugs
-Error [PlaceholderDetailView]: unable to guess serializer.
This happened to all views when using in combination with drf-spectacular. I could fix this for all other views by refactoring init, but this particular one covers a lot of specific functionality which I am not sure, what's the best approach. To get rid of this error, we need to add@extend_schema( responses=PlaceholderSerializer", )
with reworked init functionality.Summary by Sourcery
New Features: