Issue 8124 8126 fix 7660 7682 2 6266 after review

specifyweb/backend/businessrules/migration_utils.py

@@ -1,71 +1,31 @@
-from typing import Tuple, List
-
-from specifyweb.backend.businessrules.uniqueness_rules import create_uniqueness_rule
-
-
 def catnum_rule_editable(apps, schema_editor=None):
     """ Find any CollectionObject catalogNumber must be unique to Collection 
     rules which are readonly on the frontend (have isDatabaseConstraint=True)
     and set their isDatabaseConstraint=False.
-    
+
     Generally should be run only after migration businessrules/0003 has been 
     applied
     """
     UniquenessRule = apps.get_model("businessrules", "UniquenessRule")
-
-    model_rules = UniquenessRule.objects.filter(modelName="Collectionobject", isDatabaseConstraint=True)
 
-    catalog_number_rules: List[int] = []
-    for rule in model_rules: 
+    model_rules = UniquenessRule.objects.filter(
+        modelName="Collectionobject",
+        isDatabaseConstraint=True
+    )
+
+    catalog_number_rules: list[int] = []
+    for rule in model_rules:
         rule_fields = rule.uniquenessrulefield_set.all()
 
         fields = rule_fields.filter(isScope=False)
         scopes = rule_fields.filter(isScope=True)
 
-        # We're only interested in the rule "CollectionObject catalogNumber 
+        # We're only interested in the rule "CollectionObject catalogNumber
         # must be unique to Collection"
-        # We check for length of fields and scopes because get() raises an 
+        # We check for length of fields and scopes because get() raises an
         # exception if more than one result is returned
         if (len(fields) == 1 and len(scopes) == 1) and (fields.get().fieldPath.lower() == "catalognumber" and scopes.get().fieldPath.lower() == "collection"):
             catalog_number_rules.append(rule.id)
-    
+
     rules_to_update = UniquenessRule.objects.filter(id__in=catalog_number_rules)
     rules_to_update.update(isDatabaseConstraint=False)
-
-
-def catnum_rule_uneditable(apps, schema_editor=None):
-    """ Find any CollectionObject catalogNumber must be unique to Collection 
-    rules which are editable on the frontend (have isDatabaseConstraint=False)
-    and set their isDatabaseConstraint=True.
-
-    Generally should be run when migration businessrules/0003 is being reverted
-    """
-    Discipline = apps.get_model("specify", "Discipline")
-    UniquenessRule = apps.get_model("businessrules", "UniquenessRule")
-
-    for discipline in Discipline.objects.all():
-        model_rules = UniquenessRule.objects.filter(modelName="Collectionobject", discipline_id=discipline.id, isDatabaseConstraint=False)
-
-        has_catalognumber_rule = False
-        for rule in model_rules: 
-            rule_fields = rule.uniquenessrulefield_set.all()
-
-            fields = rule_fields.filter(isScope=False)
-            scopes = rule_fields.filter(isScope=True)
-
-            # We're only interested in the rule "CollectionObject catalogNumber 
-            # must be unique to Collection"
-            # We check for length of fields and scopes because get() raises an 
-            # exception if more than one result is returned
-            if (len(fields) == 1 and len(scopes) == 1) and (fields.get().fieldPath.lower() == "catalognumber" and scopes.get().fieldPath.lower() == "collection"):
-                has_catalognumber_rule = True
-
-        if not has_catalognumber_rule:
-            create_uniqueness_rule(
-                model_name="Collectionobject",
-                discipline=discipline,
-                is_database_constraint=True,
-                fields=["catalogNumber"],
-                scopes=["collection"],
-                registry=apps,
-            )

specifyweb/backend/businessrules/migrations/0004_catnum_uniquerule.py

@@ -1,48 +1,64 @@
 from django.db import migrations
 
-from specifyweb.backend.businessrules.migration_utils import catnum_rule_editable, catnum_rule_uneditable
+from specifyweb.backend.businessrules.migration_utils import catnum_rule_editable
 from specifyweb.backend.businessrules.uniqueness_rules import create_uniqueness_rule
 
 
-def catnum_rule_editable(apps, schema_editor):
-    UniquenessRule = apps.get_model('businessrules', 'UniquenessRule')
-    UniquenessRuleField = apps.get_model('businessrules', 'UniquenessRuleField')
-
-    candidate_rules_with_field: tuple[int] = tuple(UniquenessRuleField.objects.filter(uniquenessrule__modelName__iexact='collectionobject', uniquenessrule__isDatabaseConstraint=True, fieldPath__iexact='catalognumber', isScope=False).values_list('uniquenessrule_id', flat=True))
+def catnum_rule_uneditable(apps, schema_editor):
+    """ Find any CollectionObject catalogNumber must be unique to Collection
+    rules which are editable on the frontend (have isDatabaseConstraint=False)
+    and set their isDatabaseConstraint=True.
 
-    candidate_rules_with_scope: tuple[int] = tuple(UniquenessRuleField.objects.filter(uniquenessrule_id__in=candidate_rules_with_field, fieldPath__iexact='collection', isScope=True).values_list('uniquenessrule_id', flat=True))
+    Generally should be run when migration businessrules/0003 is being reverted
+    """
+    Discipline = apps.get_model("specify", "Discipline")
+    UniquenessRule = apps.get_model("businessrules", "UniquenessRule")
 
-    candidate_rules = UniquenessRule.objects.filter(id__in=candidate_rules_with_scope)
-    candidate_rules.update(isDatabaseConstraint=False)
+    for discipline in Discipline.objects.all():
+        # REFACTOR: Some of these queries should be able to be combined to
+        # improve performance and limit how often we need to hit the database
+        model_rules = UniquenessRule.objects.filter(
+            modelName="Collectionobject",
+            discipline_id=discipline.id,
+            isDatabaseConstraint=False
+        )
 
-def catnum_rule_uneditable(apps, schema_editor):
-    Discipline = apps.get_model('specify', 'Discipline')
-    UniquenessRule = apps.get_model('businessrules', 'UniquenessRule')
-    UniquenessRuleField = apps.get_model('businessrules', 'UniquenessRuleField')
+        has_catalognumber_rule = False
+        matching_rule_ids: list[int] = []
+        for rule in model_rules:
+            rule_fields = rule.uniquenessrulefield_set.all()
 
-    for discipline in Discipline.objects.all():
-        candidate_rules_with_field: tuple[int] = tuple(UniquenessRuleField.objects.filter(uniquenessrule__modelName__iexact='collectionobject', uniquenessrule__discipline=discipline.id, uniquenessrule__isDatabaseConstraint=False, fieldPath__iexact='catalognumber', isScope=False).values_list('uniquenessrule_id', flat=True))
+            fields = rule_fields.filter(isScope=False)
+            scopes = rule_fields.filter(isScope=True)
 
-        candidate_rules_with_scope: tuple[int] = tuple(UniquenessRuleField.objects.filter(uniquenessrule_id__in=candidate_rules_with_field, fieldPath__iexact='collection', isScope=True).values_list('uniquenessrule_id', flat=True))
+            # We're only interested in the rule "CollectionObject catalogNumber
+            # must be unique to Collection"
+            # We check for length of fields and scopes because get() raises an
+            # exception if more than one result is returned
+            if (len(fields) == 1 and len(scopes) == 1) and (fields.get().fieldPath.lower() == "catalognumber" and scopes.get().fieldPath.lower() == "collection"):
+                has_catalognumber_rule = True
+                matching_rule_ids.append(rule.id)
 
-        candidate_rules = UniquenessRule.objects.filter(id__in=candidate_rules_with_scope)
-        if len(candidate_rules) == 0: 
+        if has_catalognumber_rule:
+            UniquenessRule.objects.filter(
+                id__in=matching_rule_ids).update(isDatabaseConstraint=True)
+        else:
             create_uniqueness_rule(
-                model_name='Collectionobject',
+                model_name="Collectionobject",
                 discipline=discipline,
                 is_database_constraint=True,
-                fields=['catalogNumber'],
-                scopes=['collection'],
-                registry=apps
+                fields=["catalogNumber"],
+                scopes=["collection"],
+                registry=apps,
             )
-        else: 
-            candidate_rules.update(isDatabaseConstraint=True)
+
 
 class Migration(migrations.Migration):
     dependencies = [
         ('businessrules', '0003_catnum_constraint')
     ]
 
     operations = [
-        migrations.RunPython(catnum_rule_editable, catnum_rule_uneditable, atomic=True)
+        migrations.RunPython(catnum_rule_editable,
+                             catnum_rule_uneditable, atomic=True)
     ]

specifyweb/backend/businessrules/uniqueness_rules.py

@@ -490,58 +490,48 @@ def rule_is_global(scopes: Iterable[str]) -> bool:
 
 
 def fix_global_default_rules(registry=None):
+    """
+    Removes UniquenessRules that are scoped to Discipline that already exist
+    globally.
+
+    There were historically cases where UniquenessRules were incorrectly
+    created in two places: globally and scoped to a particular discipline.
+
+    See https://github.com/specify/specify7/pull/6308#issuecomment-3247556491
+    """
     UniquenessRule = registry.get_model('businessrules', 'UniquenessRule') \
         if registry \
         else models.UniquenessRule
-    UniquenessRuleField = registry.get_model('businessrules', 'UniquenessRuleField') \
-        if registry \
-        else models.UniquenessRuleField
-
-    global_rule_fields = UniquenessRuleField.objects.filter(
-        uniquenessrule__discipline__isnull=True
-    ).values(
-        "uniquenessrule__modelName",
-        "uniquenessrule__isDatabaseConstraint",
-        "fieldPath",
-        "isScope",
-    )
-
-    global_rule_exists = UniquenessRule.objects.filter(
-        discipline__isnull=True,
-        modelName=OuterRef("modelName"),
-        isDatabaseConstraint=OuterRef("isDatabaseConstraint"),
-    )
-
-    discipline_ids = (
-        UniquenessRule.objects.exclude(discipline__isnull=True)
-        .values_list("discipline_id", flat=True)
-        .distinct()
-    )
 
-    for discipline_id in discipline_ids:
-        with transaction.atomic():
-            # Delete matching fields for this discipline
-            matching_fields_qs = UniquenessRuleField.objects.filter(
-                uniquenessrule__discipline_id=discipline_id
-            ).filter(
-                Exists(
-                    global_rule_fields.filter(
-                        **{
-                            "uniquenessrule__modelName": OuterRef("uniquenessrule__modelName"),
-                            "uniquenessrule__isDatabaseConstraint": OuterRef("uniquenessrule__isDatabaseConstraint"),
-                            "fieldPath": OuterRef("fieldPath"),
-                            "isScope": OuterRef("isScope"),
-                        }
-                    )
-                )
-            )
-            matching_fields_qs.delete()
-
-            # Delete UniquenessRule rows for this discipline that are now empty
-            empty_rules_qs = (
-                UniquenessRule.objects.filter(discipline_id=discipline_id)
-                .annotate(field_count=Count("uniquenessrulefield"))
-                .filter(field_count=0)  # now empty after field deletions
-                .filter(Exists(global_rule_exists))
+    global_rule_signatures = {
+            (
+                rule.modelName,
+                rule.isDatabaseConstraint,
+                frozenset(
+                    (field.fieldPath, field.isScope)
+                        for field in rule.uniquenessrulefield_set.all()
+                ),
             )
-            empty_rules_qs.delete()
+            for rule in UniquenessRule.objects.filter(
+                discipline__isnull=True
+            ).prefetch_related("uniquenessrulefield_set")
+        }
+
+    with transaction.atomic():
+        # REFACTOR: See if we can simplify this even further. We should be able
+        # to collapse this query -> iteration -> check workflow to a single
+        # query.
+        # That would eliminate the N + 1 problem with this current approach,
+        # where every scoped rule needs to be evaluated.
+        for rule in UniquenessRule.objects.exclude(discipline__isnull=True).prefetch_related("uniquenessrulefield_set"):
+            signature = (
+                        rule.modelName,
+                        rule.isDatabaseConstraint,
+                        frozenset(
+                            (field.fieldPath, field.isScope)
+                            for field in rule.uniquenessrulefield_set.all()
+                        ),
+                    )
+            if signature in global_rule_signatures:
+                    rule.uniquenessrulefield_set.all().delete()
+                    rule.delete()

specifyweb/backend/permissions/initialize.py

@@ -53,13 +53,28 @@ def create_admins(apps=apps) -> None:
     UserPolicy = apps.get_model('permissions', 'UserPolicy')
     Specifyuser = apps.get_model('specify', 'Specifyuser')
 
-    if UserPolicy.objects.filter(collection__isnull=True, resource='%', action='%').exists():
-        # don't do anything if there is already any admin.
-        return
-
     users = Specifyuser.objects.all()
     for user in users:
-        if is_sp6_user_permissions_migrated(user, apps):
+        # REFACTOR: Try and fold the following checks into a single query to
+        # avoid making multiple queries per user.
+        # Ideally, we only make a single query to fetch all users that:
+        # - Are not already Institution Admins
+        # - Have not already seen activity in Sp 7 (don't have Sp7 permissions)
+        #   - (The Institution Admin permission could have been intentionally
+        #      removed)
+        # - Are admins in Sp 6
+
+        # The ordering here for checks here is intentional: it's more likely a
+        # user has Sp 7 permissions than being an admin, so we do the former
+        # check first
+        if is_sp6_user_permissions_migrated(user=user, apps=apps):
+            continue
+        if UserPolicy.objects.filter(
+            collection__isnull=True,
+            specifyuser_id=user.id,
+            resource="%",
+            action="%",
+        ).exists():
             continue
         if is_legacy_admin(user):
             UserPolicy.objects.get_or_create(
@@ -92,14 +107,6 @@ def assign_users_to_roles(apps=apps) -> None:
 
     results = []
     with connection.cursor() as cursor:
-        cursor.execute("""
-            SELECT COUNT(*)
-            FROM information_schema.tables
-            WHERE table_name IN ('specifyuser_spprincipal', 'spuserrole')
-            AND table_schema = DATABASE();
-        """)
-        if cursor.fetchone()[0] < 2:
-            return # Newly created sp7 databases don't have these sp6 specific tables.
         cursor.execute("""
             SELECT
                 u.SpecifyUserID as user_id,
@@ -112,37 +119,34 @@ def assign_users_to_roles(apps=apps) -> None:
             JOIN spprincipal p ON p.SpPrincipalID = up.SpPrincipalID
             JOIN collection c ON c.UserGroupScopeId = p.userGroupScopeID
             WHERE p.groupType IS NULL
-            AND u.SpecifyUserID NOT IN (
-                SELECT ur.specifyuser_id
+            AND NOT EXISTS (
+                SELECT 1
                 FROM spuserrole ur 
                 JOIN sprole r ON r.id = ur.role_id 
-                WHERE r.collection_id = p.usergroupscopeid
-            )
-            AND c.UserGroupScopeId NOT IN (
-                SELECT DISTINCT r.collection_id
-                FROM spuserrole ur 
-                JOIN sprole r ON r.id = ur.role_id
-                JOIN collection c ON c.UserGroupScopeId = r.collection_id
+                WHERE r.collection_id = c.UserGroupScopeId
+                AND ur.specifyuser_id = u.SpecifyUserID
             );
         """)
 
         results = cursor.fetchall()
 
     for user_id, user_name, user_type, collection_id, collection_name in results:
-        if user_type not in {'Manager', 'FullAccess', 'LimitedAccess', 'Guest'}:
+        # REFACTOR: If we want to exlcude all other roles, why don't we write
+        # the exlcusion in the query rather than evaluate in Python?
+        if user_type not in ROLE_NAMES.keys():
             continue
 
         role_name = ROLE_NAMES.get(user_type, f"{user_type} - {collection_name}")
         role_description = ROLE_DESCRIPTIONS.get(user_type, "No description available.")
         logger.info(f"Assigned user {user_name} to role {role_name} for collection {collection_name}.")
 
-        role, is_new_role = Role.objects.get_or_create(
+        role, _ = Role.objects.get_or_create(
             collection_id=collection_id,
-            name=role_name
+            name=role_name,
+            defaults={
+                "description": role_description
+            }
         )
-        if is_new_role:
-            role.description = role_description
-            role.save()
         UserRole.objects.get_or_create(
             specifyuser_id=user_id,
             role=role

specifyweb/backend/stored_queries/execution.py

@@ -856,9 +856,8 @@ def execute(
         if limit:
             query = query.limit(limit)
 
+        log_sqlalchemy_query(query)
 
-
-        log_sqlalchemy_query(query) # Debugging
         return {"results": apply_special_post_query_processing(query, tableid, field_specs, collection, user)}
 
 def build_query(
@@ -1065,7 +1064,7 @@ def series_post_query(query, limit=40, offset=0, sort_type=0, co_id_cat_num_pair
     and adding a co_id colum and formatted catnum range column.
     Sort the results by the first catnum in the range."""
 
-    log_sqlalchemy_query(query)  # Debugging
+    log_sqlalchemy_query(query)
 
     def parse_catalog_for_comparing(s):
         def check_for_decimal(s):