fix(value-source): preserve store contract for game_event + error precedence

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.
This commit is contained in:
2026-05-23 00:00:30 +03:00
parent 3b8f00e3f9
commit c1aa2ebec5
3 changed files with 43 additions and 6 deletions
@@ -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(
@@ -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)
@@ -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
# ---------------------------------------------------------------------------