Open
Conversation
Contributor
Reviewer's GuideIntroduces a centralized mechanism for resolving plugin serializers, adding a built‑in default serializer for djangocms_alias.AliasPlugin, reusing the existing RESTRenderer/plugin serialization pipeline, and refactoring generic auto‑serializer creation and minor formatting. Sequence diagram for AliasPlugin serialization via RESTRenderersequenceDiagram
actor Client
participant RESTRenderer
participant Request
participant CMSPlugin as CMSPluginInstance
participant AliasPlugin as AliasPluginInstance
participant Resolver as resolve_plugin_serializer
participant AutoSer as get_auto_model_serializer
participant Serializer as AliasInlineSerializer
participant RendererAlias as RESTRendererAliasContent
Client->>RESTRenderer: serialize_plugins(placeholder, language, context)
RESTRenderer->>Request: attach request to context
loop For each plugin
RESTRenderer->>CMSPlugin: serialize_cms_plugin(instance, context)
CMSPlugin-->>RESTRenderer: plugin_instance, plugin
RESTRenderer->>Resolver: resolve_plugin_serializer(plugin, AliasPlugin)
alt plugin.serializer_class is set
Resolver-->>RESTRenderer: plugin.serializer_class
else plugin.serializer_class not set
Resolver-->>RESTRenderer: AliasInlineSerializer (from overrides)
end
RESTRenderer->>Serializer: __init__(AliasPluginInstance, context)
RESTRenderer->>Serializer: data
Serializer->>Serializer: get_content(instance)
Serializer->>Request: read _rest_alias_stack
alt alias_id already in _rest_alias_stack
Serializer-->>RESTRenderer: content = []
else alias_id not in stack
Serializer->>Request: push alias_id to _rest_alias_stack
Serializer->>AliasPlugin: alias.get_placeholder(language, draft_flag)
alt placeholder exists
Serializer->>RendererAlias: RESTRenderer(request).serialize_plugins(placeholder, language, context)
RendererAlias-->>Serializer: serialized children
Serializer-->>RESTRenderer: content with nested plugins
else no placeholder
Serializer-->>RESTRenderer: content = []
end
Serializer->>Request: pop alias_id from _rest_alias_stack
end
RESTRenderer-->>Client: serialized plugin data (including alias content)
end
Updated class diagram for plugin serializers and AliasInlineSerializerclassDiagram
class GenericPluginSerializer {
}
class AliasPlugin {
}
class AliasInlineSerializer {
+content SerializerMethodField
+get_content(instance AliasPlugin) list~dict~
}
GenericPluginSerializer <|-- AliasInlineSerializer
AliasInlineSerializer ..> AliasPlugin
class RESTRenderer {
+serialize_plugins(placeholder, language, context) list
+serialize_placeholder(placeholder, context, language, use_cache)
}
RESTRenderer ..> AliasInlineSerializer : uses for alias content
class PluginSerializerResolution {
+get_plugin_serializer_overrides() dict~str, type~
+resolve_plugin_serializer(plugin, model_class) type
+get_auto_model_serializer(model_class) type
}
PluginSerializerResolution ..> GenericPluginSerializer
PluginSerializerResolution ..> AliasInlineSerializer
RESTRenderer ..> PluginSerializerResolution : uses for serializer resolution
AliasInlineSerializer ..> RESTRenderer : invokes for nested plugins
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+15
to
+18
| from djangocms_rest.serializers.plugins import ( | ||
| get_auto_model_serializer, | ||
| resolve_plugin_serializer, | ||
| ) |
Check notice
Code scanning / CodeQL
Cyclic import Note
| from djangocms_alias.models import AliasPlugin | ||
| from rest_framework import serializers | ||
|
|
||
| from djangocms_rest.serializers.plugins import GenericPluginSerializer, base_exclude |
Check notice
Code scanning / CodeQL
Cyclic import Note
| if not placeholder: | ||
| return [] | ||
|
|
||
| from djangocms_rest.plugin_rendering import RESTRenderer |
Check notice
Code scanning / CodeQL
Cyclic import Note
Collaborator
Author
There was a problem hiding this comment.
@fsbraun exist to break circular dependencies
| """Return built-in serializer overrides keyed by model label.""" | ||
| overrides: dict[str, type] = {} | ||
| if apps.is_installed("djangocms_alias"): | ||
| from djangocms_rest.serializers.alias import AliasInlineSerializer |
Check notice
Code scanning / CodeQL
Cyclic import Note
Collaborator
Author
There was a problem hiding this comment.
@fsbraun exist to break circular dependencies
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider caching the result of
get_plugin_serializer_overrides()(e.g. at module level or vialru_cache) so it isn’t recomputed on every serializer resolution whendjangocms_aliasis installed. - In
resolve_plugin_serializer, you’ve removed the previous behavior of caching the resolved serializer onplugin.__class__.serializer_class; if that was relied upon for performance, you might want to explicitly document or reintroduce a lightweight caching strategy for resolved serializers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider caching the result of `get_plugin_serializer_overrides()` (e.g. at module level or via `lru_cache`) so it isn’t recomputed on every serializer resolution when `djangocms_alias` is installed.
- In `resolve_plugin_serializer`, you’ve removed the previous behavior of caching the resolved serializer on `plugin.__class__.serializer_class`; if that was relied upon for performance, you might want to explicitly document or reintroduce a lightweight caching strategy for resolved serializers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 91.98% 92.40% +0.42%
==========================================
Files 19 20 +1
Lines 886 909 +23
Branches 100 100
==========================================
+ Hits 815 840 +25
+ Misses 44 43 -1
+ Partials 27 26 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
metaforx
changed the title
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
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.
fixes #5
I opted for a more integrated approach instead of overriding the serializer via the app init function in the apps.py file because I consider it to be cleaner. @fsbraun: Do you agree?
{ "content": [ { "plugin_type": "TextPlugin", "body": "<p>Ich bin ein Alias</p>", "json": { "type": "doc", "content": [ { "type": "paragraph", "attrs": { "textAlign": null }, "content": [ { "text": "Ich bin ein Alias", "type": "text" } ] } ] }, "rte": "tiptap" } ], "plugin_type": "Alias", "template": "default", "alias": "djangocms_alias.alias:1" }Summary by Sourcery
Add a default serializer for Alias plugins by integrating an alias-specific inline serializer into the generic plugin serialization pipeline and centralizing plugin serializer resolution.
New Features:
Enhancements: