Skip to content
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

Static placeholder triggering a cache set on every request or always part of the SQL queries #4310

Closed
marksweb opened this issue Jul 24, 2015 · 8 comments

Comments

@marksweb
Copy link
Member

I've got a static_placeholder in my base template for the footer of the site.

            <div class="wrapper"><!-- Start wrapper -->

                {% block base-static-footer %}
                    {% static_placeholder "footer" site %}
                {% endblock base-static-footer %}

            </div><!-- End wrapper -->

This testing is happening on my local machine running memcache, in debug mode, without being logged in. Cache settings;

CMS_PAGE_CACHE = False
CMS_PLACEHOLDER_CACHE = True
CMS_PLUGIN_CACHE = True

# -- CMS CACHE DEFAULTS INCREASED -- #
CMS_CACHE_DURATIONS = {
    'menus': 3600,  # default: 3600
    'content': 3600,  # default: 60
    'permissions': 3600,  # default: 3600
}

As I understand it, all static placeholders are cached, and I've had my breakpoints in PyCharm to watch what's happening with all the placeholders on my homepage & it looks as though the footer calls the cache.set as expected.

However if you monitor the debug toolbar on each page refresh, there are 2 cache sets, one comes from the static footer, the other from the cache version.

Now step forward 24 hours & I'm seeing between 54 & 15 SQL queries per request, and in each instance, the static footer is part of the database hits, so today I'm not even seeing the cache sets that I was yesterday (no code/config changes in that time). SQL query;

SELECT ••• FROM `cms_staticplaceholder` WHERE (`cms_staticplaceholder`.`code` = 'footer' AND `cms_staticplaceholder`.`site_id` = 1) LIMIT 21

The end of the stack on that query is;

/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/templatetags/cms_tags.py in render_tag(670)
  rendered_contents = nodelist.render(context)
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/classytags/core.py in render(106)
  return self.render_tag(context, **kwargs)
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/classytags/helpers.py in render_tag(87)
  output = render_to_string(template, data)
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/classytags/core.py in render(106)
  return self.render_tag(context, **kwargs)
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/templatetags/cms_tags.py in render_tag(1121)
  'creation_method': StaticPlaceholder.CREATION_BY_TEMPLATE})

Also, I'm seeing queries relating to other apps which sit on other pages via an app hook, but show as coming from {% static_placeholder "footer" site %} when you inspect the query in the debug toolbar. In this instance, the body attribute of the LatestNews model is a PlaceholderField;

SELECT COUNT(*) FROM `news_latestnews` WHERE `news_latestnews`.`body_id` = 50

Is that query triggered because the ID of LatestNews.body (PlaceholderField) could potentially be 50? It seems an odd query, given the template tag is static_placeholder and it might have an ID of 50, but then why does it query my app's placeholder field?

And if it helps, the bottom end of the stack on that query is;

/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/templatetags/cms_tags.py in render_tag(1137)
  content = render_placeholder(placeholder, context, name_fallback=code, default=nodelist)
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/plugin_rendering.py in render_placeholder(115)
  if placeholder.has_change_permission(request) or not placeholder.cache_placeholder:
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/models/placeholdermodel.py in has_change_permission(118)
  return self._get_permission(request, 'change')
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/models/placeholdermodel.py in _get_permission(107)
  for obj in self._get_attached_objects():
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/models/placeholdermodel.py in _get_attached_objects(227)
  return [obj for field in self._get_attached_fields()
/Users/mwalker/Sites/proj/lib/python2.7/site-packages/cms/models/placeholdermodel.py in _get_attached_fields(157)
  if field.count():
@thoreg
Copy link

thoreg commented Sep 18, 2015

Any news here? Any work in progress?

@FinalAngel
Copy link
Member

@marksweb any update?

@FinalAngel FinalAngel added this to the Clean up issues & pull request milestone Oct 19, 2015
@marksweb
Copy link
Member Author

Unfortunately I've not had the opportunity to look back over this one.

@marksweb
Copy link
Member Author

I think I may have discovered the source of this issue.

We have to monkey patch some of the placeholder rendering & cache key functions because we use Django mobile to support mobile devices. This requires rendering templates to be aware of what flavour of the site you require.

So we needed the ability for the placeholder cache keys to know if we wanted the desktop or mobile flavours. This was a tough issue to locate & I was most confused when desktop was rendered to mobile or mobile to desktop.

So essentially I have to pass the request to placeholder.get_cache_key() for example;

    if get_cms_setting('PLACEHOLDER_CACHE'):
        cache_key = placeholder.get_cache_key(lang, request=request)

When I started to dig around in the CMS code, there are get_cache_key calls without the request being passed so this appeared to be a source of our problems. I'm not sure on a solution yet.

Here are our monkey patches;

from cms import views


def _get_cache_key(request):
    """
    Monkey patch for DjangoCMS cache keys to add in the flavour of the request
    (from Django Mobile)
    """
    import hashlib
    from cms.utils import get_cms_setting
    from django.conf import settings
    from django.utils.encoding import iri_to_uri, force_text
    from django.utils.timezone import get_current_timezone_name

    # md5 key of current path
    cache_key = "%s:%s:%s" % (
        get_cms_setting("CACHE_PREFIX"),
        getattr(request, 'flavour', 'full'),
        hashlib.md5(
            iri_to_uri(request.get_full_path()).encode('utf-8')
        ).hexdigest()
    )
    if settings.USE_TZ:
        # The datetime module doesn't restrict the output of tzname().
        # Windows is known to use non-standard, locale-dependant names.
        # User-defined tzinfo classes may return absolutely anything.
        # Hence this paranoid conversion to create a valid cache key.
        tz_name = force_text(get_current_timezone_name(), errors='ignore')
        cache_key += '.%s' % tz_name.encode(
            'ascii', 'ignore').decode('ascii').replace(' ', '_')
    return cache_key

views._get_cache_key = _get_cache_key


from cms.models import Placeholder


def get_cache_key(self, lang, **kwargs):
    from django.conf import settings
    from django.utils.encoding import force_text
    from django.utils.timezone import get_current_timezone_name
    from cms.utils import get_cms_setting
    from django_mobile import get_flavour

    if 'request' in kwargs:
        flavour = get_flavour(kwargs['request'])
        cache_key = '%srender_placeholder:%s:%s.%s' % (get_cms_setting("CACHE_PREFIX"), flavour, self.pk, str(lang))
    else:
        cache_key = '%srender_placeholder:%s.%s' % (get_cms_setting("CACHE_PREFIX"), self.pk, str(lang))
    if settings.USE_TZ:
        tz_name = force_text(get_current_timezone_name(), errors='ignore')
        cache_key += '.%s' % tz_name.encode('ascii', 'ignore').decode('ascii').replace(' ', '_')

    return cache_key

Placeholder.get_cache_key = get_cache_key


from cms import plugin_rendering


def render_placeholder(placeholder, context_to_copy, name_fallback="Placeholder", lang=None, default=None):
    """
    Renders plugins for a placeholder on the given page using shallow copies of the
    given context, and returns a string containing the rendered output.
    """
    from cms.plugin_rendering import render_plugin, render_plugins, render_placeholder_toolbar
    from cms.utils import get_language_from_request
    from cms.utils.conf import get_cms_setting
    from cms.utils.placeholder import get_placeholder_conf, restore_sekizai_context
    from django.template.loader import render_to_string
    from django.utils.safestring import mark_safe

    # these are always called before all other plugin context processors
    from sekizai.helpers import Watcher

    if not placeholder:
        return
    from cms.utils.plugins import get_plugins
    context = context_to_copy
    context.push()
    request = context['request']
    if not hasattr(request, 'placeholders'):
        request.placeholders = []
    request.placeholders.append(placeholder)
    if hasattr(placeholder, 'content_cache'):
        return mark_safe(placeholder.content_cache)
    page = placeholder.page if placeholder else None
    # It's kind of duplicate of the similar call in `get_plugins`, but it's required
    # to have a valid language in this function for `get_fallback_languages` to work
    if lang:
        save_language = lang
    else:
        lang = get_language_from_request(request)
        save_language = lang

    # Prepend frontedit toolbar output if applicable
    edit = False
    toolbar = getattr(request, 'toolbar', None)

    if getattr(toolbar, 'edit_mode', False):
        edit = True
    if edit:
        from cms.middleware.toolbar import toolbar_plugin_processor

        processors = (toolbar_plugin_processor,)
    else:
        processors = None
    from django.core.cache import cache
    if get_cms_setting('PLACEHOLDER_CACHE'):
        cache_key = placeholder.get_cache_key(lang, request=request)
        if not edit and placeholder and not hasattr(placeholder, 'cache_checked'):
            cached_value = cache.get(cache_key)
            if cached_value is not None:
                restore_sekizai_context(context, cached_value['sekizai'])
                return mark_safe(cached_value['content'])
    if page:
        template = page.template
    else:
        template = None

    plugins = [plugin for plugin in get_plugins(request, placeholder, template, lang=lang)]

    # Add extra context as defined in settings, but do not overwrite existing context variables,
    # since settings are general and database/template are specific
    # TODO this should actually happen as a plugin context processor, but these currently overwrite
    # existing context -- maybe change this order?
    slot = getattr(placeholder, 'slot', None)
    extra_context = {}
    if slot:
        extra_context = get_placeholder_conf("extra_context", slot, template, {})
    for key, value in extra_context.items():
        if key not in context:
            context[key] = value

    content = []
    watcher = Watcher(context)
    content.extend(render_plugins(plugins, context, placeholder, processors))
    toolbar_content = ''

    if edit:
        if not hasattr(request.toolbar, 'placeholders'):
            request.toolbar.placeholders = {}
        if placeholder.pk not in request.toolbar.placeholders:
            request.toolbar.placeholders[placeholder.pk] = placeholder
    if edit:
        toolbar_content = mark_safe(
            render_placeholder_toolbar(
                placeholder, context, name_fallback, save_language
            )
        )
    if content:
        content = mark_safe("".join(content))
    elif default:
        # should be nodelist from a template
        content = mark_safe(default.render(context_to_copy))
    else:
        content = ''
    context['content'] = content
    context['placeholder'] = toolbar_content
    context['edit'] = edit
    result = render_to_string("cms/toolbar/content.html", context)
    changes = watcher.get_changes()
    if placeholder and not edit and placeholder.cache_placeholder and get_cms_setting('PLACEHOLDER_CACHE'):
        cache.set(cache_key, {'content': result, 'sekizai': changes}, get_cms_setting('CACHE_DURATIONS')['content'])
    context.pop()
    return result

plugin_rendering.render_placeholder = render_placeholder

from cms.templatetags import cms_tags


def _get_placeholder(current_page, page, context, name):
    from django.core.cache import cache
    from django.utils.translation import get_language

    from cms.exceptions import PlaceholderNotFound
    from cms.utils.plugins import assign_plugins
    from cms.utils.conf import get_cms_setting
    from cms.utils.placeholder import restore_sekizai_context

    placeholder_cache = getattr(current_page, '_tmp_placeholders_cache', {})
    if page.pk in placeholder_cache:
        placeholder = placeholder_cache[page.pk].get(name, None)
        if placeholder:
            return placeholder
    placeholder_cache[page.pk] = {}
    placeholders = page.rescan_placeholders().values()
    fetch_placeholders = []
    request = context['request']
    if not get_cms_setting('PLACEHOLDER_CACHE') or (
                hasattr(request, 'toolbar') and request.toolbar.edit_mode):
        fetch_placeholders = placeholders
    else:
        for placeholder in placeholders:
            cache_key = placeholder.get_cache_key(
                get_language(), request=request
            )
            cached_value = cache.get(cache_key)
            if cached_value is not None:
                restore_sekizai_context(context, cached_value['sekizai'])
                placeholder.content_cache = cached_value['content']
            else:
                fetch_placeholders.append(placeholder)
            placeholder.cache_checked = True

    if fetch_placeholders:
        assign_plugins(context['request'], fetch_placeholders,
                       page.get_template(), get_language())
    for placeholder in placeholders:
        placeholder_cache[page.pk][placeholder.slot] = placeholder
        placeholder.page = page
    current_page._tmp_placeholders_cache = placeholder_cache
    placeholder = placeholder_cache[page.pk].get(name, None)
    if page.application_urls and not placeholder:
        raise PlaceholderNotFound(
            '"%s" placeholder not found in an apphook application. '
            'Please use a static placeholder instead.' % name)
    return placeholder


cms_tags._get_placeholder = _get_placeholder

@czpython czpython self-assigned this Mar 29, 2016
@mkoistinen mkoistinen assigned mkoistinen and unassigned czpython Apr 12, 2016
@mkoistinen
Copy link
Contributor

I think the issue here is that, the placeholder cache, is still keyed off the page cache version number. I think there's a lot of room for improvement here especially now that we have cache expirations for the plugins, their placeholders and ultimately the page.

This will be fixed as the placeholder-cache becomes more independent of the page-cache. I am working on this now. Stay tuned.

@marksweb
Copy link
Member Author

marksweb commented May 8, 2017

@martinkosir Hello Martin, I've got a curious client who's asked if there is an update on this issue. Do you know when we might see a fix for this please?

@mkoistinen mkoistinen removed their assignment May 9, 2017
@mkoistinen
Copy link
Contributor

Hi @marksweb I'm not in a position to spend any time on this for the foreseeable future. Sorry. Perhaps you can add some test cases, etc., to your monkey-patches and make it a pull request?

@Aiky30
Copy link
Contributor

Aiky30 commented Oct 15, 2020

Hello @marksweb,

Thank you very much for your contribution to the django CMS project. The django CMS project has recently joined forces under the banner of the newly founded django CMS Association to revive the development of django CMS. For this reason we have analyzed all pull requests and issues in detail and decided which ones fit the product vision we want to go with django CMS.

Due to the age of this issue we are unable to safely accept the contribution. If you feel this issue is still required please inform a member of our technical committee: nicolai@django-cms.org.

Nevertheless we thank you for your participation and would like to emphasize that we do not want to discourage you in your contribution to the django CMS project. On the contrary, we encourage everyone who is interested in the success of django CMS to join us!

If you want to learn more about the work of the technical committee, our product vision and how you can become a member of the dCA, you can find all information here:

We are looking forward to every companion who would like to shape the future of django CMS together with us!

If you have any questions, feel free to reach out to Nicolai, our Community Manager: nicolai@django-cms.org

Sign up for the Association now!

@Aiky30 Aiky30 closed this as completed Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants