From 17ebf27bb3b0d46e266da7de45944922a64460c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 18:57:55 +0000 Subject: [PATCH 01/25] fix(settings): env beats a stored override for env-readable settings (#486) build_settings passed every DB override as an init kwarg, which pydantic-settings ranks above its env source, so for a class that reads env (BackgroundTasksSettings and its SM_BG_TASKS_* vars) a value saved in the admin UI silently beat the operator's variable. Drop overrides for fields the class's own non-init sources supply. DbBackedSettings has no such sources and is unchanged. The admin 'source' badge now reports env over db to match. Closes #486 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm --- modules/settings/settings/_module_settings.py | 10 +-- modules/settings/settings/hydrate.py | 40 +++++++++- modules/settings/tests/test_hydrate.py | 78 ++++++++++++++++++- .../tests/test_settings_field_sources.py | 8 +- .../test_invalidation_cross_process.py | 9 +-- 5 files changed, 127 insertions(+), 18 deletions(-) diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index 672920e01..c9451b661 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -76,17 +76,17 @@ class ModuleSettingField: def source(self) -> str: """Where the live value came from: ``db``, ``env`` or ``default``. - Mirrors the precedence in ``hydrate_settings``: DB overrides are passed - to the constructor explicitly, so they beat anything pydantic reads for - fields left unset, which in turn beats the field default. ``env`` only + Mirrors the precedence in ``hydrate_settings``: env → DB → default + (GH #486). A stored override for a field the environment sets is + ignored, so ``env`` wins even when ``db_override`` is true. ``env`` only appears for settings classes that actually declare an ``env_prefix``; see :attr:`env_set`. Showing this is the difference between "why is this not taking effect" being a five-minute question and an afternoon. """ - if self.db_override: - return "db" if self.env_set: return "env" + if self.db_override: + return "db" return "default" diff --git a/modules/settings/settings/hydrate.py b/modules/settings/settings/hydrate.py index 9662acb8b..7daf4ff15 100644 --- a/modules/settings/settings/hydrate.py +++ b/modules/settings/settings/hydrate.py @@ -14,7 +14,13 @@ from typing import get_origin from pydantic import ValidationError -from pydantic_settings import BaseSettings +from pydantic_settings import ( + BaseSettings, + DotEnvSettingsSource, + EnvSettingsSource, + InitSettingsSource, + SecretsSettingsSource, +) from sqlalchemy.orm import Session from settings.store import SettingsStore, get_overrides_sync @@ -57,6 +63,31 @@ def _parse(raw: str, value_type: str): return raw +def env_supplied_fields(cls: type[BaseSettings]) -> frozenset[str]: + """Fields the class's own non-init sources (env, ``.env``, secrets) answer. + + Asks the sources ``cls`` actually installs, so a ``DbBackedSettings`` + (init-only) reports nothing and an ``env_prefix`` class reports exactly the + ```` names that are set. Deliberately not ``cls()``: a + model validator can reject the env-only view (a production broker check) + that the DB-merged one would pass. + """ + sources = cls.settings_customise_sources( + cls, + init_settings=InitSettingsSource(cls, init_kwargs={}), + env_settings=EnvSettingsSource(cls), + dotenv_settings=DotEnvSettingsSource(cls), + file_secret_settings=SecretsSettingsSource(cls), + ) + by_key = {(info.alias or name): name for name, info in cls.model_fields.items()} + found: set[str] = set() + for source in sources: + if isinstance(source, InitSettingsSource): + continue + found.update(by_key.get(key, key) for key in source()) + return frozenset(found) & cls.model_fields.keys() + + def build_settings[T: BaseSettings]( cls: type[T], raw_overrides: dict[str, tuple[str, str]], *, lenient: bool = False ) -> T: @@ -69,10 +100,15 @@ def build_settings[T: BaseSettings]( lifespan, workers): a row that was stored before a validator tightened must not take the app down, nor discard the module's other, valid overrides. Each offending field is logged and ignored (it falls back to env/default). + + Precedence is env → DB → default (GH #486). pydantic-settings ranks init + kwargs above its env source, so a stored override for a field the + environment sets is dropped here rather than passed in. """ + from_env = env_supplied_fields(cls) parsed: dict[str, object] = {} for field_name, (raw, vtype) in raw_overrides.items(): - if field_name not in cls.model_fields: + if field_name not in cls.model_fields or field_name in from_env: continue try: parsed[field_name] = _parse(raw, vtype) diff --git a/modules/settings/tests/test_hydrate.py b/modules/settings/tests/test_hydrate.py index cb6ef9af5..0b6d15cc3 100644 --- a/modules/settings/tests/test_hydrate.py +++ b/modules/settings/tests/test_hydrate.py @@ -4,13 +4,16 @@ import pytest from pydantic import ValidationError, field_validator, model_validator -from pydantic_settings import BaseSettings +from pydantic_settings import BaseSettings, SettingsConfigDict from settings.hydrate import hydrate_settings, value_type_for_field from settings.service import SettingService from settings.store import SettingsStore class _Cfg(BaseSettings): + # Namespaced so a stray ``HOST``/``PORT`` in the runner's env can't win. + model_config = SettingsConfigDict(env_prefix="SM_TEST_HYDRATE_") + allow: bool = False port: int = 25 host: str = "localhost" @@ -104,6 +107,8 @@ async def test_lenient_hydrate_ignores_only_the_invalid_field(db_session, caplog class _Guarded(BaseSettings): + model_config = SettingsConfigDict(env_prefix="SM_TEST_GUARDED_") + broker: str = "redis://redis:6379" token: str = "ok" prod: bool = False @@ -151,3 +156,74 @@ async def test_lenient_dropping_a_field_cannot_bypass_a_cross_field_check(db_ses await store.set_override("g", "broker", "redis://localhost:6379", "string") with pytest.raises(ValidationError): await hydrate_settings(_Guarded, store, package="g", lenient=True) + + +@pytest.mark.asyncio +async def test_env_var_beats_a_stored_override(db_session, monkeypatch) -> None: + """env → DB → default (GH #486): init kwargs outrank pydantic's env source, + so the hydrator must drop the stored row rather than pass it in.""" + store = SettingsStore(SettingService(db_session)) + await store.set_override("demo", "port", "587", "int") + await store.set_override("demo", "host", "db.example.com", "string") + monkeypatch.setenv("SM_TEST_HYDRATE_PORT", "2525") + cfg = await hydrate_settings(_Cfg, store, package="demo") + assert cfg.port == 2525 + assert cfg.host == "db.example.com" + + +def test_env_beats_db_for_sync_hydration(tmp_path, monkeypatch) -> None: + from settings.hydrate import hydrate_settings_sync + from settings.models import Setting + from sqlalchemy import create_engine + from sqlalchemy.orm import Session + + engine = create_engine(f"sqlite:///{tmp_path / 's.db'}") + Setting.metadata.create_all(engine) + monkeypatch.setenv("SM_TEST_HYDRATE_ALLOW", "false") + with Session(engine) as session: + session.add(Setting(key="demo.allow", value="true", value_type="bool")) + session.commit() + assert hydrate_settings_sync(_Cfg, session, "demo").allow is False + + +@pytest.mark.asyncio +async def test_db_backed_settings_ignore_env_and_keep_db(db_session, monkeypatch) -> None: + """``DbBackedSettings`` has no env source, so a same-named var changes nothing.""" + from simple_module_core.settings_base import DbBackedSettings + + class _Db(DbBackedSettings): + model_config = SettingsConfigDict(env_prefix="SM_TEST_DBB_") + port: int = 25 + + store = SettingsStore(SettingService(db_session)) + await store.set_override("dbb", "port", "587", "int") + monkeypatch.setenv("SM_TEST_DBB_PORT", "2525") + monkeypatch.setenv("PORT", "2525") + assert (await hydrate_settings(_Db, store, package="dbb")).port == 587 + + +@pytest.mark.asyncio +async def test_env_wins_even_where_env_alone_fails_validation(db_session, monkeypatch) -> None: + """Detecting env-set fields must not construct the env-only view: here it + would trip the cross-field check that the merged view passes.""" + store = SettingsStore(SettingService(db_session)) + await store.set_override("g", "broker", "redis://redis:6379", "string") + monkeypatch.setenv("SM_TEST_GUARDED_PROD", "true") + monkeypatch.setenv("SM_TEST_GUARDED_TOKEN", "from-env") + await store.set_override("g", "token", "from-db", "string") + cfg = await hydrate_settings(_Guarded, store, package="g") + assert (cfg.prod, cfg.token) == (True, "from-env") + + +@pytest.mark.asyncio +async def test_background_tasks_env_var_beats_stored_override(db_session, monkeypatch) -> None: + """The bundled class the bug was real for: ``SM_BG_TASKS_*`` must win.""" + from background_tasks.settings import BackgroundTasksSettings + + store = SettingsStore(SettingService(db_session)) + await store.set_override("background_tasks", "task_default_queue", "from-db", "string") + await store.set_override("background_tasks", "worker_concurrency", "4", "int") + monkeypatch.setenv("SM_BG_TASKS_TASK_DEFAULT_QUEUE", "from-env") + cfg = await hydrate_settings(BackgroundTasksSettings, store, package="background_tasks") + assert cfg.task_default_queue == "from-env" + assert cfg.worker_concurrency == 4 diff --git a/modules/settings/tests/test_settings_field_sources.py b/modules/settings/tests/test_settings_field_sources.py index f3e498562..5581c7a50 100644 --- a/modules/settings/tests/test_settings_field_sources.py +++ b/modules/settings/tests/test_settings_field_sources.py @@ -45,10 +45,10 @@ def test_env_var_present_reports_env(self): def test_stored_override_reports_db(self): assert _field(db_override=True).source == "db" - def test_db_override_beats_env(self): - """Mirrors hydrate_settings: DB values are passed to the constructor, - so pydantic never consults the environment for that field.""" - assert _field(env_set=True, db_override=True).source == "db" + def test_env_beats_db_override(self): + """Mirrors hydrate_settings: env → DB → default (GH #486), so a stored + override for a field the environment sets is not the live value.""" + assert _field(env_set=True, db_override=True).source == "env" class TestModulesView: diff --git a/tests/integration/test_invalidation_cross_process.py b/tests/integration/test_invalidation_cross_process.py index 97331abfb..cee638a05 100644 --- a/tests/integration/test_invalidation_cross_process.py +++ b/tests/integration/test_invalidation_cross_process.py @@ -137,12 +137,9 @@ def stack(redis_server, tmp_path_factory) -> _Stack: assert migrated.returncode == 0, f"alembic failed:\n{migrated.stdout}\n{migrated.stderr}" # import-from-env writes an override for *every* SM__ var it - # finds, and a stored override beats the environment from then on - # (``hydrate_settings`` passes DB values as init args, which pydantic-settings - # ranks above its env source). Leaving the SM_BG_TASKS_* vars in this env - # would therefore pin broadcasting to whatever this call happened to see and - # make the per-worker env switch inert — which is exactly how the first draft - # of this test managed to fail its own treatment arm. + # finds. The env still wins over those rows at boot (GH #486), but keeping + # the SM_BG_TASKS_* vars out means the per-worker env switch is the only + # thing deciding broadcasting — no stored row for it exists to reason about. import_env = {k: v for k, v in env.items() if not k.startswith("SM_BG_TASKS_")} import_env["SM_USERS_COOKIE_SECURE"] = "false" import_env["SM_USERS_SESSION_VERSION_CACHE_TTL_SECONDS"] = str(_CACHE_TTL_SECONDS) From 3393bc70c078dde374d0c998b2b545b05804fc75 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 17 Oct 2026 19:04:29 +0000 Subject: [PATCH 02/25] fix(settings): edit json and optional fields as their real types (#487) The generic editor sent a dict field's textarea text as a string and rendered an unset int | None as the literal 'null'. The field view now carries input_type/nullable (X | None unwrapped to X), so optional scalars get their scalar control with empty meaning None, and list/dict fields are parsed as JSON before they are sent. The server stores the validated value rather than the raw input, so a validator that json.loads text no longer leaves a JSON string in the store. Also relabels the 'DB override shadows env' badge: since #486 the env var wins. And test_put_rejects_and_does_not_persist_* read a separate engine, so its 'nothing stored' assertion could not fail; it now reads the app's DB. Closes #487 Claude-Session: https://claude.ai/code/session_017fs25HQddM6G2C7jdZUkbm --- modules/settings/settings/_module_settings.py | 9 +- .../settings/_module_settings_props.py | 2 + modules/settings/settings/hydrate.py | 23 +++- modules/settings/settings/locales/en.json | 8 +- .../settings/pages/components/FieldInput.tsx | 53 ++++++-- .../settings/pages/components/FieldSource.tsx | 8 +- .../settings/pages/components/JsonInput.tsx | 77 +++++++++++ modules/settings/settings/reload.py | 9 +- modules/settings/tests-js/JsonFields.test.tsx | 127 ++++++++++++++++++ modules/settings/tests-js/ModuleForm.test.tsx | 2 +- modules/settings/tests/test_module_api.py | 7 +- .../tests/test_module_api_json_fields.py | 90 +++++++++++++ package-lock.json | 20 +++ packages/i18n/src/generated-resources.ts | 6 +- packages/i18n/src/keys.generated.ts | 6 +- 15 files changed, 421 insertions(+), 26 deletions(-) create mode 100644 modules/settings/settings/pages/components/JsonInput.tsx create mode 100644 modules/settings/tests-js/JsonFields.test.tsx create mode 100644 modules/settings/tests/test_module_api_json_fields.py diff --git a/modules/settings/settings/_module_settings.py b/modules/settings/settings/_module_settings.py index c9451b661..f45039db6 100644 --- a/modules/settings/settings/_module_settings.py +++ b/modules/settings/settings/_module_settings.py @@ -26,7 +26,7 @@ mask, ) from settings.env_vars import env_prefix_for -from settings.hydrate import value_type_for_field +from settings.hydrate import input_type_for_field, value_type_for_field from settings.service import SettingService @@ -71,6 +71,10 @@ class ModuleSettingField: """ choices: list[str] | None = None """Closed set of accepted values, or ``None`` when the field is free text.""" + input_type: str = "" + """Editor control: ``type`` with an ``X | None`` unwrapped to ``X``.""" + nullable: bool = False + """``None`` is a valid value; the editor sends it for an empty input.""" @property def source(self) -> str: @@ -131,6 +135,7 @@ def _field_view( info = cls.model_fields[name] raw_value = getattr(settings, name) value_type = value_type_for_field(cls, name) + input_type, nullable = input_type_for_field(cls, name) named_secret = is_named_secret(name, value_type) extra = info.json_schema_extra if isinstance(info.json_schema_extra, dict) else {} default = resolve_default(info) @@ -166,6 +171,8 @@ def hidden(value: Any) -> bool: # Resolved value panel's env row into a row of dots for every key. env_value=(mask(raw_env) if hidden(raw_env) else raw_env) if raw_env is not None else None, choices=choices_for(info), + input_type=input_type, + nullable=nullable, ) diff --git a/modules/settings/settings/_module_settings_props.py b/modules/settings/settings/_module_settings_props.py index 311f55a09..0d971706f 100644 --- a/modules/settings/settings/_module_settings_props.py +++ b/modules/settings/settings/_module_settings_props.py @@ -49,6 +49,8 @@ def serialize(views: list[ModuleSettingsView]) -> list[dict[str, Any]]: # Turns a pattern-constrained string into a select rather # than a text box whose only feedback on a typo is a 422. "choices": f.choices, + "input_type": f.input_type or f.type, + "nullable": f.nullable, } for f in v.fields ], diff --git a/modules/settings/settings/hydrate.py b/modules/settings/settings/hydrate.py index 7daf4ff15..72f50e03d 100644 --- a/modules/settings/settings/hydrate.py +++ b/modules/settings/settings/hydrate.py @@ -11,7 +11,8 @@ import json import logging -from typing import get_origin +import types +from typing import Union, get_args, get_origin from pydantic import ValidationError from pydantic_settings import ( @@ -51,6 +52,26 @@ def value_type_for_field(cls: type[BaseSettings], field_name: str) -> str: return "string" +def input_type_for_field(cls: type[BaseSettings], field_name: str) -> tuple[str, bool]: + """``(input_type, nullable)`` — which editor control a field needs. + + Storage stays on :func:`value_type_for_field`, which files every union + under ``json``. That is right for the store and wrong for the form: + ``int | None`` wants a number box where empty means ``None``, not a JSON + textarea that shows the literal text ``null`` (GH #487). + """ + ann = cls.model_fields[field_name].annotation + if get_origin(ann) not in (Union, types.UnionType): + return value_type_for_field(cls, field_name), False + args = get_args(ann) + rest = [a for a in args if a is not type(None)] + nullable = len(rest) < len(args) + if len(rest) == 1 and get_origin(rest[0]) is None: + scalar = {bool: "bool", int: "int", float: "float"}.get(rest[0], "string") + return scalar, nullable + return "json", nullable + + def _parse(raw: str, value_type: str): if value_type == "bool": return raw.lower() in ("1", "true", "yes", "on") diff --git a/modules/settings/settings/locales/en.json b/modules/settings/settings/locales/en.json index 274a33ea2..2831c0a4e 100644 --- a/modules/settings/settings/locales/en.json +++ b/modules/settings/settings/locales/en.json @@ -119,7 +119,7 @@ "env_var_hint": "Environment variable name for this field, used by \"smpy settings import-from-env\"", "env_var_not_read_hint": "{env_var} is a label only — this module reads its settings from the database, never from the environment", "source_db": "overridden in DB", - "source_db_over_env": "overridden in DB · shadows env", + "source_db_ignored_for_env": "stored in DB · env var wins", "secret_write_only": "write-only · never returned", "head_title": "Module settings", "managed_title": "Managed on its own page", @@ -134,7 +134,11 @@ "save_failed": "Saving failed. Try again.", "requires_restart": "Requires restart", "reset_to_default": "Revert", - "set_new_value": "Set new value" + "set_new_value": "Set new value", + "invalid_json": "Not valid JSON — it will be rejected on save.", + "not_set": "Not set (default)", + "option_on": "On", + "option_off": "Off" }, "nav": { "settings": "Settings" diff --git a/modules/settings/settings/pages/components/FieldInput.tsx b/modules/settings/settings/pages/components/FieldInput.tsx index 8dddb05e8..c88382bdb 100644 --- a/modules/settings/settings/pages/components/FieldInput.tsx +++ b/modules/settings/settings/pages/components/FieldInput.tsx @@ -8,8 +8,8 @@ import { SelectValue, } from '@simple-module-py/ui/components/ui/select'; import { Switch } from '@simple-module-py/ui/components/ui/switch'; -import { Textarea } from '@simple-module-py/ui/components/ui/textarea'; import { useState } from 'react'; +import { JsonInput } from './JsonInput'; export type FieldType = 'bool' | 'int' | 'float' | 'string' | 'json'; @@ -33,8 +33,14 @@ export type FieldMeta = { source?: 'db' | 'env' | 'default'; /** Closed set of accepted values, or null when the field is free text. */ choices?: string[] | null; + /** Editor control: `type` with an `X | None` unwrapped to `X`. */ + input_type?: FieldType; + /** `null` is a valid value; an empty input sends it. */ + nullable?: boolean; }; +const UNSET = '__unset__'; + type Props = { field: FieldMeta; onChange: (name: string, value: unknown) => void; @@ -49,6 +55,8 @@ export function FieldInput({ field, onChange, value, id }: Props) { const { t } = useT(); const [revealed, setRevealed] = useState(false); const highlight = field.db_override ? OVERRIDDEN : ''; + const kind = field.input_type ?? field.type; + const nullable = !!field.nullable; if (field.is_secret && !revealed) { // Not "Reveal": the server never returns the stored value, so there is @@ -83,7 +91,27 @@ export function FieldInput({ field, onChange, value, id }: Props) { ); } - if (field.type === 'bool') { + if (kind === 'bool' && nullable) { + // `bool | None`: a switch cannot say "not set", which is the default. + const current = value === null || value === undefined ? UNSET : String(value); + return ( + + ); + } + + if (kind === 'bool') { return ( onChange(field.name, e.target.value === '' ? null : Number(e.target.value)) @@ -128,14 +156,15 @@ export function FieldInput({ field, onChange, value, id }: Props) { ); } - if (field.type === 'json') { + if (kind === 'json') { return ( -