Skip to content

arch: unify settings update paths to eliminate PRICE_STORE_TO_API translation registry #197

Description

@johanzander

Background

Discovered while diagnosing issue #126 (Frank's Belgian spot_multiplier silently reverting to 1.0 on every restart, fixed in v9.9.0b3). The fix was correct but the underlying architecture is fragile and will cause the same class of bug again.

The problem

BSM.update_settings() is called from two code paths with different contracts:

Startup:   store (snake_case) → PRICE_STORE_TO_API filter → camelCase dict → BSM
PATCH API: store (snake_case) ──────────────────────────────────────────────→ BSM

The startup path filters through a hand-maintained registry (PRICE_STORE_TO_API in api_conversion.py) that translates snake_case → camelCase before passing to BSM. The PATCH handler bypasses this entirely and passes the raw store dict directly.

Both paths happen to work because PriceSettings.update() does _camel_to_snake() on every kwarg — meaning _camel_to_snake("spot_multiplier") == "spot_multiplier" (no-op), so PATCH works with snake_case even though BSM was designed for camelCase.

Why this is fragile

1. Manual registry can silently drift from the dataclass

PRICE_STORE_TO_API is a hand-maintained list. When spot_multiplier and export_spot_multiplier were added to PriceSettings and the wizard's save flow, nobody updated the registry. The startup path silently dropped them; the PATCH path accidentally worked. No test caught this because:

  • PriceSettings.update(**kwargs) accepts partial kwargs and silently keeps defaults for missing fields — no error, no warning
  • mypy cannot see inside update_settings({"price": some_dict})
  • The test fixture _valid_options() only contained the fields already in the registry

We added TestPriceModelAttrsConsistency as a structural guard, but this is a symptom treatment, not a root cause fix.

2. "Works in session, breaks on restart" is undetectable by manual testing

You save settings via the wizard → they apply immediately via PATCH path → everything looks correct. On restart the startup path loses the unmapped fields. Since developers don't restart the service during testing, this class of bug survives review and CI.

3. No validation that the startup dict is complete

If a field is missing from the store (not in PRICE_STORE_TO_API, or absent from bess_settings.json), PriceSettings.update() silently uses the dataclass default. No warning is logged. The system appears healthy.

Root cause

update_settings() was designed to accept camelCase API payloads. The store is snake_case. The translation map (PRICE_STORE_TO_API) exists only to bridge that gap. But the PATCH handler already bypasses it, proving the camelCase layer in the startup path is vestigial.

Proposed fix

Make update_settings() accept snake_case (store format) as the canonical input. Then:

  1. Drop PRICE_STORE_TO_API as a translation registry — no manual list to drift
  2. build_system_settings() validates presence of required fields against the dataclass directly, then passes raw store dicts unchanged
  3. PATCH path: already passes raw store dicts — no change needed
  4. PriceSettings.update(): drop _camel_to_snake() conversion (no longer needed)
  5. At startup: after update_settings(), warn if any non-default-having PriceSettings field was absent from the store (observable gap instead of silent default)

This makes both paths identical. Adding a field to PriceSettings no longer requires updating any registry — just a bootstrap default and a migration entry, both already enforced by existing structural tests.

Files affected

  • core/bess/settings.pyPriceSettings.update(): remove _camel_to_snake conversion
  • backend/api_conversion.pybuild_system_settings(): validate against dataclass fields, pass raw dicts (no camelCase translation); remove translation entries from PRICE_STORE_TO_API
  • backend/api.py — camelCase translation for PATCH/POST payloads stays in the API layer where it belongs
  • backend/tests/test_settings_contracts.py — add startup round-trip integration test: SettingsStore.load()build_system_settings()BSM.update_settings() → assert BSM field values match stored values

Acceptance criteria

  • A field added to PriceSettings with a bootstrap default and migration entry works correctly after restart with zero changes to any mapping file
  • Startup path and PATCH path reach BSM via the same code
  • A startup round-trip integration test covers the full pipeline end-to-end
  • Adding a PriceSettings field without a bootstrap default causes a test failure in CI, not a silent wrong value in production

Metadata

Metadata

Assignees

No one assigned

    Labels

    bot-analyzedTriage bot has processed this issueenhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions