Change hooks control (deny_all_hooks_but / allow_all_hooks_but) to be more predictable
authorSylvain Thénault <sylvain.thenault@logilab.fr>
Fri, 20 Jan 2017 18:17:04 +0100
changeset 11919 3a6746dfc57f
parent 11918 a88101bf9f87
child 11920 f13799fbcfea
Change hooks control (deny_all_hooks_but / allow_all_hooks_but) to be more predictable Prior to this, if one execute code like: with cnx.hooks.deny_all_hooks_but('metadata'): with cnx.hooks.deny_all_hooks_but(): # mycode 'metadata' hooks will be activated anyway in the inner block, which is rather unexpected (of course in real life you only see the latest hooks control statement, the former being higher in the call stack). This is due to the underlying usage of old `enable_hook_categories` / `disable_hook_categories` methods, which were introduced much before the now official context manager based API (with `cnx.[deny|all]_all_hooks_but(...)`). To move on, this patch drop the two legacy methods, rename and privatize related internal state on the connection (`hooks_mode` becomes `_hooks_mode`, `disabled_hook_cats` and `enabled_hook_cats` become `_hooks_categories`) and reimplement the `_hooks_control` context manager to simply update them. See the added unit test for details.
cubicweb/server/session.py
cubicweb/server/test/unittest_session.py
flake8-ok-files.txt
--- a/cubicweb/server/session.py	Tue Jan 24 14:09:13 2017 +0100
+++ b/cubicweb/server/session.py	Fri Jan 20 18:17:04 2017 +0100
@@ -1,4 +1,4 @@
-# copyright 2003-2016 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
+# copyright 2003-2017 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
 # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr
 #
 # This file is part of CubicWeb.
@@ -109,27 +109,19 @@
         assert mode in (HOOKS_ALLOW_ALL, HOOKS_DENY_ALL)
         self.cnx = cnx
         self.mode = mode
-        self.categories = categories
-        self.oldmode = None
-        self.changes = ()
+        self.categories = set(categories)
+        self.old_mode = None
+        self.old_categories = None
 
     def __enter__(self):
-        self.oldmode = self.cnx.hooks_mode
-        self.cnx.hooks_mode = self.mode
-        if self.mode is HOOKS_DENY_ALL:
-            self.changes = self.cnx.enable_hook_categories(*self.categories)
-        else:
-            self.changes = self.cnx.disable_hook_categories(*self.categories)
+        self.old_mode = self.cnx._hooks_mode
+        self.old_categories = self.cnx._hooks_categories
+        self.cnx._hooks_mode = self.mode
+        self.cnx._hooks_categories = self.categories
 
     def __exit__(self, exctype, exc, traceback):
-        try:
-            if self.categories:
-                if self.mode is HOOKS_DENY_ALL:
-                    self.cnx.disable_hook_categories(*self.categories)
-                else:
-                    self.cnx.enable_hook_categories(*self.categories)
-        finally:
-            self.cnx.hooks_mode = self.oldmode
+        self.cnx._hooks_mode = self.old_mode
+        self.cnx._hooks_categories = self.old_categories
 
 
 @deprecated('[3.17] use <object>.security_enabled instead')
@@ -238,13 +230,8 @@
 
     Hooks controls:
 
-      :attr:`hooks_mode`, may be either `HOOKS_ALLOW_ALL` or `HOOKS_DENY_ALL`.
-
-      :attr:`enabled_hook_cats`, when :attr:`hooks_mode` is
-      `HOOKS_DENY_ALL`, this set contains hooks categories that are enabled.
-
-      :attr:`disabled_hook_cats`, when :attr:`hooks_mode` is
-      `HOOKS_ALLOW_ALL`, this set contains hooks categories that are disabled.
+    .. automethod:: cubicweb.server.session.Connection.deny_all_hooks_but
+    .. automethod:: cubicweb.server.session.Connection.allow_all_hooks_but
 
     Security level Management:
 
@@ -283,9 +270,13 @@
         self.commit_state = None
 
         # hook control attribute
-        self.hooks_mode = HOOKS_ALLOW_ALL
-        self.disabled_hook_cats = set()
-        self.enabled_hook_cats = set()
+        # `_hooks_mode`, may be either `HOOKS_ALLOW_ALL` or `HOOKS_DENY_ALL`.
+        self._hooks_mode = HOOKS_ALLOW_ALL
+        # `_hooks_categories`, when :attr:`_hooks_mode` is `HOOKS_DENY_ALL`,
+        # this set contains hooks categories that are enabled ;
+        # when :attr:`_hooks_mode` is `HOOKS_ALLOW_ALL`, it contains hooks
+        # categories that are disabled.
+        self._hooks_categories = set()
         self.pruned_hooks_cache = {}
 
         # security control attributes
@@ -674,60 +665,26 @@
 
     @_open_only
     def allow_all_hooks_but(self, *categories):
+        """Context manager to enable all hooks but those in the given
+        categories.
+        """
         return _hooks_control(self, HOOKS_ALLOW_ALL, *categories)
 
     @_open_only
     def deny_all_hooks_but(self, *categories):
-        return _hooks_control(self, HOOKS_DENY_ALL, *categories)
-
-    @_open_only
-    def disable_hook_categories(self, *categories):
-        """disable the given hook categories:
-
-        - on HOOKS_DENY_ALL mode, ensure those categories are not enabled
-        - on HOOKS_ALLOW_ALL mode, ensure those categories are disabled
+        """Context manager to disable all hooks but those in the given
+        categories.
         """
-        changes = set()
-        self.pruned_hooks_cache.clear()
-        categories = set(categories)
-        if self.hooks_mode is HOOKS_DENY_ALL:
-            enabledcats = self.enabled_hook_cats
-            changes = enabledcats & categories
-            enabledcats -= changes  # changes is small hence faster
-        else:
-            disabledcats = self.disabled_hook_cats
-            changes = categories - disabledcats
-            disabledcats |= changes  # changes is small hence faster
-        return tuple(changes)
-
-    @_open_only
-    def enable_hook_categories(self, *categories):
-        """enable the given hook categories:
-
-        - on HOOKS_DENY_ALL mode, ensure those categories are enabled
-        - on HOOKS_ALLOW_ALL mode, ensure those categories are not disabled
-        """
-        changes = set()
-        self.pruned_hooks_cache.clear()
-        categories = set(categories)
-        if self.hooks_mode is HOOKS_DENY_ALL:
-            enabledcats = self.enabled_hook_cats
-            changes = categories - enabledcats
-            enabledcats |= changes  # changes is small hence faster
-        else:
-            disabledcats = self.disabled_hook_cats
-            changes = disabledcats & categories
-            disabledcats -= changes  # changes is small hence faster
-        return tuple(changes)
+        return _hooks_control(self, HOOKS_DENY_ALL, *categories)
 
     @_open_only
     def is_hook_category_activated(self, category):
         """return a boolean telling if the given category is currently activated
         or not
         """
-        if self.hooks_mode is HOOKS_DENY_ALL:
-            return category in self.enabled_hook_cats
-        return category not in self.disabled_hook_cats
+        if self._hooks_mode is HOOKS_DENY_ALL:
+            return category in self._hooks_categories
+        return category not in self._hooks_categories
 
     @_open_only
     def is_hook_activated(self, hook):
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/cubicweb/server/test/unittest_session.py	Fri Jan 20 18:17:04 2017 +0100
@@ -0,0 +1,54 @@
+# copyright 2017 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
+# contact http://www.logilab.fr/ -- mailto:contact@logilab.fr
+#
+# This file is part of CubicWeb.
+#
+# CubicWeb is free software: you can redistribute it and/or modify it under the
+# terms of the GNU Lesser General Public License as published by the Free
+# Software Foundation, either version 2.1 of the License, or (at your option)
+# any later version.
+#
+# CubicWeb is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+# FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Lesser General Public License along
+# with CubicWeb.  If not, see <http://www.gnu.org/licenses/>.
+
+from cubicweb.devtools.testlib import CubicWebTC
+from cubicweb.server import session
+
+
+class HooksControlTC(CubicWebTC):
+
+    def test_hooks_control(self):
+        with self.admin_access.repo_cnx() as cnx:
+            self.assertEqual(cnx._hooks_mode, session.HOOKS_ALLOW_ALL)
+            self.assertEqual(cnx._hooks_categories, set())
+
+            with cnx.deny_all_hooks_but('metadata'):
+                self.assertEqual(cnx._hooks_mode, session.HOOKS_DENY_ALL)
+                self.assertEqual(cnx._hooks_categories, set(['metadata']))
+
+                with cnx.deny_all_hooks_but():
+                    self.assertEqual(cnx._hooks_categories, set())
+                self.assertEqual(cnx._hooks_categories, set(['metadata']))
+
+                with cnx.deny_all_hooks_but('integrity'):
+                    self.assertEqual(cnx._hooks_categories, set(['integrity']))
+                self.assertEqual(cnx._hooks_categories, set(['metadata']))
+
+                with cnx.allow_all_hooks_but('integrity'):
+                    self.assertEqual(cnx._hooks_mode, session.HOOKS_ALLOW_ALL)
+                    self.assertEqual(cnx._hooks_categories, set(['integrity']))
+                self.assertEqual(cnx._hooks_mode, session.HOOKS_DENY_ALL)
+                self.assertEqual(cnx._hooks_categories, set(['metadata']))
+
+            self.assertEqual(cnx._hooks_mode, session.HOOKS_ALLOW_ALL)
+            self.assertEqual(cnx._hooks_categories, set())
+
+
+if __name__ == '__main__':
+    import unittest
+    unittest.main()
--- a/flake8-ok-files.txt	Tue Jan 24 14:09:13 2017 +0100
+++ b/flake8-ok-files.txt	Fri Jan 20 18:17:04 2017 +0100
@@ -47,6 +47,7 @@
 cubicweb/server/test/data-schema2sql/__init__.py
 cubicweb/server/test/unittest_checkintegrity.py
 cubicweb/server/test/unittest_ldapsource.py
+cubicweb/server/test/unittest_session.py
 cubicweb/sobjects/test/unittest_notification.py
 cubicweb/sobjects/test/unittest_register_user.py
 cubicweb/sobjects/textparsers.py