refactor: change some classes to use __slots__

Speed up access to and reduce size of some low level classes. A few
classes in security.py, rest.py are heavily used. But for all, it
prevents adding random properties to lower level classes that people
shouldn't be mucking with. While doing this I found some test cases
accessing an invalid property name and this change caused the cases to
crash.

admin.py:

  Use new method Role.props_dict() and Permission.props_dict() where
  original code just referenced __dict__ when printing Role/Permission.

mlink_expr.py:

  Add slots to multiple classes.

  Classes Binary and Unary set real properties/attributes. Classes that
  inherit from them (Equals, Empty, Not, Or, And) define empty slots
  tuple to eliminate need for __dict__.

  Class Expression also gets a slot.

rate_limit.py:

  RateLimit and Gcra classes get slots.

  A couple of pep8 fixes: sort imports, remove trailing spaces on a
  line, remove unused noqa comment.

rest.py:

  Add slots to class SimulateFieldStorageFromJson and FsValue
  classes. The memory savings from this could be useful as well as
  speedier access to the attributes.

security.py:

  Add slots to Permission class. To prevent conflict between slot
  limit_perm_to_props_only and the class variable of the same name,
  rename the class variable to limit_perm_to_props_only_default.
  Also define method props_dict() to allow other code to get a dict to
  iterate over when checking permissions.

  Add slots to class Role along with props_dict() method.

  Add slots to class Security. Also have to add explicit __dict__ slot
  to support test override of the hasPermission() method. Add
  props_dict() method, currently unused, but added for symmetry.

support.py:

  TruthDict and PrioList gets slots.

test/test_cgi.py:

  Fix incorrect setting of permission property. Was setting
  permissions. So testing may not have been doing what we thought it
  was. Multiple places found with this typo.

  Remove setting of permissions in some places where it should
  have no effect on the test and looks like it was just copypasta.

test/test_xmlrpc.py

  Remove setting of permissions in some places where it should
  have no effect on the test and looks like it was just copypasta.

Author: rouilj

CHANGES.txt

@@ -35,7 +35,9 @@ Fixed:
   Rouillard)
 - replaced hostname localhost with 127.0.0.1 in docker healthcheck
   script. Found/patch by Norbert Schlemmer. (John Rouillard)
-
+- change some internal classes to use __slots__ for hopefully a small
+  performance improvement. (John Rouillard)
+  
 Features:
 
 - add support for authorized changes. User can be prompted to enter

roundup/admin.py

@@ -250,7 +250,7 @@ def help_commands(self):
                 if seq in h:
                     commands.append(' ' + h.split(seq, 1)[1].lstrip())
                     break
-                    
+
         commands.sort()
         commands.append(_(
 """Commands may be abbreviated as long as the abbreviation
@@ -2084,9 +2084,9 @@ def do_security(self, args):
                 sys.stdout.write(_('New Email users get the Role "%(role)s"\n') % locals())
         roles.sort()
         for _rolename, role in roles:
-            sys.stdout.write(_('Role "%(name)s":\n') % role.__dict__)
+            sys.stdout.write(_('Role "%(name)s":\n') % role.props_dict())
             for permission in role.permission_list():
-                d = permission.__dict__
+                d = permission.props_dict()
                 if permission.klass:
                     if permission.properties:
                         sys.stdout.write(_(

roundup/mlink_expr.py

@@ -60,6 +60,8 @@ def __repr__(self):
 
 class Binary:
 
+    __slots__ = ("x", "y")
+
     def __init__(self, x, y):
         self.x = x
         self.y = y
@@ -71,6 +73,8 @@ def visit(self, visitor):
 
 class Unary:
 
+    __slots__ = ("x",)
+
     def __init__(self, x):
         self.x = x
 
@@ -83,6 +87,8 @@ def visit(self, visitor):
 
 class Equals(Unary):
 
+    __slots__ = ()
+
     def evaluate(self, v):
         return self.x in v
 
@@ -95,6 +101,8 @@ def __repr__(self):
 
 class Empty(Unary):
 
+    __slots__ = ()
+
     def evaluate(self, v):
         return not v
 
@@ -107,6 +115,8 @@ def __repr__(self):
 
 class Not(Unary):
 
+    __slots__ = ()
+
     def evaluate(self, v):
         return not self.x.evaluate(v)
 
@@ -119,6 +129,8 @@ def __repr__(self):
 
 class Or(Binary):
 
+    __slots__ = ()
+
     def evaluate(self, v):
         return self.x.evaluate(v) or self.y.evaluate(v)
 
@@ -133,6 +145,8 @@ def __repr__(self):
 
 class And(Binary):
 
+    __slots__ = ()
+
     def evaluate(self, v):
         return self.x.evaluate(v) and self.y.evaluate(v)
 
@@ -185,6 +199,8 @@ def compile_expression(opcodes):
 
 class Expression:
 
+    __slots__ = ("evaluate",)
+
     def __init__(self, v, is_link=False):
         try:
             opcodes = [int(x) for x in v]

roundup/rate_limit.py

@@ -4,7 +4,7 @@
 # set/get_tat and marshaling as string, support for testonly
 # and status method.
 
-from datetime import timedelta, datetime
+from datetime import datetime, timedelta
 
 try:
     # used by python 3.11 and newer use tz aware dates
@@ -19,12 +19,15 @@
     dt_epoch = datetime(1970, 1, 1)
     def fromisoformat(date):
         # only for naive dates
-        return datetime.strptime(date, "%Y-%m-%dT%H:%M:%S.%f")        
+        return datetime.strptime(date, "%Y-%m-%dT%H:%M:%S.%f")
 
 from roundup.anypy.datetime_ import utcnow
 
 
 class RateLimit:  # pylint: disable=too-few-public-methods
+
+    __slots__ = ("count", "period")
+
     def __init__(self, count, period):
         self.count = count
         self.period = period
@@ -35,6 +38,9 @@ def inverse(self):
 
 
 class Gcra:
+
+    __slots__ = ("memory",)
+
     def __init__(self):
         self.memory = {}
 
@@ -124,7 +130,7 @@ def status(self, key, limit):
             # One item is dequeued every limit.inverse seconds.
             ret['Retry-After'] = str(int(limit.inverse))
             ret['Retry-After-Timestamp'] = "%s" % \
-                    (now + timedelta(seconds=limit.inverse))  # noqa: E127
+                    (now + timedelta(seconds=limit.inverse))
         else:
             # if we are not rejected, the user can post another
             # attempt immediately.

roundup/rest.py

@@ -426,6 +426,9 @@ def execute(cls, instance, path, method, input_payload):
                 func = func_obj['func']
 
                 # zip the varlist into a dictionary, and pass it to the caller
+                # FIXME: 3.10 is min version- add strict=True to zip
+                # also wrap with try/except ValueError if different number of
+                # items (which should never happen).
                 args = dict(zip(list_vars, match_obj.groups()))
                 args['input_payload'] = input_payload
                 return func(instance, **args)
@@ -2741,6 +2744,9 @@ class SimulateFieldStorageFromJson():
     string.
 
     '''
+
+    __slots__ = ("json_dict", "value")
+
     def __init__(self, json_string):
         '''Parse the json string into an internal dict.
 
@@ -2764,6 +2770,9 @@ def raise_error_on_constant(x):
             raise ValueError(e.args[0] + ". JSON is: " + json_string)
 
     class FsValue:
+
+        __slots__ = ("name", "value")
+
         '''Class that does nothing but response to a .value property '''
         def __init__(self, name, val):
             self.name = u2s(name)

roundup/security.py

@@ -18,6 +18,7 @@ class Permission:
         - properties (optional)
         - check function (optional)
         - props_only (optional, internal field is limit_perm_to_props_only)
+          default value taken from Permission.limit_perm_to_props_only_default.
         - filter function (optional) returns filter arguments for
           determining which records are visible by the user. The filter
           function comes into play when determining if a set of nodes
@@ -58,7 +59,18 @@ class Permission:
         with a True or False value.
     '''
 
-    limit_perm_to_props_only = False
+    __slots__ = (
+        "_properties_dict",
+        "check",
+        "check_version",
+        "description",
+        "filter",
+        "klass",
+        "limit_perm_to_props_only",
+        "name",
+        "properties")
+
+    limit_perm_to_props_only_default = False
 
     def __init__(self, name='', description='', klass=None,
                  properties=None, check=None, props_only=None, filter=None):
@@ -80,7 +92,7 @@ def __init__(self, name='', description='', klass=None,
             # a == b will be true.
             if props_only is None:
                 self.limit_perm_to_props_only = \
-                                Permission.limit_perm_to_props_only
+                                Permission.limit_perm_to_props_only_default
             else:
                 # see note on use of bool() in set_props_only_default()
                 self.limit_perm_to_props_only = bool(props_only)
@@ -123,6 +135,9 @@ def check(db, userid, itemid):
 
         return check
 
+    def props_dict(self):
+        return {name: getattr(self, name) for name in self.__slots__}
+
     def test(self, db, permission, classname, property, userid, itemid):
         ''' Test permissions 5 args:
             permission - string like Edit, Register etc. Required, no wildcard.
@@ -222,6 +237,9 @@ class Role:
         - description
         - permissions
     '''
+
+    __slots__ = ("_permissions", "description", "name")
+
     def __init__(self, name='', description='', permissions=None):
         self.name = name.lower()
         self.description = description
@@ -301,6 +319,9 @@ def permission_list(self):
         perm_list.sort(key=lambda x: (x.klass or '', x.name))
         return perm_list
 
+    def props_dict(self):
+        return {name: getattr(self, name) for name in self.__slots__}
+
     def searchable(self, classname, propname):
         for perm_name in 'View', 'Search':
             # Only permissions without a check method
@@ -321,6 +342,12 @@ def searchable(self, classname, propname):
 
 
 class Security:
+
+    # __dict__ is needed to allow mocking of db.security.hasPermission
+    # in test/test_templating.py. Define slots for properties used in
+    # production to increase speed.
+    __slots__ = ("__dict__", "db", "permission", "role")
+
     def __init__(self, db):
         ''' Initialise the permission and role classes, and add in the
             base roles (for admin user).
@@ -452,6 +479,9 @@ def is_filterable(self, permission, userid, classname):
                 return False
         return True
 
+    def props_dict(self):
+        return {name: getattr(self, name) for name in self.__slots__}
+
     def roleHasSearchPermission(self, classname, property, *rolenames):
         """ For each of the given roles, check the permissions.
             Property can be a transitive property.
@@ -544,11 +574,11 @@ def set_props_only_default(self, props_only=None):
             # will be compared as part of tuple == tuple and
             # (3,) == (True,) is False even though 3 is a True value
             # in a boolean context. So use bool() to coerce value.
-            Permission.limit_perm_to_props_only = \
+            Permission.limit_perm_to_props_only_default = \
                                     bool(props_only)
 
     def get_props_only_default(self):
-        return Permission.limit_perm_to_props_only
+        return Permission.limit_perm_to_props_only_default
 
     def addPermissionToRole(self, rolename, permission, classname=None,
                             properties=None, check=None, props_only=None):

roundup/support.py

@@ -11,6 +11,9 @@
 class TruthDict:
     '''Returns True for valid keys, False for others.
     '''
+
+    __slots__ = ('keys',)
+
     def __init__(self, keys):
         if keys:
             self.keys = {}
@@ -49,6 +52,9 @@ class PrioList:
     7
 
     '''
+
+    __slots__ = ('key', 'list', 'sorted')
+
     def __init__(self, key=None):
         self.list = []
         self.key = key

test/test_cgi.py

@@ -1973,7 +1973,7 @@ def testClassPermission(self):
             actions.EditItemAction(cl).handle)
 
     def testCheckAndPropertyPermission(self):
-        self.db.security.permissions = {}
+        self.db.security.permission = {}
         def own_record(db, userid, itemid):
             return userid == itemid
         p = self.db.security.addPermission(name='Edit', klass='user',
@@ -2004,7 +2004,7 @@ def own_record(db, userid, itemid):
     def testCreatePermission(self):
         # this checks if we properly differentiate between create and
         # edit permissions
-        self.db.security.permissions = {}
+        self.db.security.permission = {}
         self.db.security.addRole(name='UserAdd')
         # Don't allow roles
         p = self.db.security.addPermission(name='Create', klass='user',
@@ -2061,7 +2061,6 @@ def testCreatePermission(self):
 
     def testSearchPermission(self):
         # this checks if we properly check for search permissions
-        self.db.security.permissions = {}
         self.db.security.addRole(name='User')
         self.db.security.addRole(name='Project')
         self.db.security.addPermissionToRole('User', 'Web Access')

test/test_xmlrpc.py

@@ -228,7 +228,6 @@ def testAuthAllowedCreate(self):
 
     def testAuthFilter(self):
         # this checks if we properly check for search permissions
-        self.db.security.permissions = {}
         # self.db.security.set_props_only_default(props_only=False)
         self.db.security.addRole(name='User')
         self.db.security.addRole(name='Project')