From 915892758067b533ab6b020b5cbc3b09131247b2 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Tue, 13 Oct 2026 15:29:12 -0500 Subject: [PATCH] fix(settings): environment variables beat datajoint.json for every setting The module documents environment variables as taking precedence over `datajoint.json`, but `_update_from_flat_dict` enforced that only for keys in the hand-kept `ENV_VAR_MAPPING`. For every other setting the file replaced the value read from the environment: `jobs.*` (`DJ_JOBS_*`), `provenance.*` (`DJ_PROVENANCE_*`), `database.use_tls`, `database.reconnect`, and the top-level `safemode`, `query_cache`, `download_path` and `ignore_config_file`. A deployment could not, for example, turn off `safemode` or provenance capture for one worker when the project's file set it. `ENV_VAR_MAPPING` is now derived from the settings models: a field's variable is its `validation_alias`, otherwise its model's `env_prefix` plus the field name. A setting added later is covered as soon as it is declared, including one that has only a `validation_alias`. Models without an `env_prefix` (`connection`, `display`) contribute only their aliased fields, matching the documented variables, and `stores` stays with `_apply_stores_env`, which already runs after the file. The eleven names in the old mapping are unchanged. The tests list every setting that reads an environment variable, assert the derived mapping matches that list, and check for each one, in both flat and nested file layouts, that pydantic reads the variable and the file does not replace it. Closes #1585. --- src/datajoint/settings.py | 87 +++++++++++++++++--------- tests/unit/test_settings.py | 118 ++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 29 deletions(-) diff --git a/src/datajoint/settings.py b/src/datajoint/settings.py index 38339a1d9..44f192e37 100644 --- a/src/datajoint/settings.py +++ b/src/datajoint/settings.py @@ -63,22 +63,6 @@ SYSTEM_SECRETS_DIR = Path("/run/secrets/datajoint") DEFAULT_SUBFOLDING = (2, 2) -# Mapping of config keys to environment variables -# Environment variables take precedence over config file values -ENV_VAR_MAPPING = { - "database.host": "DJ_HOST", - "database.user": "DJ_USER", - "database.password": "DJ_PASS", - "database.backend": "DJ_BACKEND", - "database.port": "DJ_PORT", - "database.name": "DJ_DATABASE_NAME", - "database.database_prefix": "DJ_DATABASE_PREFIX", - "database.create_tables": "DJ_CREATE_TABLES", - "loglevel": "DJ_LOG_LEVEL", - "display.diagram_direction": "DJ_DIAGRAM_DIRECTION", - "display.diagram_theme": "DJ_DIAGRAM_THEME", -} - Role = Enum("Role", "manual lookup imported computed job") role_to_prefix = { Role.manual: "", @@ -761,11 +745,7 @@ def _update_from_flat_dict(self, data: dict[str, Any]) -> None: group_obj = getattr(self, key) for nested_key, nested_value in value.items(): if hasattr(group_obj, nested_key): - # Check if env var is set for this nested key - full_key = f"{key}.{nested_key}" - env_var = ENV_VAR_MAPPING.get(full_key) - if env_var and os.environ.get(env_var): - logger.debug(f"Skipping {full_key} from file (env var {env_var} takes precedence)") + if _is_set_by_env(f"{key}.{nested_key}"): continue setattr(group_obj, nested_key, nested_value) continue @@ -774,10 +754,7 @@ def _update_from_flat_dict(self, data: dict[str, Any]) -> None: parts = key.split(".") if len(parts) == 1: if hasattr(self, key) and not key.startswith("_"): - # Check if env var is set for this key - env_var = ENV_VAR_MAPPING.get(key) - if env_var and os.environ.get(env_var): - logger.debug(f"Skipping {key} from file (env var {env_var} takes precedence)") + if _is_set_by_env(key): continue setattr(self, key, value) elif len(parts) == 2: @@ -785,10 +762,7 @@ def _update_from_flat_dict(self, data: dict[str, Any]) -> None: if hasattr(self, group): group_obj = getattr(self, group) if hasattr(group_obj, attr): - # Check if env var is set for this key - env_var = ENV_VAR_MAPPING.get(key) - if env_var and os.environ.get(env_var): - logger.debug(f"Skipping {key} from file (env var {env_var} takes precedence)") + if _is_set_by_env(key): continue setattr(group_obj, attr, value) elif len(parts) == 3: @@ -1109,6 +1083,61 @@ def get(self, key: str, default: Any = None) -> Any: return default +def _env_var_mapping() -> dict[str, str]: + """ + Map each setting's dot-notation key to the environment variable that sets it. + + The names are read from the settings models, so a new setting is covered + as soon as it is declared: the field's ``validation_alias`` if it has one, + otherwise its model's ``env_prefix`` followed by the field name. A model + without an ``env_prefix`` (``connection``, ``display``) contributes only + its aliased fields. ``stores`` is left out because ``DJ_STORES`` is applied + after the config file, by ``Config._apply_stores_env``. + + Returns + ------- + dict[str, str] + Environment variable name (upper case) by setting key. + """ + + def env_var(model: type[BaseSettings], name: str) -> str | None: + alias = model.model_fields[name].validation_alias + if isinstance(alias, str): + return alias.upper() + prefix = model.model_config.get("env_prefix") or "" + return f"{prefix}{name}".upper() if prefix else None + + mapping: dict[str, str] = {} + for name, field in Config.model_fields.items(): + if name == "stores": + continue + group = field.annotation + if isinstance(group, type) and issubclass(group, BaseSettings): + names = {f"{name}.{attr}": env_var(group, attr) for attr in group.model_fields} + else: + names = {name: env_var(Config, name)} + mapping.update({key: var for key, var in names.items() if var}) + return mapping + + +# Environment variable for each setting. Environment variables take precedence +# over config file values; see ``Config._update_from_flat_dict``. +ENV_VAR_MAPPING = _env_var_mapping() + + +def _is_set_by_env(key: str) -> bool: + """ + Return True if the environment variable for setting ``key`` is set. + + The config file skips such a setting, so the environment keeps precedence. + """ + env_var = ENV_VAR_MAPPING.get(key) + if env_var and os.environ.get(env_var): + logger.debug(f"Skipping {key} from file (env var {env_var} takes precedence)") + return True + return False + + def _create_config() -> Config: """Create and initialize the global config instance.""" cfg = Config() diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index a2fd26b3f..8cb900a84 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -1,5 +1,6 @@ """Tests for DataJoint settings module.""" +import json from pathlib import Path import pytest @@ -310,6 +311,123 @@ def test_load_nonexistent_file(self): dj.config.load("/nonexistent/path/config.json") +# Every setting that reads an environment variable: the variable, a value for +# it, and a different config file value. A setting added to the models must be +# added here too, or test_every_env_var_has_a_case fails. +ENV_PRECEDENCE_CASES = { + "database.host": ("DJ_HOST", "env-host", "file-host"), + "database.user": ("DJ_USER", "env-user", "file-user"), + "database.password": ("DJ_PASS", "env-pass", "file-pass"), + "database.backend": ("DJ_BACKEND", "postgresql", "mysql"), + "database.port": ("DJ_PORT", "3307", 3308), + "database.name": ("DJ_DATABASE_NAME", "env_db", "file_db"), + "database.reconnect": ("DJ_RECONNECT", "false", True), + "database.use_tls": ("DJ_USE_TLS", "false", True), + "database.database_prefix": ("DJ_DATABASE_PREFIX", "env_", "file_"), + "database.create_tables": ("DJ_CREATE_TABLES", "false", True), + "display.diagram_direction": ("DJ_DIAGRAM_DIRECTION", "TB", "LR"), + "display.diagram_theme": ("DJ_DIAGRAM_THEME", "dark", "light"), + "jobs.auto_refresh": ("DJ_JOBS_AUTO_REFRESH", "false", True), + "jobs.keep_completed": ("DJ_JOBS_KEEP_COMPLETED", "true", False), + "jobs.stale_timeout": ("DJ_JOBS_STALE_TIMEOUT", "60", 120), + "jobs.default_priority": ("DJ_JOBS_DEFAULT_PRIORITY", "1", 9), + "jobs.version_method": ("DJ_JOBS_VERSION_METHOD", "git", "none"), + "jobs.allow_new_pk_fields_in_computed_tables": ("DJ_JOBS_ALLOW_NEW_PK_FIELDS_IN_COMPUTED_TABLES", "true", False), + "jobs.add_job_metadata": ("DJ_JOBS_ADD_JOB_METADATA", "true", False), + "provenance.capture": ("DJ_PROVENANCE_CAPTURE", "true", False), + "provenance.source": ("DJ_PROVENANCE_SOURCE", '{"system": "env"}', {"system": "file"}), + "loglevel": ("DJ_LOG_LEVEL", "DEBUG", "ERROR"), + "safemode": ("DJ_SAFEMODE", "false", True), + "ignore_config_file": ("DJ_IGNORE_CONFIG_FILE", "true", False), + "query_cache": ("DJ_QUERY_CACHE", "/env/cache", "/file/cache"), + "download_path": ("DJ_DOWNLOAD_PATH", "/env/downloads", "/file/downloads"), +} + + +def _precedence_params(): + """One case per setting and config file layout: flat dot-notation, and nested for group settings.""" + for key, (env_var, env_value, file_value) in ENV_PRECEDENCE_CASES.items(): + yield pytest.param(key, env_var, env_value, {key: file_value}, id=f"{key}-flat") + if "." in key: + group, attr = key.split(".") + yield pytest.param(key, env_var, env_value, {group: {attr: file_value}}, id=f"{key}-nested") + + +class TestEnvPrecedence: + """Environment variables take precedence over datajoint.json for every setting.""" + + @pytest.fixture(autouse=True) + def _restore_log_level(self): + """Setting ``loglevel`` sets the level of the datajoint logger; put it back.""" + level = settings.logger.level + yield + settings.logger.setLevel(level) + + def test_every_env_var_has_a_case(self): + """ENV_VAR_MAPPING, derived from the models, lists exactly the settings above. + + A field's variable is its ``validation_alias``, else its model's + ``env_prefix`` plus the field name. Fields of a model without an + ``env_prefix`` (``display.limit``, ``connection.charset``) have none, + and ``stores`` is applied after the file by ``_apply_stores_env``. + """ + expected = {key: env_var for key, (env_var, _, _) in ENV_PRECEDENCE_CASES.items()} + assert settings.ENV_VAR_MAPPING == expected + + @pytest.mark.parametrize("key, env_var, env_value, data", list(_precedence_params())) + def test_env_var_beats_config_file(self, key, env_var, env_value, data, tmp_path, monkeypatch): + """The value read from the environment survives loading a file that sets the key.""" + config_file = tmp_path / CONFIG_FILENAME + config_file.write_text(json.dumps(data)) + + monkeypatch.delenv(env_var, raising=False) + unset = settings.Config()[key] + monkeypatch.setenv(env_var, env_value) + cfg = settings.Config() + from_env = cfg[key] + assert from_env != unset # pydantic-settings reads the variable + + cfg.load(config_file) + assert cfg[key] == from_env # the file does not replace it + + monkeypatch.delenv(env_var) + cfg.load(config_file) + assert cfg[key] != from_env # without the variable, the file applies + + def test_issue_reproduction(self, tmp_path, monkeypatch): + """Settings that were missing from the hand-kept mapping now follow the environment.""" + (tmp_path / ".git").mkdir() + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(settings, "SYSTEM_SECRETS_DIR", tmp_path / "nonexistent-system-secrets") + (tmp_path / CONFIG_FILENAME).write_text( + json.dumps( + { + "safemode": True, + "database.use_tls": True, + "jobs.keep_completed": False, + "provenance.capture": False, + "database.host": "file-host", + } + ) + ) + for var, value in { + "DJ_SAFEMODE": "false", + "DJ_USE_TLS": "false", + "DJ_JOBS_KEEP_COMPLETED": "true", + "DJ_PROVENANCE_CAPTURE": "true", + "DJ_HOST": "env-host", + }.items(): + monkeypatch.setenv(var, value) + + cfg = settings._create_config() + + assert cfg.database.host == "env-host" + assert cfg.safemode is False + assert cfg.database.use_tls is False + assert cfg.jobs.keep_completed is True + assert cfg.provenance.capture is True + + class TestStoreSpec: """Test external store configuration."""