From c1aa2ebec5be18c19cfbb3cc87659ec5d3af4734 Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Sat, 23 May 2026 00:00:30 +0300 Subject: [PATCH] fix(value-source): preserve store contract for game_event + error precedence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two HIGH issues surfaced by review of 3b8f00e: 1. ``_build_game_event`` was newly succeeding where the old store raised ``ValueError("Invalid source type: game_event")``. The coverage-assertion-symmetry comment was honest about it being a path that didn't exist before, but silent broadening of the create contract is a real behaviour delta — any internal caller that previously caught the error would now succeed. Make ``_build_game_event`` raise NotImplementedError. The coverage assertion still passes (the entry exists), but the historical "you can't create game_event sources through the store" contract is preserved. game_event instances continue to be wired up by the game-integration setup path. 2. The new ``create_source`` ran ``_check_name_unique`` BEFORE ``build_source``. When both ``source_type`` is invalid AND ``name`` collides with an existing source, the old code raised ``"Invalid source type: …"`` first; the new code raised the name-collision error. Swap the order: build first (which validates source_type), then check name uniqueness, then persist. Bonus: a uuid is no longer minted for a source we end up rejecting on type. New test pins the game_event NotImplementedError so a future refactor doesn't accidentally re-open the create path. 38 value-source-store + factory tests stay green; ruff clean. --- .../ledgrab/storage/value_source_factories.py | 16 +++++++++++---- .../src/ledgrab/storage/value_source_store.py | 13 ++++++++++-- .../storage/test_value_source_factories.py | 20 +++++++++++++++++++ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/server/src/ledgrab/storage/value_source_factories.py b/server/src/ledgrab/storage/value_source_factories.py index 674fbde..41a4fce 100644 --- a/server/src/ledgrab/storage/value_source_factories.py +++ b/server/src/ledgrab/storage/value_source_factories.py @@ -327,10 +327,18 @@ def _build_game_event( common: dict, **_, ) -> ValueSource: - # GameEventValueSource has its own defaults; the store never previously - # exposed a way to create one through this path, but we register a - # builder anyway so the coverage assertion stays symmetric. - return GameEventValueSource(**common) + # The store's pre-refactor ``create_source`` never accepted ``game_event`` + # (it wasn't in the ``_VALID`` tuple, and there's no Pydantic create + # schema for it in ``api/schemas/value_sources.py``). GameEventValueSource + # instances are constructed by other code paths (game integration setup, + # data migration). The builder exists purely so the coverage assertion + # stays symmetric with ``_VALUE_SOURCE_MAP`` — invoking it raises so the + # historical "you cannot create a game_event source through the store" + # contract is preserved. + raise NotImplementedError( + "game_event value sources cannot be created through ValueSourceStore; " + "construct GameEventValueSource directly via the game-integration path" + ) def _build_http( diff --git a/server/src/ledgrab/storage/value_source_store.py b/server/src/ledgrab/storage/value_source_store.py index 10a0edf..8374727 100644 --- a/server/src/ledgrab/storage/value_source_store.py +++ b/server/src/ledgrab/storage/value_source_store.py @@ -44,9 +44,13 @@ class ValueSourceStore(BaseSqliteStore[ValueSource]): Per-type construction (defaults, validation, sub-mode dispatch) lives in ``value_source_factories.CREATE_BUILDERS`` so the store only owns name-uniqueness, id minting, and persistence. - """ - self._check_name_unique(name) + Validation order is: source_type membership → type-specific field + validation → name uniqueness. This matches the pre-refactor + precedence so an invalid ``source_type`` + a duplicate ``name`` + still surface as ``"Invalid source type: …"`` instead of the + name-collision error. + """ sid = f"vs_{uuid.uuid4().hex[:8]}" now = datetime.now(timezone.utc) @@ -62,6 +66,11 @@ class ValueSourceStore(BaseSqliteStore[ValueSource]): **kwargs, ) + # Name-uniqueness happens last so we never burn a uuid on a source + # we end up rejecting AND so the user-facing error precedence + # (type errors before name errors) matches the old code's order. + self._check_name_unique(name) + self._items[sid] = source self._save_item(sid, source) diff --git a/server/tests/storage/test_value_source_factories.py b/server/tests/storage/test_value_source_factories.py index 8bac5da..9407a1e 100644 --- a/server/tests/storage/test_value_source_factories.py +++ b/server/tests/storage/test_value_source_factories.py @@ -180,6 +180,26 @@ def test_build_source_unknown_type_raises(): ) +def test_build_source_game_event_is_not_creatable_via_store(): + """game_event sources were never creatable through the store API. + + The builder exists for coverage-assertion symmetry but raises so the + historical contract holds (game-event value sources are wired up by + the game-integration setup path, not user CRUD). + """ + with pytest.raises(NotImplementedError, match="game_event"): + vsf.build_source( + source_type="game_event", + sid="vs_t", + name="Test", + now=_now(), + description=None, + tags=None, + icon=None, + icon_color=None, + ) + + # --------------------------------------------------------------------------- # apply_update — representative paths # ---------------------------------------------------------------------------