diff --git a/netbox/netbox/config/__init__.py b/netbox/netbox/config/__init__.py index b6ca4f90c..1d508c5d5 100644 --- a/netbox/netbox/config/__init__.py +++ b/netbox/netbox/config/__init__.py @@ -20,6 +20,10 @@ _thread_locals = threading.local() logger = logging.getLogger('netbox.config') +# Sentinel used to distinguish a cache miss from a cached "empty" config (an empty dict is a +# legitimate cached value when no ConfigRevision exists). +_MISSING = object() + def get_config(): """ @@ -47,7 +51,9 @@ class Config: """ def __init__(self): self._populate_from_cache() - if not self.config or not self.version: + # Only consult the database when the cache has genuinely never been populated. A cached + # empty config (no ConfigRevision) is authoritative and must not trigger a re-query. + if self._cache_miss: self._populate_from_db() self.defaults = {param.name: param.default for param in PARAMS} @@ -69,8 +75,19 @@ class Config: def _populate_from_cache(self): """Populate config data from Redis cache""" - self.config = cache.get('config') or {} - self.version = cache.get('config_version') + cached_config = cache.get('config', _MISSING) + cached_version = cache.get('config_version', _MISSING) + + # Treat the cache as warm only when both keys are present. A missing 'config_version' + # (e.g. evicted or never written) must re-query the database, even if 'config' is cached. + # The no-revision branch writes both keys (config={}, config_version=None), so the + # intentional empty state is still a cache hit. + self._cache_miss = cached_config is _MISSING or cached_version is _MISSING + + # A cached value of None (ConfigRevision.data is nullable) or {} is a legitimate empty + # config and must not crash attribute access; normalize it to an empty dict. + self.config = {} if cached_config is _MISSING else (cached_config or {}) + self.version = None if cached_version is _MISSING else cached_version if self.config: logger.debug("Loaded configuration data from cache") @@ -86,10 +103,17 @@ class Config: revision = ConfigRevision.objects.order_by('-created').first() if revision is None: logger.debug("No configuration found in database; proceeding with default values") + # Cache the empty state so subsequent requests are served from the cache rather than + # re-querying the database on every request (#22158). Creating the first + # ConfigRevision overwrites this via the post_save handler. + cache.set('config', {}, None) + cache.set('config_version', None, None) + self._populate_from_cache() return logger.debug(f"No active configuration revision found; falling back to most recent (#{revision.pk})") except DatabaseError: - # The database may not be available yet (e.g. when running a management command) + # The database may not be available yet (e.g. when running a management command). Do NOT + # cache anything here, so the next instantiation re-queries once the database is reachable. logger.warning("Skipping config initialization (database unavailable)") return diff --git a/netbox/netbox/tests/test_config.py b/netbox/netbox/tests/test_config.py index 4d2a0345e..9c0d721a3 100644 --- a/netbox/netbox/tests/test_config.py +++ b/netbox/netbox/tests/test_config.py @@ -1,22 +1,31 @@ -from django.conf import settings from django.core.cache import cache +from django.db import connection from django.test import TestCase, override_settings +from django.test.utils import CaptureQueriesContext from core.models import ConfigRevision from netbox.config import clear_config, get_config -# Prefix cache keys to avoid interfering with the local environment -CACHES = settings.CACHES -CACHES['default'].update({'KEY_PREFIX': 'TEST-'}) + +def _configrevision_query_count(queries): + """Count captured queries that touch the core_configrevision table.""" + return len([q for q in queries if 'core_configrevision' in q['sql']]) -@override_settings(CACHES=CACHES) +# Use a per-process in-memory cache so the shared 'config'/'config_version' keys can't be +# contaminated by other tests running in parallel against the same Redis instance. +@override_settings(CACHES={ + 'default': { + 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', + 'LOCATION': 'netbox-config-tests', + }, +}) class ConfigTestCase(TestCase): def setUp(self): super().setUp() # Register cleanup so it runs after each test, even on assertion failure. - # clear_config drops the thread-local Config; cache.clear wipes the Redis + # clear_config drops the thread-local Config; cache.clear wipes the cache # entries that configrevision.activate writes. self.addCleanup(cache.clear) self.addCleanup(clear_config) @@ -30,6 +39,68 @@ class ConfigTestCase(TestCase): self.assertEqual(config.config, {}) self.assertEqual(config.version, None) + def test_empty_config_not_requeried(self): + # With no ConfigRevision present, the empty state should be cached after the first load so + # that subsequent requests are served from the cache rather than re-querying the database + # on every request (#22158). + with CaptureQueriesContext(connection) as ctx: + first = get_config() + self.assertGreaterEqual(_configrevision_query_count(ctx.captured_queries), 1) + self.assertEqual(first.config, {}) + self.assertEqual(first.version, None) + + # Simulate the request boundary, where the middleware drops the thread-local config. + clear_config() + + with CaptureQueriesContext(connection) as ctx: + second = get_config() + self.assertEqual(_configrevision_query_count(ctx.captured_queries), 0) + self.assertEqual(second.config, {}) + self.assertEqual(second.version, None) + + def test_empty_then_create_first_revision(self): + # Prime the cache with the empty state. + get_config() + + # Creating the first ConfigRevision fires the post_save handler, which activates it and + # overwrites the cached empty state. + CONFIG_DATA = {'BANNER_TOP': 'A'} + configrevision = ConfigRevision.objects.create(data=CONFIG_DATA) + + clear_config() + config = get_config() + self.assertEqual(config.config, CONFIG_DATA) + self.assertEqual(config.version, configrevision.pk) + + def test_config_init_from_cache_null_data(self): + # ConfigRevision.data is nullable; a revision with data=None caches None. Config access + # must normalize that to an empty dict rather than crash on attribute lookup. + configrevision = ConfigRevision.objects.create(data=None) + configrevision.activate() + + clear_config() + config = get_config() + self.assertEqual(config.config, {}) + self.assertEqual(config.version, configrevision.pk) + # Attribute access must fall back to defaults without raising. + self.assertEqual(config.BANNER_TOP, '') + + def test_missing_version_key_requeries(self): + # If 'config_version' is missing (evicted/never written) while 'config' is cached, the + # cache must be treated as cold and re-populated from the database rather than leaving + # version=None when a ConfigRevision exists. + CONFIG_DATA = {'BANNER_TOP': 'A'} + configrevision = ConfigRevision.objects.create(data=CONFIG_DATA) + configrevision.activate() + + # Drop only the version key, leaving 'config' populated. + cache.delete('config_version') + clear_config() + + config = get_config() + self.assertEqual(config.config, CONFIG_DATA) + self.assertEqual(config.version, configrevision.pk) + def test_config_init_from_db(self): CONFIG_DATA = {'BANNER_TOP': 'A'}