Repository: django-cms/django-cms
Pull request: fix: child restrictions cached despite children have cache_parent_classes set to False
## Description

This PR fixes #8180. 

This seems to be a long-standing flaw in caching plugin restrictions. The fix is necessary for django CMS 5, since plugin restrictions are now cached globally (i.e. not on a per request-basis) and a wrong cache is easily noticeable. It should also  be ported back to django CMS 4.1 and 3.11 imho.


## Related resources

<!--
Add here links to existing issues or conversation from GitHub
or any other resource.
-->

* #8180 
* #4939 

## Checklist

<!--
Please check the following items before submitting, otherwise,
your pull request will be closed.

Use 'x' to check each item: [x] I have ...
-->

* [x] I have opened this pull request against ``main``
* [ ] I have added or modified the tests when changing logic
* [x] I have followed [the conventional commits guidelines](https://www.conventionalcommits.org/) to add meaningful information into the changelog
* [x] I have read the [contribution guidelines ](https://github.com/django-cms/django-cms/blob/develop/CONTRIBUTING.rst) and I have joined the channel #pr-reviews on our [Discord Server](https://discord-pr-review-channel.django-cms.org) to find a “pr review buddy” who is going to review my pull request.

## Summary by Sourcery

Bug Fixes:
- Fixes an issue where child restrictions were being cached even when children had `cache_parent_classes` set to `False`.

## Summary by Sourcery

Fixes an issue where child restrictions were being cached even when children had `cache_parent_classes` set to `False`. This ensures that plugin restrictions are correctly evaluated, especially in environments where caching is more aggressive.

Bug Fixes:
- Fixes an issue where child restrictions were being cached even when children had `cache_parent_classes` set to `False`.
- Ensures that plugin restrictions are correctly evaluated, especially in environments where caching is more aggressive.
- Fixes a long-standing flaw in caching plugin restrictions.

Problem statement:
bug: Cannot add form fields indirectly to a form (e.g., below row/col)
## Issue

djangocms-form-builder customizes plugin restrictions such that form fields only can be added as indirect children of a form plugin, e.g.,

* Form
  * Textarea

or

* Form
  * Row
    * Col
      * Textarea    
    * Col
      * Textarea    

To this end it sets the `cache_parent_classes` property of form fields plugins to `False` (since they need to computed dynamically investigating the plugin parents).

django CMS caches the allowed children of the Col plugin in the second example. Hence if a Col plugin appears before outside a form, the cached classes will not include any form fields.

**To Reproduce**
Steps to reproduce the behavior:

1. Creat a Row with columns
1. Add a "Form" plugin to start a form after the row 
2. Now, you should be able to add form field plugins inside this form plugin as immediate children ("directly")
3. Also, you shold be able to add other plugins, such as rows and columns (by django CMS frontend, e.g.), and form field plugins as their children ("indirectly")

## Solution

django CMS must not cache allowed child plugin classes if one of the children has set `cache_parent_classes` to `False`

Hints from issue discussion:
Hi @fsbraun, I’m looking into this issue but could use a bit more detail to reproduce the behavior. Whenever you have a moment, could you provide some additional information on the ticket? I’d really appreciate it. Thanks!
Hi @amandasavluchinske ! Sure! The supposed behavior should be:

1. Add a "Form" plugin to start a form 
2. Now, you should be able to add form field plugins inside this form plugin as immediate children ("directly")
3. Also, you shold be able to add other plugins, such as rows and columns (by django CMS frontend, e.g.), and form field plugins as their children ("indirectly")

Currently, (2) works, (3) does not, but you can add a child directly and then drag it into position three.

The place to look at is:
https://github.com/django-cms/djangocms-form-builder/blob/a4889f5ff24808c6b23041b3f69d9b1bf023e65e/djangocms_form_builder/cms_plugins/form_plugins.py#L35-L45

It might be connected to caching: 
https://github.com/django-cms/django-cms/blob/1e2ff09e1134eddd49c12b7dc04f1acb63018a26/cms/plugin_base.py#L254-L258

Let me know if this helps!
@amandasavluchinske @maxnoelp2 I have looked into this some more and realized that this might be an issue with caching plugin restrictions in the django CMS core. Depending on the page I was able  to reproduce the following examples:

## Example: Form only
* Form
  * Row
    * Column
       * Textarea (I could add this directly to the Column plugin)
* Row
  * Column
     * Textarea (I could add this directly, despite it should not be possible)

## Example: Row/Col before form, too
* Row
  * Column
* Form
  * Row
    * Column
       * X (I could not add a Textarea directly to the Column plugin)
  
## Explanation
Django CMS caches plugin restrictions for performance reasons. In the first example, when the allowed child plugins are calculated for the Column plugin, Textarea is added (and cached) to the list of allowed child plugins, because there is a Form plugin further up the tree.

In the second example, no Textarea plugins can be added to the first Column pluguin (since it is outside a Form). This is cached and remains true for the second Column plugin.

## Hypothesis
* To fix this, djangocms-form-builder's plugin would need to set `cache_parent_classes` to `False` to avoid caching them.
* django CMS core would need to avoid caching child classes if direct children have `cache_parent_classes` set to `False`and to reevaluate those every time.

## Next steps
Before we potentially move this issue to django CMS core: @maxnoelp2 Can you confirm this behavior?
@fsbraun Exactly, you cannot access the form plugins directly at third level. You have to place them directly in the From plugin in order to move them to the columns.
@maxnoelp2 In the first example I could. Can you verify?

Commit message: fix: child restrictions cached despite children have cache_parent_classes set to False

Patch to review:
diff --git a/cms/plugin_base.py b/cms/plugin_base.py
index d8cb75ff83a..187f7a51233 100644
--- a/cms/plugin_base.py
+++ b/cms/plugin_base.py
@@ -251,9 +251,9 @@ class MyPlugin(CMSPluginBase):
     #: plugins. Defaults to ``True`` unless :setting:`CMS_ALWAYS_REFRESH_CONTENT` is set to ``True``.
     is_local = not get_cms_setting("ALWAYS_REFRESH_CONTENT")
 
-    # Warning: setting these to False, may have a serious performance impact,
-    # because their child-parent-relation must be recomputed each
-    # time the plugin tree is rendered.
+    # Warning: setting these to False, may have a serious performance impact, because their child-parent-relation must
+    # be recomputed each time the plugin tree is rendered. If a parent with cache_child_classes is True but has
+    # children with cache_parent_classes set to False, the child classes will not be cached.
     cache_child_classes = True
     cache_parent_classes = True
 
diff --git a/cms/utils/plugins.py b/cms/utils/plugins.py
index 742fa8b4bb0..cf2d6315953 100644
--- a/cms/utils/plugins.py
+++ b/cms/utils/plugins.py
@@ -55,11 +55,11 @@ def get_plugins(request, placeholder, template, lang=None):
         plugins = get_plugins(request, placeholder, template)
 
         # Get plugins for a placeholder in a template with specific language
-        plugins = get_plugins(request, placeholder, template, lang='en')
+        plugins = get_plugins(request, placeholder, template, lang="en")
     """
     if not placeholder:
         return []
-    if not hasattr(placeholder, '_plugins_cache'):
+    if not hasattr(placeholder, "_plugins_cache"):
         assign_plugins(request, [placeholder], template, lang)
     return placeholder._plugins_cache
 
@@ -91,11 +91,7 @@ def assign_plugins(request, placeholders, template=None, lang=None):
         return
     placeholders = tuple(placeholders)  # Trigger db hit
     lang = lang or get_language_from_request(request)
-    plugins = list(
-        CMSPlugin
-        .objects
-        .filter(placeholder__in=placeholders, language=lang)
-    )
+    plugins = list(CMSPlugin.objects.filter(placeholder__in=placeholders, language=lang))
     if not plugins:
         # Create default plugins if enabled
         plugins = create_default_plugins(request, placeholders, template, lang)
@@ -136,14 +132,11 @@ def _create_default_plugins(placeholder, confs, parent=None):
         Returns the plugins at the current level along with all descendants.
         """
         plugins, descendants = [], []
-        addable_confs = (conf for conf in confs
-                         if has_plugin_permission(request.user,
-                                                  conf['plugin_type'], 'add'))
+        addable_confs = (conf for conf in confs if has_plugin_permission(request.user, conf["plugin_type"], "add"))
         for conf in addable_confs:
-            plugin = add_plugin(placeholder, conf['plugin_type'], lang,
-                                target=parent, **conf['values'])
-            if 'children' in conf:
-                args = placeholder, conf['children'], plugin
+            plugin = add_plugin(placeholder, conf["plugin_type"], lang, target=parent, **conf["values"])
+            if "children" in conf:
+                args = placeholder, conf["children"], plugin
                 descendants += _create_default_plugins(*args)
             plugin.notify_on_autoadd(request, conf)
             plugins.append(plugin)
@@ -151,14 +144,13 @@ def _create_default_plugins(placeholder, confs, parent=None):
             parent.notify_on_autoadd_children(request, conf, plugins)
         return plugins + descendants
 
-    unfiltered_confs = ((ph, get_placeholder_conf('default_plugins',
-                                                  ph.slot, template))
-                        for ph in placeholders)
+    unfiltered_confs = ((ph, get_placeholder_conf("default_plugins", ph.slot, template)) for ph in placeholders)
     # Empty confs must be filtered before filtering on add permission
-    mutable_confs = ((ph, default_plugin_confs)
-                     for ph, default_plugin_confs
-                     in filter(itemgetter(1), unfiltered_confs)
-                     if ph.has_change_permission(request.user))
+    mutable_confs = (
+        (ph, default_plugin_confs)
+        for ph, default_plugin_confs in filter(itemgetter(1), unfiltered_confs)
+        if ph.has_change_permission(request.user)
+    )
     return sum(starmap(_create_default_plugins, mutable_confs), [])
 
 
@@ -188,8 +180,8 @@ def get_plugin_restrictions(plugin, page=None, restrictions_cache=None):
     cache = plugin_pool.get_restrictions_cache(restrictions_cache, plugin, page=page)
     plugin_type = plugin.plugin_type
     plugin_class = get_plugin_class(plugin.plugin_type)
-    parents_cache = cache.setdefault('plugin_parents', {})
-    children_cache = cache.setdefault('plugin_children', {})
+    parents_cache = cache.setdefault("plugin_parents", {})
+    children_cache = cache.setdefault("plugin_children", {})
 
     try:
         parent_classes = parents_cache[plugin_type]
@@ -206,14 +198,18 @@ def get_plugin_restrictions(plugin, page=None, restrictions_cache=None):
     try:
         child_classes = children_cache[plugin_type]
     except KeyError:
+        cache_child_classes = plugin_class.cache_child_classes
         child_classes = plugin_class.get_child_classes(
             slot=plugin.placeholder.slot,
             page=page,
             instance=plugin,
         )
+        cache_child_classes = cache_child_classes and all(
+            plugin_pool.get_plugin(child_class).cache_parent_classes for child_class in child_classes
+        )
+        if cache_child_classes:
+            children_cache[plugin_type] = child_classes or []
 
-    if plugin_class.cache_child_classes:
-        children_cache[plugin_type] = child_classes or []
     return child_classes, parent_classes
 
 
@@ -234,8 +230,7 @@ def _reunite_orphaned_placeholder_plugin_children(root_plugin, orphaned_plugin_l
             new_plugin.save()
 
 
-def copy_plugins_to_placeholder(plugins, placeholder, language=None,
-                                root_plugin=None, start_positions=None):
+def copy_plugins_to_placeholder(plugins, placeholder, language=None, root_plugin=None, start_positions=None):
     """Copies an iterable of plugins to a placeholder
 
     :param iterable plugins: Plugins to be copied
@@ -298,7 +293,7 @@ def copy_plugins_to_placeholder(plugins, placeholder, language=None,
             position = placeholder.get_next_plugin_position(
                 language=new_plugin.language,
                 parent=new_plugin.parent,
-                insert_order='last',
+                insert_order="last",
             )
             # Because it is the first time this language is processed,
             # shift all plugins to the right of the next position.
@@ -319,9 +314,7 @@ def copy_plugins_to_placeholder(plugins, placeholder, language=None,
 
         # Rescue any orphaned plugins
         if not parent and source_plugin.parent_id:
-            orphaned_plugin_list.append(
-                (source_plugin.parent_id, new_plugin)
-            )
+            orphaned_plugin_list.append((source_plugin.parent_id, new_plugin))
 
     # Reunite any orphaned plugins with the parent
     if orphaned_plugin_list:
@@ -384,9 +377,9 @@ def get_bound_plugins(plugins):
             plugin_lookup[instance.pk] = instance
 
     for plugin in plugins:
-        parent_not_available = (not plugin.parent_id or plugin.parent_id not in plugin_ids)
+        parent_not_available = not plugin.parent_id or plugin.parent_id not in plugin_ids
         # The plugin either has no parent or needs to have a non-ghost parent
-        valid_parent = (parent_not_available or plugin.parent_id in plugin_lookup)
+        valid_parent = parent_not_available or plugin.parent_id in plugin_lookup
 
         if valid_parent and plugin.pk in plugin_lookup:
             plugin._inst = plugin_lookup[plugin.pk]  # Populate bound plugin cache for plugin
@@ -397,7 +390,7 @@ def downcast_plugins(
     plugins: Iterable[CMSPlugin],
     placeholders: Optional[list] = None,
     select_placeholder: bool = False,
-    request: Optional[HttpRequest] = None
+    request: Optional[HttpRequest] = None,
 ) -> Iterable[CMSPlugin]:
     """
     Downcasts the given list of plugins to their respective classes. Ignores any plugins
@@ -425,9 +418,7 @@ def downcast_plugins(
             base_model = get_plugin_model(plugin.plugin_type)._meta.concrete_model  # Collect all base models
         except KeyError:
             # Plugin not available
-            logger.error(
-                f"Plugin not installed: {plugin.plugin_type} (pk={plugin.pk})", exc_info=sys.exc_info()
-            )
+            logger.error(f"Plugin not installed: {plugin.plugin_type} (pk={plugin.pk})", exc_info=sys.exc_info())
             continue
         plugin_ids.append(plugin.pk)
         if base_model is CMSPlugin:
@@ -443,7 +434,7 @@ def downcast_plugins(
         plugin_qs = plugin_model.objects.filter(pk__in=pks)
 
         if select_placeholder:
-            plugin_qs = plugin_qs.select_related('placeholder')
+            plugin_qs = plugin_qs.select_related("placeholder")
 
         # put them in a map, so we can replace the base CMSPlugins with their
         # downcasted versions
@@ -453,9 +444,9 @@ def downcast_plugins(
             plugin_lookup[instance.pk] = instance
 
     for plugin in plugins:
-        parent_not_available = (not plugin.parent_id or plugin.parent_id not in plugin_ids)
+        parent_not_available = not plugin.parent_id or plugin.parent_id not in plugin_ids
         # The plugin either has no parent or needs to have a non-ghost parent
-        valid_parent = (parent_not_available or plugin.parent_id in plugin_lookup)
+        valid_parent = parent_not_available or plugin.parent_id in plugin_lookup
 
         if valid_parent and plugin.pk in plugin_lookup:
             instance = plugin_lookup[plugin.pk]
@@ -500,15 +491,13 @@ def has_reached_plugin_limit(placeholder, plugin_type, language, template=None):
             raise PluginLimitReached(_("This placeholder already has the maximum number of plugins (%s)." % count))
         elif type_limit:
             # total plugin type count
-            type_count = (
-                placeholder
-                .get_plugins(language=language)
-                .filter(plugin_type=plugin_type)
-                .count()
-            )
+            type_count = placeholder.get_plugins(language=language).filter(plugin_type=plugin_type).count()
             if type_count >= type_limit:
                 plugin_name = force_str(plugin_pool.get_plugin(plugin_type).name)
-                raise PluginLimitReached(_(
-                    "This placeholder already has the maximum number (%(limit)s) of allowed %(plugin_name)s plugins."
-                ) % {'limit': type_limit, 'plugin_name': plugin_name})
+                raise PluginLimitReached(
+                    _(
+                        "This placeholder already has the maximum number (%(limit)s) of allowed %(plugin_name)s plugins."
+                    )
+                    % {"limit": type_limit, "plugin_name": plugin_name}
+                )
     return False


Code review comments:
diff_hunk: @@ -206,14 +198,18 @@ def get_plugin_restrictions(plugin, page=None, restrictions_cache=None):
     try:
         child_classes = children_cache[plugin_type]
     except KeyError:
+        cache_child_classes = plugin_class.cache_child_classes
         child_classes = plugin_class.get_child_classes(
             slot=plugin.placeholder.slot,
             page=page,
             instance=plugin,
         )
+        cache_child_classes = cache_child_classes and all(
+            plugin_pool.get_plugin(child_class).cache_parent_classes for child_class in child_classes
+        )
+        if cache_child_classes:
path: cms/utils/plugins.py
text: @user1:
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))

```suggestion
        if cache_child_classes := cache_child_classes and all(
            plugin_pool.get_plugin(child_class).cache_parent_classes
            for child_class in child_classes
        ):
```