505-race-condition-caused-by-when-constance-registers-django-checks (#514)

Co-authored-by: Iurchenko Sergei

constance/__init__.py

@@ -1,12 +1,7 @@
-import django
 from django.utils.functional import LazyObject
-from . import checks
 
 __version__ = '2.9.1'
 
-if django.VERSION < (3, 2):  # pragma: no cover
-    default_app_config = 'constance.apps.ConstanceConfig'
-
 
 class LazyConfig(LazyObject):
     def _setup(self):

constance/admin.py

@@ -174,7 +174,8 @@ class ConstanceForm(forms.Form):
         if not settings.CONFIG_FIELDSETS:
             return cleaned_data
 
-        if get_inconsistent_fieldnames():
+        missing_keys, extra_keys = get_inconsistent_fieldnames()
+        if missing_keys or extra_keys:
             raise forms.ValidationError(_('CONSTANCE_CONFIG_FIELDSETS is missing '
                                           'field(s) that exists in CONSTANCE_CONFIG.'))
 

constance/apps.py

@@ -7,3 +7,6 @@ class ConstanceConfig(AppConfig):
     verbose_name = _('Constance')
     default_auto_field = 'django.db.models.AutoField'
 
+    def ready(self):
+        from . import checks
+

constance/checks.py

@@ -1,36 +1,51 @@
+from typing import Tuple, Set, List
 from django.core import checks
+from django.core.checks import CheckMessage
 from django.utils.translation import gettext_lazy as _
 
 
 @checks.register("constance")
-def check_fieldsets(*args, **kwargs):
+def check_fieldsets(*args, **kwargs) -> List[CheckMessage]:
     """
-    A Django system check to make sure that, if defined, CONFIG_FIELDSETS accounts for
-    every entry in settings.CONFIG.
+    A Django system check to make sure that, if defined,
+    CONFIG_FIELDSETS is consistent with settings.CONFIG.
     """
     from . import settings
 
+    errors = []
+
     if hasattr(settings, "CONFIG_FIELDSETS") and settings.CONFIG_FIELDSETS:
-        inconsistent_fieldnames = get_inconsistent_fieldnames()
-        if inconsistent_fieldnames:
-            return [
-                checks.Warning(
-                    _(
-                        "CONSTANCE_CONFIG_FIELDSETS is missing "
-                        "field(s) that exists in CONSTANCE_CONFIG."
-                    ),
-                    hint=", ".join(sorted(inconsistent_fieldnames)),
-                    obj="settings.CONSTANCE_CONFIG",
-                    id="constance.E001",
-                )
-            ]
-    return []
+        missing_keys, extra_keys = get_inconsistent_fieldnames()
+        if missing_keys:
+            check = checks.Warning(
+                _(
+                    "CONSTANCE_CONFIG_FIELDSETS is missing "
+                    "field(s) that exists in CONSTANCE_CONFIG."
+                ),
+                hint=", ".join(sorted(missing_keys)),
+                obj="settings.CONSTANCE_CONFIG",
+                id="constance.E001",
+            )
+            errors.append(check)
+        if extra_keys:
+            check = checks.Warning(
+                _(
+                    "CONSTANCE_CONFIG_FIELDSETS contains extra "
+                    "field(s) that does not exist in CONFIG."
+                ),
+                hint=", ".join(sorted(extra_keys)),
+                obj="settings.CONSTANCE_CONFIG",
+                id="constance.E002",
+            )
+            errors.append(check)
+    return errors
 
 
-def get_inconsistent_fieldnames():
+def get_inconsistent_fieldnames() -> Tuple[Set, Set]:
     """
-    Returns a set of keys from settings.CONFIG that are not accounted for in
-    settings.CONFIG_FIELDSETS.
+    Returns a pair of values:
+    1) set of keys from settings.CONFIG that are not accounted for in settings.CONFIG_FIELDSETS
+    2) set of keys from settings.CONFIG_FIELDSETS that are not present in settings.CONFIG
     If there are no fieldnames in settings.CONFIG_FIELDSETS, returns an empty set.
     """
     from . import settings
@@ -40,13 +55,16 @@ def get_inconsistent_fieldnames():
     else:
         fieldset_items = settings.CONFIG_FIELDSETS
 
-    field_name_list = []
+    unique_field_names = set()
     for fieldset_title, fields_list in fieldset_items:
         # fields_list can be a dictionary, when a fieldset is defined as collapsible
         # https://django-constance.readthedocs.io/en/latest/#fieldsets-collapsing
         if isinstance(fields_list, dict) and 'fields' in fields_list:
             fields_list = fields_list['fields']
-        field_name_list += list(fields_list)
-    if not field_name_list:
-        return {}
-    return set(set(settings.CONFIG.keys()) - set(field_name_list))
+        unique_field_names.update(fields_list)
+    if not unique_field_names:
+        return unique_field_names, unique_field_names
+    config_keys = set(settings.CONFIG.keys())
+    missing_keys = config_keys - unique_field_names
+    extra_keys = unique_field_names - config_keys
+    return missing_keys, extra_keys

example/cheeseshop/settings.py

@@ -117,6 +117,17 @@ CONSTANCE_CONFIG = {
     ),
 }
 
+CONSTANCE_CONFIG_FIELDSETS = {
+    'Cheese shop general info': [
+        'BANNER',
+        'OWNER',
+        'OWNER_EMAIL',
+        'MUSICIANS',
+        'DATE_ESTABLISHED',
+    ],
+    'Awkward test settings': ['MY_SELECT_KEY', 'MULTILINE', 'JSON_DATA'],
+}
+
 CONSTANCE_BACKEND = 'constance.backends.database.DatabaseBackend'
 
 

tests/test_checks.py

@@ -1,11 +1,6 @@
-import datetime
-from decimal import Decimal
 from unittest import mock
 
-from constance.admin import get_values
 from constance.checks import check_fieldsets, get_inconsistent_fieldnames
-from constance.management.commands.constance import _set_constance_value
-from django.core.exceptions import ValidationError
 from django.test import TestCase
 from constance import settings
 
@@ -14,22 +9,39 @@ class ChecksTestCase(TestCase):
     @mock.patch("constance.settings.CONFIG_FIELDSETS", {"Set1": settings.CONFIG.keys()})
     def test_get_inconsistent_fieldnames_none(self):
         """
-        Test that get_inconsistent_fieldnames returns an empty set and no checks fail
+        Test that get_inconsistent_fieldnames returns an empty data and no checks fail
         if CONFIG_FIELDSETS accounts for every key in settings.CONFIG.
         """
-        self.assertFalse(get_inconsistent_fieldnames())
-        self.assertEqual(0, len(check_fieldsets()))
+        missing_keys, extra_keys = get_inconsistent_fieldnames()
+        self.assertFalse(missing_keys)
+        self.assertFalse(extra_keys)
 
     @mock.patch(
         "constance.settings.CONFIG_FIELDSETS",
         {"Set1": list(settings.CONFIG.keys())[:-1]},
     )
-    def test_get_inconsistent_fieldnames_one(self):
+    def test_get_inconsistent_fieldnames_for_missing_keys(self):
         """
-        Test that get_inconsistent_fieldnames returns a set and the check fails
+        Test that get_inconsistent_fieldnames returns data and the check fails
         if CONFIG_FIELDSETS does not account for every key in settings.CONFIG.
         """
-        self.assertTrue(get_inconsistent_fieldnames())
+        missing_keys, extra_keys = get_inconsistent_fieldnames()
+        self.assertTrue(missing_keys)
+        self.assertFalse(extra_keys)
+        self.assertEqual(1, len(check_fieldsets()))
+
+    @mock.patch(
+        "constance.settings.CONFIG_FIELDSETS",
+        {"Set1": list(settings.CONFIG.keys()) + ['FORGOTTEN_KEY']},
+    )
+    def test_get_inconsistent_fieldnames_for_extra_keys(self):
+        """
+        Test that get_inconsistent_fieldnames returns data and the check fails
+        if CONFIG_FIELDSETS contains extra key that is absent in settings.CONFIG.
+        """
+        missing_keys, extra_keys = get_inconsistent_fieldnames()
+        self.assertFalse(missing_keys)
+        self.assertTrue(extra_keys)
         self.assertEqual(1, len(check_fieldsets()))
 
     @mock.patch(