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."""