refactor: add distinct operationId in openapi schema for menu endpoin… #80
+223
−108
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ts, separate schemas from views
Reviewer's GuideThis PR refactors the OpenAPI schema generation for menu-related endpoints by introducing a custom MenuSchema that generates unique operationIds, wrapping view registrations with create_view_with_url_name to tag URL names for schema derivation, decorating view classes with the new schema while removing inline schema decorators, and updating tests to use the newly named endpoints. Class diagram for new OpenAPI schema utilities (MenuSchema and helpers)classDiagram
class MenuSchema {
+get_operation_id()
}
class AutoSchema
MenuSchema --|> AutoSchema
class NavigationNodeSerializer
class ViewWithUrlName {
_url_name
}
class create_view_with_url_name {
+__call__(view_class, url_name)
}
class menu_schema_class {
+__call__(view_class)
}
class method_schema_decorator {
+__call__(method)
}
class extend_placeholder_schema
create_view_with_url_name ..> ViewWithUrlName : creates
menu_schema_class ..> MenuSchema : applies
method_schema_decorator ..> NavigationNodeSerializer : uses
Class diagram for updated MenuView, SubMenuView, and BreadcrumbView schema usageclassDiagram
class BaseAPIView
class MenuView {
permission_classes
serializer_class
tag
return_key
+get(request)
schema : MenuSchema
}
class SubMenuView {
tag
+populate_defaults(kwargs)
schema : MenuSchema
}
class BreadcrumbView {
tag
return_key
schema : MenuSchema
}
MenuView --|> BaseAPIView
SubMenuView --|> MenuView
BreadcrumbView --|> MenuView
MenuView ..> MenuSchema : uses
SubMenuView ..> MenuSchema : uses
BreadcrumbView ..> MenuSchema : uses
Flow diagram for view registration with URL name for schema generationflowchart TD
A["urls.py path registration"] --> B["create_view_with_url_name(view_class, url_name)"]
B --> C["ViewWithUrlName instance"]
C --> D["View registered with _url_name"]
D --> E["MenuSchema uses _url_name for operationId"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
- Coverage 91.23% 90.94% -0.30%
==========================================
Files 18 19 +1
Lines 844 872 +28
Branches 97 99 +2
==========================================
+ Hits 770 793 +23
- Misses 46 50 +4
- Partials 28 29 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider programmatically constructing the repetitive URL patterns and names (e.g., via a loop or helper) to reduce boilerplate and minimize risk of naming mismatches across menu endpoints.
- Add a test that introspects the generated OpenAPI schema and asserts the new operationId values so you can verify distinct IDs are applied as intended.
- In the ImportError fallback for create_view_with_url_name, you might still attach the
_url_namemetadata even when drf-spectacular isn’t installed to keep view metadata consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider programmatically constructing the repetitive URL patterns and names (e.g., via a loop or helper) to reduce boilerplate and minimize risk of naming mismatches across menu endpoints.
- Add a test that introspects the generated OpenAPI schema and asserts the new operationId values so you can verify distinct IDs are applied as intended.
- In the ImportError fallback for create_view_with_url_name, you might still attach the `_url_name` metadata even when drf-spectacular isn’t installed to keep view metadata consistent.
## Individual Comments
### Comment 1
<location> `tests/endpoints/test_menu.py:124` </location>
<code_context>
def test_non_existing_root_id(self):
url = reverse(
- "menu",
+ "menu-root-levels",
kwargs={
"language": "en",
+ "root_id": "I_DO_NOT_EXIST",
"from_level": 0,
"to_level": 100,
"extra_inactive": 0,
"extra_active": 100,
- "root_id": "I_DO_NOT_EXIST",
},
)
response = self.client.get(url)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions for the specific error response structure and status code.
Additionally, assert the error message or response structure to verify the API returns a meaningful error payload and to strengthen coverage for error handling.
```suggestion
response = self.client.get(url)
# Assert status code (assuming 404 for non-existing root_id)
assert response.status_code == 404
# Assert error response structure
data = response.json()
assert "detail" in data or "error" in data
# Assert error message is meaningful
if "detail" in data:
assert "not found" in data["detail"].lower() or "does not exist" in data["detail"].lower()
elif "error" in data:
assert "not found" in data["error"].lower() or "does not exist" in data["error"].lower()
```
</issue_to_address>
### Comment 2
<location> `djangocms_rest/schemas.py:3` </location>
<code_context>
+"""OpenAPI schema generation utilities for djangocms-rest."""
+
+try:
+ from drf_spectacular.openapi import AutoSchema
+ from drf_spectacular.types import OpenApiTypes
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the schema utilities to use a single import-time feature flag and unified conditional definitions for reduced duplication and clearer logic.
```python
# schemas.py
# 1) Single import‐time check instead of repeating try/except
try:
from drf_spectacular.openapi import AutoSchema
from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import (
OpenApiParameter, OpenApiResponse, extend_schema,
)
HAS_SPECTACULAR = True
except ImportError:
HAS_SPECTACULAR = False
# 2) Custom AutoSchema lives only if spectacular is installed
if HAS_SPECTACULAR:
class MenuSchema(AutoSchema):
"""
Uses a stored `_url_name` on the view to generate operation_id,
falling back to the parent impl if missing.
"""
def get_operation_id(self):
url_name = getattr(self.view, "_url_name", None) \
or getattr(self.view.__class__, "_url_name", None)
if url_name:
return url_name.replace("-", "_") + "_retrieve"
return super().get_operation_id()
# 3) Unified decorator/factory defs that noop when HAS_SPECTACULAR is False
def menu_schema_class(cls):
"""Attach MenuSchema() to a view class, or no‐op."""
if HAS_SPECTACULAR:
cls.schema = MenuSchema()
return cls
def method_schema_decorator(fn):
"""
Force many=True on NavigationNodeSerializer for list endpoints,
or no‐op if spectacular is absent.
"""
if HAS_SPECTACULAR:
from djangocms_rest.serializers.menus import NavigationNodeSerializer
response = OpenApiResponse(response=NavigationNodeSerializer(many=True))
return extend_schema(responses=response)(fn)
return fn
# 4) Single factory for as_view + url_name
def as_view_with_url_name(viewset, *args, url_name=None, **kwargs):
"""
Wrap as_view, store `_url_name` on the returned view for schema.
"""
view = viewset.as_view(*args, **kwargs)
if HAS_SPECTACULAR and url_name:
view._url_name = url_name
return view
# 5) Single placeholder decorator for shared parameters
if HAS_SPECTACULAR:
extend_placeholder_schema = extend_schema(
parameters=[
OpenApiParameter(
name="html",
type=OpenApiTypes.INT,
location="query",
description="Include HTML rendering (set to 1)",
required=False,
enum=[1],
),
OpenApiParameter(
name="preview",
type=OpenApiTypes.BOOL,
location="query",
description="Preview unpublished content",
required=False,
),
]
)
else:
extend_placeholder_schema = lambda fn: fn
```
Key reductions:
- One `HAS_SPECTACULAR` flag at module top instead of duplicated stubs.
- Conditional class and decorator definitions that either apply spectacular logic or noop.
- Merged `create_view_with_url_name` and decorator into a single `as_view_with_url_name`.
- No repeated `# pragma: no cover` blocks or multiple try/except layers.
</issue_to_address>
### Comment 3
<location> `djangocms_rest/schemas.py:25-27` </location>
<code_context>
def get_operation_id(self):
"""Override to use URL name stored in view as operation_id."""
try:
url_name = getattr(self.view, "_url_name", None)
if not url_name and hasattr(self.view, "__class__"):
url_name = getattr(self.view.__class__, "_url_name", None)
if url_name:
operation_id = url_name.replace("-", "_") + "_retrieve"
return operation_id
# Fallback to default
return super().get_operation_id()
except Exception:
return super().get_operation_id()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))
```suggestion
return url_name.replace("-", "_") + "_retrieve"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… drf-spectacular is unavailable
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
path,levels,root,nephewsfor all menu endpoints._retrireveto each operationId to align with the default naming pattern from OpenAPI.drf_spectacularis not availableResults in:
Summary by Sourcery
Generate distinct OpenAPI operationIds for menu-related endpoints by extracting schema utilities into schemas.py, wrapping views with URL-specific naming, and updating URL patterns and tests accordingly
Enhancements: