preferences: clear cache in a more clever way

Introduce something similar to "cache cookie", but more suitable for
preferences that depend both on the site configuration and user
preferences.

We store for each user a value in _preference_cache and we modify this
value each time user change his user preference. This value is then
used part of the a cache key.
The same mechanism happen with a global value for site and group
preferences. See _getCacheId docstring for more details.

This way, when one user modify their preference this does not reset
cache for every user.

Because we have proper invalidation, increase the case duration by using
erp5_ui_short instead of erp5_ui_long

This is applied to both preference methods and to templates documents
inside preferences.

test_system_preference_value_prefererred_clear_cache_disabled is updated
to use another preference name, because it was same as in a previous
test, so that we don't have test isolation problems.

product/ERP5/bootstrap/erp5_core/SkinTemplateItem/portal_skins/erp5_core/Base_makeTemplateFromDocument.py

@@ -30,7 +30,7 @@ paste_info, = preference.manage_pasteObjects(cb_copy_data=cp, is_indexable=False
 template = getattr(preference, paste_info['new_id'])
 template.makeTemplate()
 
-context.portal_caches.clearCacheFactory('erp5_ui_short')
+portal.portal_preferences.clearCache(preference)
 
 kw['keep_items'] = dict(portal_status_message=message)
 return context.Base_redirect(form_id,

product/ERP5/bootstrap/erp5_core/WorkflowTemplateItem/portal_workflow/preference_interaction_workflow/scripts/ERP5Site_clearPreferrenceCache.py

-"""Clear caches used by methods of this preference
-
-# TODO: clear different caches according to the preference priority
-# TODO: (XXX) currently, if one use enables / disables a cache, caches
-        for all other users are reset. This is not good for a system
-        in which users do a lot of preference validation. A better solution
-        is needed for this. But it is not easy because the concept of
-        "per user cache" has been proven to be ambiguous or useless.
-        In theory, a solution could consist in using more keys to
-        select caches or to delete "manually" certain cache keys.
-"""
 portal = sci['object'].getPortalObject()
-portal.portal_caches.clearCache(cache_factory_list=('erp5_ui_short',))
+portal.portal_preferences.clearCache(sci['object'])

product/ERP5Form/PreferenceTool.py

@@ -41,6 +41,7 @@ from Products.ERP5Type.Cache import CachingMethod
 from Products.ERP5Type.Utils import convertToUpperCase
 from Products.ERP5Type.TransactionalVariable import getTransactionalVariable
 from Products.ERP5Form import _dtmldir
+from BTrees.OIBTree import OIBTree
 
 _marker = object()
 
@@ -83,10 +84,11 @@ class PreferenceMethod(Method):
       return default
     _getPreference = CachingMethod(_getPreference,
             id='%s.%s' % (self._preference_cache_id,
-                          getSecurityManager().getUser().getId()),
-            cache_factory='erp5_ui_short')
+                          instance.getPortalObject().portal_preferences._getCacheId()),
+            cache_factory='erp5_ui_long')
     return _getPreference(default, *args, **kw)
 
+
 class PreferenceTool(BaseTool):
   """
     PreferenceTool manages User Preferences / User profiles.
@@ -199,6 +201,35 @@ class PreferenceTool(BaseTool):
        Note that this preference may be read only. """
     return self._getActivePreferenceByPortalType('Preference')
 
+  security.declareProtected(Permissions.View, 'clearCache')
+  def clearCache(self, preference):
+    """ clear cache when a preference is modified.
+    This is called by an interaction workflow on preferences.
+    """
+    self._getCacheId() # initialize _preference_cache if needed.
+    if preference.getPriority() == Priority.USER:
+      user_id = getSecurityManager().getUser().getId()
+      self._preference_cache[user_id] = \
+          self._preference_cache.get(user_id, 0) + 1
+    self._preference_cache[None] = self._preference_cache.get(None, 0) + 1
+
+  def _getCacheId(self):
+    """Return a cache id for preferences.
+
+    We use:
+     - user_id: because preferences are always different by user
+     - self._preference_cache[user_id] which is increased everytime a user
+       preference is modified
+     - self._preference_cache[None] which is increased everytime a global
+       preference is modified
+    """
+    user_id = getSecurityManager().getUser().getId()
+    try:
+      self._preference_cache
+    except AttributeError:
+      self._preference_cache = OIBTree()
+    return self._preference_cache.get(None), self._preference_cache.get(user_id), user_id
+
   security.declareProtected(Permissions.View, 'getActiveUserPreference')
   def getActiveUserPreference(self) :
     """ returns the current user preference for the user.
@@ -244,9 +275,10 @@ class PreferenceTool(BaseTool):
         for doc in pref.contentValues(portal_type=portal_type) :
           acceptable_template_list.append(doc.getRelativeUrl())
       return acceptable_template_list
-    _getDocumentTemplateList = CachingMethod(_getDocumentTemplateList,
-                          'portal_preferences.getDocumentTemplateList',
-                                             cache_factory='erp5_ui_short')
+    _getDocumentTemplateList = CachingMethod(
+        _getDocumentTemplateList,
+        'portal_preferences.getDocumentTemplateList.{}'.format(self._getCacheId()),
+        cache_factory='erp5_ui_long')
 
     allowed_content_types = map(lambda pti: pti.id,
                                 folder.allowedContentTypes())

product/ERP5Form/tests/testPreferences.py

@@ -29,7 +29,7 @@
 ##############################################################################
 
 import unittest
-from unittest import expectedFailure
+import mock
 
 from AccessControl.SecurityManagement import noSecurityManager
 from AccessControl.SecurityManagement import getSecurityManager
@@ -687,7 +687,6 @@ class TestPreferences(PropertySheetTestCase):
     self.assertEqual(system_preference_string,
         portal_preferences.getDummystring())
 
-  @expectedFailure
   def test_system_preference_value_prefererred_clear_cache_disabled(self):
     default_preference_string = 'Default Name'
     normal_preference_string = 'Normal Preference'
@@ -695,35 +694,50 @@ class TestPreferences(PropertySheetTestCase):
     self._addProperty('Preference',
         'test_system_preference_value_prefererred_clear_cache_disabled Preference',
         portal_type='Standard Property',
-        property_id='dummystring',
+        property_id='dummystring_cache_disabled',
         property_default='python: "%s"' % default_preference_string,
         preference=True,
         elementary_type='string')
     portal_preferences = self.portal.portal_preferences
-    self.assertEqual(default_preference_string,
-        portal_preferences.getDummystring())
+    self.assertEqual(
+        default_preference_string,
+        portal_preferences.getDummystringCacheDisabled())
 
-    preference = portal_preferences.newContent(portal_type='Preference',
-                                               dummystring=normal_preference_string,
+    preference = portal_preferences.newContent(
+        portal_type='Preference',
+        dummystring_cache_disabled=normal_preference_string,
         priority=Priority.SITE)
     preference.enable()
     self.tic()
 
-    self.assertEqual(normal_preference_string,
-        portal_preferences.getDummystring())
+    self.assertEqual(
+        normal_preference_string,
+        portal_preferences.getDummystringCacheDisabled())
 
     # simulate situation when _clearCache does nothing, for example in case
     # if memcached or any other non-deleteable cache is used
-    raise NotImplementedError("do something to simulate cache disabled")
-    system_preference = portal_preferences.newContent(portal_type='System Preference',
-                                               dummystring=system_preference_string)
+    with mock.patch.object(
+        self.portal.portal_caches.__class__,
+        'clearAllCache') as clearAllCache,\
+      mock.patch.object(
+        self.portal.portal_caches.__class__,
+        'clearCacheFactory') as clearCacheFactory:
+
+      system_preference = portal_preferences.newContent(
+          portal_type='System Preference',
+          dummystring_cache_disabled=system_preference_string)
       system_preference.enable()
       self.tic()
 
-    self.assertEqual(system_preference_string,
-        portal_preferences.getDummystring())
+      self.assertEqual(
+          system_preference_string,
+          portal_preferences.getDummystringCacheDisabled())
+
+      system_preference.setDummystringCacheDisabled("changed")
+      self.tic()
+      self.assertEqual(
+          "changed",
+          portal_preferences.getDummystringCacheDisabled())
 
-def test_suite():
-  suite = unittest.TestSuite()
-  suite.addTest(unittest.makeSuite(TestPreferences))
-  return suite
+    clearAllCache.assert_not_called()
+    clearCacheFactory.assert_not_called()

[Julien Muchembled]

I guess mixing None & strings is not valid in Python 3.

@aurel have you already noticed this ?

[Jérome Perrin]

I remember there was a problem, using None as a key in OIBTree was not working with an old version of BTrees. @seb and I discussed this problem a bit if I remember correctly, but since this was "fixed" in the version of BTrees we are using in ERP5 software release, we concluded it was not a problem.

If it's still a problem, using an empty string instead of None would also be OK, because empty string is not a valid user id.

[Aurel]

@jm Not for now