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

@@ -99,7 +99,11 @@ def _initial_businessrules_migration_applied():
         ).applied_migrations()
     )
 
+def businessrule_app_is_ready(registry):
+    return any(app.label == 'businessrules' for app in registry.get_app_configs())
 
+# BUG: If we reverse past the initial businessrule migration, Specify can still
+# consider the migration applied within those earlier migrations
 def _cached_businessrules_migration_applied() -> bool:
     cache_key = "default"
     cache_is_active, is_set = _uniqueness_migration_cache.get(cache_key, default=False)
@@ -188,12 +192,19 @@ def validate_unique(model, instance):
                 f"Skipping uniqueness rule check on non-Specify model: '{model_name}'")
         return
 
-    if not _cached_businessrules_migration_applied():
-        return
-
     # We can't directly use the main app registry in the context of migrations, which uses fake models
     registry = model._meta.apps
 
+    # If we're in a migration where businessrules have not been loaded and/or
+    # the initial businessrule migration has not been applied, then skip
+    # checking the rule for now.
+    # Note that the former can exist where the latter does: if we're reversing
+    # a migration which does not have a dependency on businessrules (so the
+    # businessrules app does not need to be loaded) but the businessrule
+    # migration is still applied
+    if not businessrule_app_is_ready(registry) or not _cached_businessrules_migration_applied():
+        return
+
     # REFACTOR(perf): We should look into batching UniquenessRule queries.
     # That is, instead of making a query to the DB for each rule, aggregate
     # the rules and make a "single" query.
@@ -490,58 +501,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"),
-                        }
-                    )
-                )
+    global_rule_signatures = {
+            (
+                rule.modelName,
+                rule.isDatabaseConstraint,
+                frozenset(
+                    (field.fieldPath, field.isScope)
+                        for field in rule.uniquenessrulefield_set.all()
+                ),
             )
-            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))
-            )
-            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()