feat: Add RESTRenderer #42
Open
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
Reviewer's GuideThis PR implements a new RESTRenderer to provide a JSON-based edit view for CMS plugin content, integrates it into the toolbar via a custom CMSConfig, enhances plugin serializers to handle foreign keys and nested JSON fields, refactors placeholder and page serializers for JSON rendering and HTML toggling, and updates tests and settings to validate the new functionality. Class diagram for RESTRenderer and related plugin rendering classesclassDiagram
class ContentRenderer {
}
class RESTRenderer {
+render_plugin(instance, context, placeholder, editable)
+render_plugins(placeholder, language, context, editable, template)
+serialize_placeholder(placeholder, context, language, use_cache)
+serialize_plugins(placeholder, language, context)
+get_plugins_and_placeholder_lot(placeholder, language, context, editable, template)
}
class GenericPluginSerializer {
+to_representation(instance)
}
class PlaceholderSerializer {
+__init__(...)
}
ContentRenderer <|-- RESTRenderer
GenericPluginSerializer <|-- get_auto_model_serializer
RESTRenderer o-- PlaceholderSerializer
RESTRenderer o-- GenericPluginSerializer
RESTRenderer o-- ContentRenderer
Class diagram for RESTToolbarMixin and RESTCMSConfig integrationclassDiagram
class RESTToolbarMixin {
+content_renderer
}
class RESTCMSConfig {
+cms_enabled
+cms_toolbar_mixin
}
RESTCMSConfig o-- RESTToolbarMixin
Class diagram for enhanced plugin serialization (serialize_fk, serialize_soft_refs)classDiagram
class serialize_fk {
+serialize_fk(request, related_model, pk, obj)
}
class serialize_soft_refs {
+serialize_soft_refs(request, data)
}
class GenericPluginSerializer {
+to_representation(instance)
}
GenericPluginSerializer o-- serialize_fk
GenericPluginSerializer o-- serialize_soft_refs
File-Level Changes
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 #42 +/- ##
==========================================
+ Coverage 91.04% 91.56% +0.52%
==========================================
Files 12 16 +4
Lines 480 688 +208
Branches 42 77 +35
==========================================
+ Hits 437 630 +193
- Misses 26 37 +11
- Partials 17 21 +4 ☔ 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 @fsbraun - I've reviewed your changes - here's some feedback:
- You’re generating PLUGIN_DEFINITIONS twice (once in serializers/plugins.py and again in views.py)—consolidate to a single source to avoid divergence.
- get_auto_model_serializer creates a new serializer class on every call—add caching (e.g. via functools.lru_cache) to reuse generated classes and reduce overhead.
- serialize_soft_refs mutates its input in place and handles many cases; consider refactoring into smaller pure functions or returning new data structures to reduce side‐effects and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’re generating PLUGIN_DEFINITIONS twice (once in serializers/plugins.py and again in views.py)—consolidate to a single source to avoid divergence.
- get_auto_model_serializer creates a new serializer class on every call—add caching (e.g. via functools.lru_cache) to reuse generated classes and reduce overhead.
- serialize_soft_refs mutates its input in place and handles many cases; consider refactoring into smaller pure functions or returning new data structures to reduce side‐effects and improve readability.
## Individual Comments
### Comment 1
<location> `djangocms_rest/serializers/plugins.py:40` </location>
<code_context>
+ dict[str, Any]: A dictionary representing the serialized foreign key, typically as a URL or identifier.
+ """
+ # First choice: Check for get_api_endpoint method
+ if hasattr(related_model, "get_api_endpoint"):
+ if obj is None:
+ obj = related_model.objects.filter(pk=pk).first()
</code_context>
<issue_to_address>
Potential performance issue with database query in serialize_fk.
This approach can cause N+1 query issues if serialize_fk is used in a loop. To improve performance, try to pass the object when possible or prefetch related objects in advance.
</issue_to_address>
### Comment 2
<location> `djangocms_rest/serializers/plugins.py:76` </location>
<code_context>
+ """
+ if isinstance(data, list):
+ return [serialize_soft_refs(request, item) for item in data]
+ for key, value in data.items():
+ if isinstance(value, dict) and set(value.keys()) == {"model", "pk"}:
+ app_name, model_name = value["model"].split(".", 1)
</code_context>
<issue_to_address>
Possible mutation of input data in serialize_soft_refs.
Mutating the input dictionary may cause unintended side effects if it's used elsewhere. Please copy the dictionary before modifying, or clearly document this behavior.
</issue_to_address>
### Comment 3
<location> `djangocms_rest/serializers/plugins.py:123` </location>
<code_context>
+ super().__init__(*args, **kwargs)
+ self.request = self.context.get("request", None)
+
+ def to_representation(self, instance: CMSPlugin):
+ request = getattr(self, "request", None)
+
</code_context>
<issue_to_address>
Potential issue with JSON_FIELDS type checking.
Since 'field' is a model field instance and JSON_FIELDS contains serializer field classes, this type check may always fail. Use the appropriate tuple of model field classes or inspect the serializer field mapping for accurate type checking.
</issue_to_address>
### Comment 4
<location> `djangocms_rest/serializers/pages.py:19` </location>
<code_context>
redirect = serializers.CharField(max_length=2048, allow_null=True)
absolute_url = serializers.URLField(max_length=200, allow_blank=True)
path = serializers.CharField(max_length=200)
+ details = serializers.CharField(max_length=200, allow_blank=True)
is_home = serializers.BooleanField()
login_required = serializers.BooleanField()
</code_context>
<issue_to_address>
The 'details' field is limited to 200 characters.
Since API endpoint URLs can be longer than 200 characters, increasing the max_length for 'details' would help prevent data truncation.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
details = serializers.CharField(max_length=200, allow_blank=True)
=======
details = serializers.CharField(max_length=2048, allow_blank=True)
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `djangocms_rest/plugin_rendering.py:23` </location>
<code_context>
+ModelType = TypeVar("ModelType", bound=models.Model)
+
+
+def get_auto_model_serializer(model_class: type[ModelType]) -> type:
+ """
+ Build (once) a generic ModelSerializer subclass that excludes
</code_context>
<issue_to_address>
Potential for serializer class cache growth.
Repeated calls for the same model will create duplicate serializer classes and increase memory usage. Caching serializer classes per model is recommended.
Suggested implementation:
```python
ModelType = TypeVar("ModelType", bound=models.Model)
# Cache for auto-generated serializer classes per model
_auto_model_serializer_cache: dict[type[ModelType], type] = {}
def get_auto_model_serializer(model_class: type[ModelType]) -> type:
```
```python
def get_auto_model_serializer(model_class: type[ModelType]) -> type:
"""
Build (once) a generic ModelSerializer subclass that excludes
common CMS bookkeeping fields.
Uses a cache to avoid duplicate serializer classes per model.
"""
if model_class in _auto_model_serializer_cache:
return _auto_model_serializer_cache[model_class]
opts = model_class._meta
real_fields = {f.name for f in opts.get_fields()}
exclude = tuple(base_exclude & real_fields)
meta_class = type(
"Meta",
(),
```
```python
serializer_class = type(
f"{model_class.__name__}AutoSerializer",
(serializers.ModelSerializer,),
{"Meta": meta_class},
)
_auto_model_serializer_cache[model_class] = serializer_class
return serializer_class
```
</issue_to_address>
### Comment 6
<location> `tests/test_fk_serializer.py:44` </location>
<code_context>
+ finally:
+ del Pizza.get_api_endpoint
+
+ def test_serialize_soft_refs(self):
+ request = self.get_request(reverse("page-root", kwargs={"language": "en"}))
+
+ pk = Pizza.objects.create(description="Delicious pizza").pk
+
+ # Serialize a single soft reference
+ fk = serialize_soft_refs(
+ request, dict(ref={"model": "test_app.pizza", "pk": pk})
+ )
+ self.assertEqual(fk, {"ref": f"http://testserver/api/pizza/{pk}/"})
+
+ fk = serialize_soft_refs(
+ request, dict(link={"internal_link": f"test_app.pizza:{pk}"})
+ )
+ self.assertEqual(fk, {"link": f"http://testserver/api/pizza/{pk}/"})
+
+ fk = serialize_soft_refs(
+ request, dict(attrs={"data-cms-href": f"test_app.pizza:{pk}"})
+ )
+ self.assertEqual(
+ fk, {"attrs": {"data-cms-href": f"http://testserver/api/pizza/{pk}/"}}
+ )
</code_context>
<issue_to_address>
Soft reference serialization is well-tested.
Please add a test case for a list of soft references to verify recursive handling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_serialize_soft_refs(self):
request = self.get_request(reverse("page-root", kwargs={"language": "en"}))
pk = Pizza.objects.create(description="Delicious pizza").pk
# Serialize a single soft reference
fk = serialize_soft_refs(
request, dict(ref={"model": "test_app.pizza", "pk": pk})
)
self.assertEqual(fk, {"ref": f"http://testserver/api/pizza/{pk}/"})
fk = serialize_soft_refs(
request, dict(link={"internal_link": f"test_app.pizza:{pk}"})
)
self.assertEqual(fk, {"link": f"http://testserver/api/pizza/{pk}/"})
fk = serialize_soft_refs(
request, dict(attrs={"data-cms-href": f"test_app.pizza:{pk}"})
)
self.assertEqual(
fk, {"attrs": {"data-cms-href": f"http://testserver/api/pizza/{pk}/"}}
)
=======
def test_serialize_soft_refs(self):
request = self.get_request(reverse("page-root", kwargs={"language": "en"}))
pk = Pizza.objects.create(description="Delicious pizza").pk
# Serialize a single soft reference
fk = serialize_soft_refs(
request, dict(ref={"model": "test_app.pizza", "pk": pk})
)
self.assertEqual(fk, {"ref": f"http://testserver/api/pizza/{pk}/"})
fk = serialize_soft_refs(
request, dict(link={"internal_link": f"test_app.pizza:{pk}"})
)
self.assertEqual(fk, {"link": f"http://testserver/api/pizza/{pk}/"})
fk = serialize_soft_refs(
request, dict(attrs={"data-cms-href": f"test_app.pizza:{pk}"})
)
self.assertEqual(
fk, {"attrs": {"data-cms-href": f"http://testserver/api/pizza/{pk}/"}}
)
def test_serialize_soft_refs_list(self):
request = self.get_request(reverse("page-root", kwargs={"language": "en"}))
pk1 = Pizza.objects.create(description="Pizza 1").pk
pk2 = Pizza.objects.create(description="Pizza 2").pk
# List of soft references
refs = [
{"ref": {"model": "test_app.pizza", "pk": pk1}},
{"ref": {"model": "test_app.pizza", "pk": pk2}},
{"link": {"internal_link": f"test_app.pizza:{pk1}"}},
{"attrs": {"data-cms-href": f"test_app.pizza:{pk2}"}},
]
serialized = serialize_soft_refs(request, refs)
self.assertEqual(
serialized,
[
{"ref": f"http://testserver/api/pizza/{pk1}/"},
{"ref": f"http://testserver/api/pizza/{pk2}/"},
{"link": f"http://testserver/api/pizza/{pk1}/"},
{"attrs": {"data-cms-href": f"http://testserver/api/pizza/{pk2}/"}},
],
)
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `djangocms_rest/serializers/plugins.py:60` </location>
<code_context>
+ return f"{app_name}.{model_name}:{pk}"
+
+
+def serialize_soft_refs(request: HttpRequest, data: Any) -> Any:
+ """
+ Serialize soft references in a dictionary or list.
</code_context>
<issue_to_address>
Consider refactoring the foreign-key reference handling into a single helper function and a simplified loop to avoid repeated parsing logic.
```markdown
You’ve added a lot of special-case logic to walk and rewrite foreign-key references. You can collapse most of it into two small helpers—a single “extract model+pk” function, and a simple loop in your serializer—so you don’t repeat `.split()`, `get_model()`, etc. For example:
```python
# utils.py
from django.apps import apps
from typing import Any, Optional, Tuple, Type
from django.db.models import Model
def extract_ref(data: dict[str, Any]) -> Optional[Tuple[Type[Model], Any]]:
"""
Recognize any of:
{"model": "app.Model", "pk": "..."}
{"internal_link": "app.Model:..."}
{"file_link": "..."} (assume filer.File)
and return (ModelClass, pk) or None.
"""
if "model" in data and "pk" in data:
app_label, model_name = data["model"].split(".", 1)
return apps.get_model(app_label, model_name), data["pk"]
if "internal_link" in data:
model_label, pk = data["internal_link"].split(":", 1)
app_label, model_name = model_label.split(".", 1)
return apps.get_model(app_label, model_name), pk
if "file_link" in data:
# hard-code filer.File once
return apps.get_model("filer", "file"), data["file_link"]
return None
```
```python
# in your serializer module
from .utils import extract_ref
def serialize_soft_refs(request, obj):
if isinstance(obj, list):
return [serialize_soft_refs(request, item) for item in obj]
if not isinstance(obj, dict):
return obj
for key, val in list(obj.items()):
# catch attrs.data-cms-href
if key == "attrs" and isinstance(val, dict) and "data-cms-href" in val:
obj["attrs"]["data-cms-href"] = serialize_fk(
request, *extract_ref({"internal_link": val["data-cms-href"]})
)
continue
# generic model/pk or internal/file link
ref = extract_ref(val) if isinstance(val, dict) else None
if ref:
obj[key] = serialize_fk(request, *ref)
elif isinstance(val, (dict, list)):
obj[key] = serialize_soft_refs(request, val)
return obj
```
Benefits:
- One place to adjust your model+pk parsing.
- No more four separate `if isinstance(value, dict)…` blocks.
- Easier to unit‐test `extract_ref()` in isolation.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| dict[str, Any]: A dictionary representing the serialized foreign key, typically as a URL or identifier. | ||
| """ | ||
| # First choice: Check for get_api_endpoint method | ||
| if hasattr(related_model, "get_api_endpoint"): |
There was a problem hiding this comment.
issue (performance): Potential performance issue with database query in serialize_fk.
This approach can cause N+1 query issues if serialize_fk is used in a loop. To improve performance, try to pass the object when possible or prefetch related objects in advance.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@fsbraun : This looks good to me. I tested this with nested foreign key plugins with a custom serializer for django-imagefield. The output is fine in JSON (CMS) and on the placeholders API endpoint. What is not yet working as expected is the plugins definition API endpoint I would expect something like this: [
{
"plugin_type": "TextPlugin",
"name": "Text",
"type": "object",
"properties": {
"plugin_type": {
"type": "string"
},
"body": {
"type": "string"
},
"json": {
"type": "object"
},
"rte": {
"type": "string",
"description": "The rich text editor used to create this text. JSON formats vary between editors."
}
}
},
{
"plugin_type": "MediaImageDisplayPlugin",
"name": "Media Image Display",
"type": "object",
"properties": {
"image": {
"type": "object",
"properties": {
"public_id": {
"type": "string",
"format": "uuid"
},
"name": {
"type": "string",
"description": "Falls nicht gesetzt wird der Dateiname verwendet."
},
"description": {
"type": "string"
},
"alt": {
"type": "string",
"description": "Alternative Textbeschreibung für das Bild (für Barrierefreiheit)"
},
"image_width": {
"type": "integer"
},
"image_height": {
"type": "integer"
},
"image_ppoi": {
"type": "string"
},
"absolute_url": {
"type": "string"
}
}
},
"plugin_type": {
"type": "string"
}
}
},
{
"plugin_type": "MediaTeaserPlugin",
"name": "Teaser",
"type": "object",
"properties": {
"plugin_type": {
"type": "string"
}
}
}
]But i get [
{
"plugin_type": "TextPlugin",
"title": "Text",
"type": "object",
"properties": {
"body": {
"type": "string",
"description": ""
},
"json": {
"type": "object",
"description": ""
},
"rte": {
"type": "string",
"description": "The rich text editor used to create this text. JSON formats vary between editors."
},
"plugin_type": {
"type": "string",
"const": "TextPlugin",
"description": "Plugin identifier"
}
}
},
{
"plugin_type": "MediaImageDisplayPlugin",
"title": "Media Image Display",
"type": "object",
"properties": {
"plugin_type": {
"type": "string",
"const": "MediaImageDisplayPlugin",
"description": "Plugin identifier"
}
}
},
{
"plugin_type": "MediaTeaserPlugin",
"title": "Teaser",
"type": "object",
"properties": {
"plugin_type": {
"type": "string",
"const": "MediaTeaserPlugin",
"description": "Plugin identifier"
}
}
}
]It only returns the object but not the serialized object fields which are needed to create proper types to set typed frontend rendering. this might be because of the following code fragments which bind it to DRF serializer and field mapping: serializer_instance = serializer_cls()
for field_name, field in serializer_instance.fields.items():
if hasattr(field, "fields"):
for nested_field_name, nested_field in field.fields.items():
"UUIDField": {"type": "string", "format": "uuid"},
"ChoiceField": {"enum": ...}
serializer_cls = getattr(plugin, "serializer_class", None)POC implementation from rest_framework import serializers
from cms.plugin_pool import plugin_pool
from typing import Dict, Any
# CMS internal fields to exclude from all plugin schemas
EXCLUDE_CMS_PLUGIN_FIELDS = {
"cmsplugin_ptr",
"id",
"parent",
"creation_date",
"changed_date",
"position",
"language",
"placeholder",
}
def map_field_to_schema(field: serializers.Field, field_name: str = "") -> dict:
"""
Map DRF field to simple JSON Schema definition for rendering.
Args:
field: DRF serializer field instance
field_name: Name of the field (unused but kept for compatibility)
Returns:
dict: Basic JSON Schema definition for the field for TypeScript compatibility
"""
# Field type mapping for TypeScript compatibility
field_mapping = {
"CharField": {"type": "string"},
"TextField": {"type": "string"},
"URLField": {"type": "string", "format": "uri"},
"EmailField": {"type": "string", "format": "email"},
"IntegerField": {"type": "integer"},
"FloatField": {"type": "number"},
"DecimalField": {"type": "number"},
"BooleanField": {"type": "boolean"},
"DateField": {"type": "string", "format": "date"},
"DateTimeField": {"type": "string", "format": "date-time"},
"TimeField": {"type": "string", "format": "time"},
"FileField": {"type": "string", "format": "uri"},
"ImageField": {"type": "string", "format": "uri"},
"JSONField": {"type": "object"},
"ForeignKey": {"type": "integer"},
"PrimaryKeyRelatedField": {"type": "integer"},
"ListField": {"type": "array"},
"DictField": {"type": "object"},
"UUIDField": {"type": "string", "format": "uuid"},
}
# Handle special cases first
if isinstance(field, serializers.ChoiceField):
schema = {"type": "string", "enum": list(field.choices.keys())}
elif hasattr(field, "fields"): # Nested serializer
schema = {"type": "object"}
# Extract nested properties
properties = {}
for nested_field_name, nested_field in field.fields.items():
properties[nested_field_name] = map_field_to_schema(nested_field, nested_field_name)
if properties:
schema["properties"] = properties
else:
# Use mapping or default to string
schema = field_mapping.get(field.__class__.__name__, {"type": "string"})
# Description from help_text
if getattr(field, "help_text", None):
schema["description"] = str(field.help_text)
return schema
def generate_plugin_definitions() -> Dict[str, Any]:
"""
Generate simple plugin definitions for rendering.
"""
definitions = {}
for plugin in plugin_pool.get_all_plugins():
# Use plugin's serializer_class or create a simple fallback
serializer_cls = getattr(plugin, "serializer_class", None)
if not serializer_cls:
class DynamicModelSerializer(serializers.ModelSerializer):
class Meta:
model = plugin.model
fields = "__all__"
serializer_cls = DynamicModelSerializer
try:
serializer_instance = serializer_cls()
properties = {}
for field_name, field in serializer_instance.fields.items():
# Skip internal CMS fields
if field_name in EXCLUDE_CMS_PLUGIN_FIELDS:
continue
properties[field_name] = map_field_to_schema(field, field_name)
definitions[plugin.__name__] = {
"name": getattr(plugin, "name", plugin.__name__),
"type": "object",
"properties": properties,
}
except Exception:
# Skip plugins that fail to process
continue
return definitionsDoes this point you to a solution for the problem? |
|
@metaforx I've just pushed an update. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR adds an edit view that show the generated JSON structure by placeholder as opposed to the rendered HTML.
Needs test
Summary by Sourcery
Add a proof-of-concept RESTRenderer that outputs root-level plugin data as pretty-printed JSON and plugs into the CMS toolbar via a custom config
New Features:
Enhancements:
Summary by Sourcery
Add a RESTRenderer and supporting serializer utilities to render and interact with CMS content as JSON in the edit view, integrate it into the CMS toolbar, and back it with comprehensive endpoint and integration tests
New Features:
Enhancements:
Tests: