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.

Sign up for GitHub

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

Enable resource caching per context to fix context aware expressions #3844

Closed
wants to merge 7 commits into from
Closed

Enable resource caching per context to fix context aware expressions #3844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8da0be4
Enable caching on per context basis to fix context aware expressions.
petschki Sep 21, 2023
d87467f
changenote
petschki Sep 21, 2023
d9d3e48
fix saving renderer in request cache
petschki Sep 21, 2023
63dc57c
fix updating mtime when changing RR entry.
petschki Sep 23, 2023
908cd23
Merge branch '6.0.x' into petschki-issue-3789
MrTango Dec 15, 2023
5ff8c19
sorted list for static cachekey
petschki Dec 20, 2023
40de8e6
Merge branch '6.0.x' into petschki-issue-3789
jensens Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 22 additions & 25 deletions Products/CMFPlone/resources/browser/resource.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

logger = logging.getLogger(__name__)

REQUEST_CACHE_KEY = "_WEBRESOURCE_CACHE_"
_RESOURCE_REGISTRY_MTIME = "__RESOURCE_REGISTRY_MTIME"
GRACEFUL_DEPENDENCY_REWRITE = {
"plone-base": "plone",
Expand Down Expand Up @@ -48,24 +47,24 @@ def _request_bundles(self):
request_disabled_bundles.update(getattr(request, "disabled_bundles", []))
return request_enabled_bundles, request_disabled_bundles

def _user_local_roles(self, site):
portal_membership = getToolByName(site, "portal_membership")
def _user_local_roles(self):
portal_membership = getToolByName(getSite(), "portal_membership")
user = portal_membership.getAuthenticatedMember()
return "|".join(user.getRolesInContext(self.context))
return "|".join(sorted(set(user.getRolesInContext(self.context))))

def _cache_attr_name(self, site):
def _cache_attr_name(self):
hashtool = hashlib.sha256()
hashtool.update(self.__class__.__name__.encode("utf8"))
hashtool.update(site.absolute_url().encode("utf8"))
hashtool.update(self.context.absolute_url().encode("utf8"))
e_bundles, d_bundles = self._request_bundles()
for bundle in e_bundles | d_bundles:
hashtool.update(bundle.encode("utf8"))
hashtool.update(self._user_local_roles(site).encode("utf8"))
hashtool.update(self._user_local_roles().encode("utf8"))
if not getattr(self, "registry", None):
self.registry = getUtility(IRegistry)
mtime = getattr(self.registry, _RESOURCE_REGISTRY_MTIME, None)
if mtime is not None:
hashtool.update(str(mtime).encode("utf8"))
mtime = getattr(self.registry, _RESOURCE_REGISTRY_MTIME, None)
if mtime is not None:
hashtool.update(str(mtime).encode("utf8"))
return f"_v_rendered_cache_{hashtool.hexdigest()}"

@property
Expand All @@ -75,15 +74,17 @@ def _rendered_cache(self):
self.registry = getUtility(IRegistry)
if not self.registry["plone.resources.development"]:
site = getSite()
return getattr(site, self._cache_attr_name(site), None)
return getattr(site, self._cache_attr_name(), None)

@_rendered_cache.setter
def _rendered_cache(self, value):
site = getSite()
setattr(site, self._cache_attr_name(site), value)
setattr(site, self._cache_attr_name(), value)

def update(self):
# cache on request
REQUEST_CACHE_KEY = self._cache_attr_name()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually needed? It seems to me that the previous hardcoded _WEBRESOURCE_CACHE_ key is fine for using on the request: the request is always for one context.

In fact, with this PR the cache on the request is not actually used. I have added some debugging code:

     def update(self):
         # cache on request
         REQUEST_CACHE_KEY = self._cache_attr_name()
+        # REQUEST_CACHE_KEY = "_WEBRESOURCE_CACHE_"
 
         cached = getattr(self.request, REQUEST_CACHE_KEY, None)
+        print(
+            f"Checking {REQUEST_CACHE_KEY} on "
+            f"request {self.request.URL} "
+            f"for resource type {self.resource_type}."
+        )
         if cached is not None:
+            print(f"Found: {cached}")
             self.renderer = cached
             return
+        print(f"Cache not found.")

When I visit a document, the output is for example:

Checking _v_rendered_cache_ab7307c68d8577d0dd04ba4522a5ad2bae88085477671c53f6bbcd312af902e4 on request http://localhost:8080/Plone/folder/world/document_view for resource type js.
Cache not found.
Checking _v_rendered_cache_c5469a242752e29e839a31cb0e0ac0005263e07e36b783af1109ad02e3c2a2a7 on request http://localhost:8080/Plone/folder/world/document_view for resource type css.
Cache not found.

This is when generating contents of the Script and Styles viewlets. (The contents will still come from the volatile attribute on the site root, once set, but that is not the point here.)

When I use REQUEST_CACHE_KEY = "_WEBRESOURCE_CACHE_", the output becomes as follows, showing that the request cache is working:

Checking _WEBRESOURCE_CACHE_ on request http://localhost:8080/Plone/folder/world/document_view for resource type js.
Cache not found.
Checking _WEBRESOURCE_CACHE_ on request http://localhost:8080/Plone/folder/world/document_view for resource type css.
Found: {'js': <webresource._api.ResourceRenderer object at 0x108dea1d0>, 'css': <webresource._api.ResourceRenderer object at 0x1093ac8d0>}

Alternatively, I suppose the part of update method that builds self.renderer could be changed so that it only handles javascript when self.resource_type is js, and only styles when self.resource_type is css. But that may be too big a change.
Just using the old static request cache key should be fine.


cached = getattr(self.request, REQUEST_CACHE_KEY, None)
if cached is not None:
self.renderer = cached
Expand Down Expand Up @@ -299,28 +300,24 @@ def check_dependencies(bundle_name, depends, bundles):

class ResourceView(ResourceBase, ViewletBase):
"""Viewlet Information for script rendering."""


class ScriptsView(ResourceView):
"""Script Viewlet."""
resource_type = None

def index(self):
rendered = self._rendered_cache
if not rendered:
rendered = self.renderer["js"].render()
if not rendered and self.resource_type is not None:
rendered = self.renderer[self.resource_type].render()
self._rendered_cache = rendered
return rendered


class ScriptsView(ResourceView):
"""Script Viewlet."""
resource_type = "js"


class StylesView(ResourceView):
"""Styles Viewlet."""

def index(self):
rendered = self._rendered_cache
if not rendered:
rendered = self.renderer["css"].render()
self._rendered_cache = rendered
return rendered
resource_type = "css"


def update_resource_registry_mtime():
Expand Down
95 changes: 95 additions & 0 deletions Products/CMFPlone/tests/testResourceRegistries.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
from plone.app.testing import logout
from plone.app.testing import setRoles
from plone.app.testing import SITE_OWNER_NAME
from plone.app.testing import SITE_OWNER_PASSWORD
from plone.app.testing import TEST_USER_ID
from plone.base.interfaces import IBundleRegistry
from plone.registry import field as regfield
from plone.registry import Record
from plone.registry.interfaces import IRegistry
from plone.testing.zope import Browser
from Products.CMFPlone.resources import add_bundle_on_request
from Products.CMFPlone.resources import remove_bundle_on_request
from Products.CMFPlone.resources.browser.resource import _RESOURCE_REGISTRY_MTIME
from Products.CMFPlone.resources.browser.resource import ScriptsView
from Products.CMFPlone.resources.browser.resource import StylesView
from Products.CMFPlone.tests import PloneTestCase
Expand Down Expand Up @@ -288,6 +291,9 @@ def test_remove_bundle_on_request_with_subrequest(self):

class TestExpressions(PloneTestCase.PloneTestCase):
def setUp(self):
self.portal = self.layer["portal"]
setRoles(self.portal, TEST_USER_ID, ["Manager"])

# Add three bundles with three different expressions.
registry = getUtility(IRegistry)
data = {
Expand Down Expand Up @@ -332,6 +338,22 @@ def setUp(self):
record.value = regdef[0]
registry.records[f"plone.bundles/testbundle3.{key}"] = record

# test expression on different context
self.portal.invokeFactory("File", id="test-file", file=None)
data = {
"jscompilation": ("http://test4.foo/test.min.js", regfield.TextLine()),
"csscompilation": ("http://test4.foo/test.css", regfield.TextLine()),
"expression": ("python: object.portal_type == 'File'", regfield.TextLine()),
"enabled": (True, regfield.Bool()),
"depends": ("", regfield.TextLine()),
"load_async": (True, regfield.Bool()),
"load_defer": (True, regfield.Bool()),
}
for key, regdef in data.items():
record = Record(regdef[1])
record.value = regdef[0]
registry.records[f"plone.bundles/testbundle4.{key}"] = record

def test_styles_authenticated(self):
styles = StylesView(self.layer["portal"], self.layer["request"], None)
styles.update()
Expand Down Expand Up @@ -359,6 +381,22 @@ def test_styles_anonymous(self):
self.assertNotIn("http://test2.foo/member.css", results)
self.assertIn("http://test3.foo/test.css", results)

def test_styles_on_portal_type(self):
styles = StylesView(self.portal, self.layer["request"], None)
styles.update()
results = styles.render()
# Check that special portal_type expression styles is missing on portal
self.assertNotIn("http://test4.foo/test.css", results)
self.assertIn("http://test3.foo/test.css", results)

# switch context
styles = StylesView(self.portal["test-file"], self.layer["request"], None)
styles.update()
results = styles.render()
# Check that special portal_type expression styles is available on context
self.assertIn("http://test4.foo/test.css", results)
self.assertIn("http://test3.foo/test.css", results)

def test_scripts_authenticated(self):
scripts = ScriptsView(self.layer["portal"], self.layer["request"], None)
scripts.update()
Expand All @@ -384,6 +422,22 @@ def test_scripts_anonymous(self):
self.assertNotIn("http://test2.foo/member.min.js", results)
self.assertIn("http://test3.foo/test.min.js", results)

def test_scripts_on_portal_type(self):
scripts = ScriptsView(self.portal, self.layer["request"], None)
scripts.update()
results = scripts.render()
# Check that special portal_type expression scripts is missing on portal
self.assertNotIn("http://test4.foo/test.min.js", results)
self.assertIn("http://test3.foo/test.min.js", results)

# switch context
scripts = ScriptsView(self.portal["test-file"], self.layer["request"], None)
scripts.update()
results = scripts.render()
# Check that special portal_type expression scripts is available on context
self.assertIn("http://test4.foo/test.min.js", results)
self.assertIn("http://test3.foo/test.min.js", results)

def test_scripts_switching_roles(self):
scripts = ScriptsView(self.layer["portal"], self.layer["request"], None)
scripts.update()
Expand All @@ -401,6 +455,7 @@ def test_scripts_switching_roles(self):
class TestRRControlPanel(PloneTestCase.PloneTestCase):
def setUp(self):
self.portal = self.layer["portal"]
self.registry = getUtility(IRegistry)
self.app = self.layer["app"]
self.browser = Browser(self.app)
self.browser.handleErrors = False
Expand Down Expand Up @@ -450,3 +505,43 @@ def test_update_resource(self):
'<h2 class="accordion-header" id="heading-new-resource-name">',
self.browser.contents,
)

def test_update_registry_mtime(self):
mtime = str(getattr(self.registry, _RESOURCE_REGISTRY_MTIME, ""))

add_form = self.browser.getForm(index=5)
add_form.getControl(name="name").value = "my-resource"
add_form.getControl(name="jscompilation").value = "++resource++my.resource.js"
add_form.getControl(name="enabled").value = "checked"
add_form.getControl("add").click()

# check for updates ScriptsViewlet
self.assertIn(
'++resource++my.resource.js"></script>',
self.browser.contents,
)

# new modification time has to be larger
self.assertTrue(
str(getattr(self.registry, _RESOURCE_REGISTRY_MTIME, ""))
> mtime
)

mtime = str(getattr(self.registry, _RESOURCE_REGISTRY_MTIME, ""))

# new resource form has index=3
my_resource_form = self.browser.getForm(index=3)
my_resource_form.getControl(name="enabled").value = ""
my_resource_form.getControl("update").click()

# check for updated ScriptsViewlet with disabled resource
self.assertNotIn(
'++resource++my.resource.js"></script>',
self.browser.contents,
)

# new modification time has to be larger
self.assertTrue(
str(getattr(self.registry, _RESOURCE_REGISTRY_MTIME, ""))
> mtime
)
3 changes: 3 additions & 0 deletions news/3789.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Generate resource cachekey on per context basis to fix
context aware expressions.
[petschki]