ERP5Catalog: Assorted simplifications.

Test loop invariant outside of loop.
Use clearer and more consistent variable names.
Avoid iterating twice when once is enough, allowing removal of
"local_role_dict" local and simplification of corresponding dict
structure.
Avoid creating new instances when the existing one can be mutated.
Evaluate dict as boolean instead of constructing an empty dict to compare
against.
Avoid using %-operator just to concatenate strings (allows en-passant
typechecking).
Drop unused locals.
Do not use a dict when a set would be sufficient.

product/ERP5Catalog/CatalogTool.py

@@ -85,10 +85,10 @@ class IndexableObjectWrapper(object):
     __security_parameter_cache = None
     __local_role_cache = None
 
-    def __init__(self, ob, user_set, role_dict):
+    def __init__(self, ob, user_set, catalog_role_set):
         self.__ob = ob
         self.__user_set = user_set
-        self.__role_dict = role_dict
+        self.__catalog_role_set = catalog_role_set
 
     def __getattr__(self, name):
         return getattr(self.__ob, name)
@@ -110,7 +110,7 @@ class IndexableObjectWrapper(object):
         skip_role_set = set()
         skip_role = skip_role_set.add
         clear_skip_role = skip_role_set.clear
-        for key, role_list in mergedLocalRoles(ob).iteritems():
+        for group_id, role_list in mergedLocalRoles(ob).iteritems():
           new_role_list = []
           new_role = new_role_list.append
           clear_skip_role()
@@ -120,7 +120,7 @@ class IndexableObjectWrapper(object):
             elif role not in skip_role_set:
               new_role(role)
           if new_role_list:
-            local_role_dict[key] = [new_role_list, False]
+            local_role_dict[group_id] = new_role_list
         self.__local_role_cache = local_role_dict
       return local_role_dict
 
@@ -136,15 +136,6 @@ class IndexableObjectWrapper(object):
       result = self.__security_parameter_cache
       if result is None:
         ob = self.__ob
-        local_role_dict = self.__getLocalRoleDict()
-
-        role_dict = self.__role_dict
-        user_set = self.__user_set
-        for security_group_id, security_group_data in local_role_dict.iteritems():
-          security_group_data[1] = security_group_id in user_set
-
-        allowed_dict = {}
-
         # For each local role of a user:
         #   If the local role grants View permission, add it.
         # Every addition implies 2 lines:
@@ -163,68 +154,78 @@ class IndexableObjectWrapper(object):
         allowed_role_set.discard('Owner')
 
         # XXX make this a method of base ?
-        local_roles_group_id_dict = deepcopy(getattr(ob,
-          '__ac_local_roles_group_id_dict__', {}))
-
+        local_roles_group_id_dict = deepcopy(getattr(
+          ob,
+          '__ac_local_roles_group_id_dict__',
+          {},
+        ))
         # If we acquire a permission, then we also want to acquire the local
         # roles group ids
         local_roles_container = ob
         while getattr(local_roles_container, 'isRADContent', 0):
           if local_roles_container._getAcquireLocalRoles():
             local_roles_container = local_roles_container.aq_parent
-            for role_definition_group, user_and_role_list in \
-                getattr(local_roles_container,
-                        '__ac_local_roles_group_id_dict__',
-                        {}).items():
-              local_roles_group_id_dict.setdefault(role_definition_group, set()
-                ).update(user_and_role_list)
+            for role_definition_group, user_and_role_list in getattr(
+              local_roles_container,
+              '__ac_local_roles_group_id_dict__',
+              {},
+            ).iteritems():
+              local_roles_group_id_dict.setdefault(
+                role_definition_group,
+                set(),
+              ).update(user_and_role_list)
           else:
             break
 
-        allowed_by_local_roles_group_id = {}
-        allowed_by_local_roles_group_id[''] = allowed_role_set
-
+        allowed_by_local_roles_group_id = {
+          '': allowed_role_set,
+        }
         optimized_role_set = set()
         for role_definition_group, user_and_role_list in local_roles_group_id_dict.iteritems():
           group_allowed_set = allowed_by_local_roles_group_id.setdefault(
-            role_definition_group, set())
+            role_definition_group,
+            set(),
+          )
           for user, role in user_and_role_list:
             if role in allowed_role_set:
               prefix = 'user:' + user
-              group_allowed_set.update((prefix, '%s:%s' % (prefix, role)))
+              group_allowed_set.add(prefix)
+              group_allowed_set.add(prefix + ':' + role)
               optimized_role_set.add((user, role))
         user_role_dict = {}
         user_view_permission_role_dict = {}
-        for user, (roles, user_exists) in local_role_dict.iteritems():
-          prefix = 'user:' + user
-          for role in roles:
-            is_not_in_optimised_role_set = (user, role) not in optimized_role_set
-            if user_exists and role in role_dict:
-              # User is a user (= not a group) and role is configured as
+        catalog_role_set = self.__catalog_role_set
+        user_set = self.__user_set
+        for group_id, role_list in self.__getLocalRoleDict().iteritems():
+          group_id_is_user = group_id in user_set
+          prefix = 'user:' + group_id
+          for role in role_list:
+            is_not_in_optimised_role_set = (group_id, role) not in optimized_role_set
+            if group_id_is_user and role in catalog_role_set:
+              # group_id is a user (= not a group) and role is configured as
               # monovalued.
               if is_not_in_optimised_role_set:
-                user_role_dict[role] = user
+                user_role_dict[role] = group_id
               if role in allowed_role_set:
                 # ...and local role grants view permission.
-                user_view_permission_role_dict[role] = user
+                user_view_permission_role_dict[role] = group_id
             elif role in allowed_role_set:
               # User is a group and local role grants view permission.
-              for group in local_roles_group_id_dict.get(user, ('', )):
+              for role_definition_group in local_roles_group_id_dict.get(group_id, ('', )):
                 group_allowed_set = allowed_by_local_roles_group_id.setdefault(
-                  group, set())
+                  role_definition_group,
+                  set(),
+                )
                 if is_not_in_optimised_role_set:
                   group_allowed_set.add(prefix)
-                  group_allowed_set.add('%s:%s' % (prefix, role))
+                  group_allowed_set.add(prefix + ':' + role)
 
-        # sort `allowed` principals
-        sorted_allowed_by_local_roles_group_id = {}
-        for local_roles_group_id, allowed in \
-                allowed_by_local_roles_group_id.iteritems():
-          sorted_allowed_by_local_roles_group_id[local_roles_group_id] = tuple(
-            sorted(allowed))
+        # sort and freeze `allowed` principals
+        for local_roles_group_id, allowed in allowed_by_local_roles_group_id.iteritems():
+          allowed_by_local_roles_group_id[local_roles_group_id] = tuple(sorted(allowed))
 
         self.__security_parameter_cache = result = (
-          sorted_allowed_by_local_roles_group_id,
+          allowed_by_local_roles_group_id,
           user_role_dict,
           user_view_permission_role_dict,
         )
@@ -579,14 +580,13 @@ class CatalogTool (UniqueObject, ZCatalog, CMFCoreCatalogTool, ActiveObject):
       role_column_dict = {}
       local_role_column_dict = {}
       catalog = self.getSQLCatalog(sql_catalog_id)
-      column_map = catalog.getColumnMap()
 
       # We only consider here the Owner role (since it was not indexed)
       # since some objects may only be visible by their owner
       # which was not indexed
-      for role, column_id in catalog.getSQLCatalogRoleKeysList():
-        # XXX This should be a list
-        if not user_is_superuser:
+      if not user_is_superuser:
+        for role, column_id in catalog.getSQLCatalogRoleKeysList():
+          # XXX This should be a list
           try:
             # if called by an executable with proxy roles, we don't use
             # owner, but only roles from the proxy.
@@ -633,7 +633,7 @@ class CatalogTool (UniqueObject, ZCatalog, CMFCoreCatalogTool, ActiveObject):
                 column_id = role_dict[role]
                 new_role_column_dict[column_id] = user_str
             new_allowedRolesAndUsers.append('%s:%s' % (user_or_group, role))
-        if local_role_column_dict == {}:
+        if not local_role_column_dict:
           allowedRolesAndUsers = new_allowedRolesAndUsers
           role_column_dict = new_role_column_dict
 
@@ -845,7 +845,7 @@ class CatalogTool (UniqueObject, ZCatalog, CMFCoreCatalogTool, ActiveObject):
       portal = self.getPortalObject()
 
       user_set = set()
-      role_dict = dict(catalog_value.getSQLCatalogRoleKeysList())
+      catalog_role_set = {x for x, _ in catalog_value.getSQLCatalogRoleKeysList()}
       catalog_security_uid_groups_columns_dict = catalog_value.getSQLCatalogSecurityUidGroupsColumnsDict()
       default_security_uid_column = catalog_security_uid_groups_columns_dict['']
       getPredicatePropertyDict = catalog_value.getPredicatePropertyDict
@@ -853,7 +853,7 @@ class CatalogTool (UniqueObject, ZCatalog, CMFCoreCatalogTool, ActiveObject):
       wrapper_list = []
       for object_value in object_value_list:
         document_object = aq_inner(object_value)
-        w = IndexableObjectWrapper(document_object, user_set, role_dict)
+        w = IndexableObjectWrapper(document_object, user_set, catalog_role_set)
         w.predicate_property_dict = getPredicatePropertyDict(object_value) or {}
         security_group_set.update(w._getSecurityGroupIdList())
 
@@ -870,7 +870,7 @@ class CatalogTool (UniqueObject, ZCatalog, CMFCoreCatalogTool, ActiveObject):
           else:
             break
         if is_acquired:
-          document_w = IndexableObjectWrapper(document_object, user_set, role_dict)
+          document_w = IndexableObjectWrapper(document_object, user_set, catalog_role_set)
           security_group_set.update(document_w._getSecurityGroupIdList())
         else:
           document_w = w