diff --git a/compose2pod/extends.py b/compose2pod/extends.py index 45d884a..2ad5394 100644 --- a/compose2pod/extends.py +++ b/compose2pod/extends.py @@ -3,7 +3,7 @@ from typing import Any from compose2pod.exceptions import UnsupportedComposeError -from compose2pod.keys import SERVICE_KEYS, pairs_to_mapping +from compose2pod.keys import SERVICE_KEYS, concat_list, pairs_to_mapping # Merge policy for keys with a SERVICE_KEYS KeySpec comes from spec.merge (see @@ -39,7 +39,14 @@ def _extends_target(name: str, ext: Any) -> str: # noqa: ANN401 - Compose value return service -def _as_mapping(key: str, name: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped +def _as_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped + """Normalize a structural mapping-merge key's value to a mapping. + + Deliberately stricter than `keys.pairs_to_mapping`: list form is accepted + only for `environment` and `depends_on`, the two keys Compose actually + defines a list form for. `extra_hosts`/`healthcheck` in list form on a + merged side are refused as an incompatible form rather than coerced. + """ if isinstance(value, dict): return value if isinstance(value, list): @@ -55,15 +62,6 @@ def _as_mapping(key: str, name: str, value: Any) -> dict[str, Any]: # noqa: ANN raise UnsupportedComposeError(msg) -def _as_list(key: str, name: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped - if isinstance(value, list): - return list(value) - if isinstance(value, str): - return [value] - msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" - raise UnsupportedComposeError(msg) - - def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str, Any]: """Merge `local` onto `base` per key category: mapping-merge, sequence-concat, else override.""" merged: dict[str, Any] = dict(base) @@ -72,9 +70,9 @@ def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str, if key in base and spec is not None and spec.merge is not None: merged[key] = spec.merge(name, key, base[key], local_val) elif key in base and key in _STRUCTURAL_MERGE_KEYS: - merged[key] = {**_as_mapping(key, name, base[key]), **_as_mapping(key, name, local_val)} + merged[key] = {**_as_mapping(name, key, base[key]), **_as_mapping(name, key, local_val)} elif key in base and key in _STRUCTURAL_CONCAT_KEYS: - merged[key] = _as_list(key, name, base[key]) + _as_list(key, name, local_val) + merged[key] = concat_list(name, key, base[key], local_val) else: merged[key] = local_val return merged diff --git a/compose2pod/keys.py b/compose2pod/keys.py index 769c05c..c03617f 100644 --- a/compose2pod/keys.py +++ b/compose2pod/keys.py @@ -147,7 +147,7 @@ def validate_map(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Com raise UnsupportedComposeError(msg) -def _as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON +def as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON """Normalize list-or-scalar-string form to a list, for merging across extends.""" if isinstance(value, list): return list(value) @@ -157,9 +157,9 @@ def _as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Co raise UnsupportedComposeError(msg) -def _concat_list(name: str, key: str, base: Any, local: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON +def concat_list(name: str, key: str, base: Any, local: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON """Merge policy for list-shaped keys: concatenate base then local.""" - return _as_list(name, key, base) + _as_list(name, key, local) + return as_list(name, key, base) + as_list(name, key, local) def pairs_to_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON @@ -212,7 +212,7 @@ def emit(value: Any) -> list[Token]: # noqa: ANN401 - Compose values are untype tokens += [flag, Expand(value=str(item))] return tokens - return KeySpec(validate=_validate_list, emit=emit, merge=_concat_list) + return KeySpec(validate=_validate_list, emit=emit, merge=concat_list) def _map(flag: str) -> KeySpec: diff --git a/planning/changes/2026-07-14.03-extends-keys-dedup.md b/planning/changes/2026-07-14.03-extends-keys-dedup.md new file mode 100644 index 0000000..136a620 --- /dev/null +++ b/planning/changes/2026-07-14.03-extends-keys-dedup.md @@ -0,0 +1,89 @@ +--- +summary: extends.py drops its private copy of keys.py's list-normalizing helper and its inline concat, using the promoted keys.concat_list instead; the two byte-identical helpers with swapped parameters are gone. +--- + +# Design: Collapse extends' duplicate merge helpers onto keys.py + +## Summary + +`extends.py` carries a private `_as_list` that is byte-identical to `keys.py`'s +`_as_list` — same body, same error message — **with the first two parameters +swapped**. Delete the copy, promote the `keys.py` original to the module's +public primitive set, and have `extends` use it. + +## Motivation + +The two functions today: + +```python +# compose2pod/extends.py +def _as_list(key: str, name: str, value: Any) -> list[Any]: + if isinstance(value, list): + return list(value) + if isinstance(value, str): + return [value] + msg = f"service {name!r}: cannot merge {key!r} across incompatible forms" + raise UnsupportedComposeError(msg) + +# compose2pod/keys.py +def _as_list(name: str, key: str, value: Any) -> list[Any]: + ...identical body, identical message... +``` + +Same name, same behavior, opposite parameter order. Each call site is correct +only because it happens to match its own module's local signature — nothing +catches a mix-up but the error message coming out with `name` and `key` +transposed, which no test asserts on. This is a latent footgun sitting in the +`extends` merge path, and `extends` runs *ahead of the gate* +(`cli.py` calls `resolve_extends()` before `validate()`), which is precisely +where this codebase has already been bitten. + +`extends._merge` also open-codes the same two merge policies `keys.py` already +names: `_as_list(base) + _as_list(local)` is `keys._concat_list`. + +## Design + +`keys.py` is already the home of the cross-module primitives — `key_value_pairs`, +`pairs_to_mapping`, `validate_map`, `require_string_keys`, `extra_host_pairs`, +`is_number` — all on the same `(name, key, value)` signature +(`2026-07-13.07-public-keys-primitives`). Two more join them: + +- `_as_list` → **`as_list(name, key, value)`** +- `_concat_list` → **`concat_list(name, key, base, local)`** + +`extends.py` then deletes its `_as_list` and calls `concat_list` for its +sequence-concatenate keys. One definition, one parameter order. + +**`extends._as_mapping` stays.** It is *not* duplication: it is deliberately +stricter than `keys.pairs_to_mapping`, accepting list form only for +`environment` and `depends_on` and refusing it for `extra_hosts`/`healthcheck` +rather than coercing. Collapsing it onto `pairs_to_mapping` would silently start +coercing list-form `extra_hosts` on a merged side — a behavior change, not a +dedup. Its parameter order is corrected to `(name, key, value)` to match every +other helper, removing the second half of the footgun. + +No structural-key registry (`decisions/2026-07-12-reject-structural-key-registry.md` +stands): this moves two helpers, it does not build a dispatch table. + +## Non-goals + +- **No behavior change.** Every accepted document still merges identically and + every rejected one still raises the same message. This is a pure refactor; if + a test needs changing, the refactor is wrong. +- Not unifying structural-key merge policy — the asymmetry where a registry key + (`labels`) coerces list form on merge while a structural key (`extra_hosts`) + refuses it is real, but it is a *policy* question, not a duplication one, and + it stays deferred behind `decisions/2026-07-12`'s revisit trigger. + +## Testing + +`just test-ci` at 100%, unchanged. The existing `tests/test_extends.py` merge +suite is the regression net: a pure refactor must leave all of it green without +edits. Add one test pinning the error message's `name`/`key` order, so a future +transposition fails loudly rather than silently producing a garbled message. + +## Risk + +- **A silent parameter transposition during the edit** — exactly the bug being + removed. Mitigated by the message-order test above and by the existing merge + suite, which covers both the concat and the incompatible-form paths. diff --git a/tests/test_extends.py b/tests/test_extends.py index a70c217..51021ec 100644 --- a/tests/test_extends.py +++ b/tests/test_extends.py @@ -305,3 +305,32 @@ def test_non_string_key_merged_into_environment_across_extends_passes_through(se } merged = resolve_extends(doc) assert merged["services"]["app"]["environment"] == {"A": "1", 3: "x"} + + +class TestMergeErrorMessageOrder: + """The service name and the key must not be transposed in a merge error.""" + + def test_structural_concat_incompatible_form_names_service_then_key(self) -> None: + # 'volumes' is a structural concat key: it goes through extends' own + # list-normalizing path, not a KeySpec.merge. A swapped (name, key) would + # render "service 'volumes': cannot merge 'app'" and go unnoticed. + doc = { + "services": { + "base": {"image": "x", "volumes": ["./a:/a"]}, + "app": {"extends": {"service": "base"}, "volumes": {"not": "a list"}}, + } + } + with pytest.raises(UnsupportedComposeError) as excinfo: + resolve_extends(doc) + assert str(excinfo.value) == "service 'app': cannot merge 'volumes' across incompatible forms" + + def test_structural_concat_merges_scalar_and_list_forms(self) -> None: + # The normalize-then-concat path itself: a scalar string on one side. + doc = { + "services": { + "base": {"image": "x", "env_file": "base.env"}, + "app": {"extends": {"service": "base"}, "env_file": ["local.env"]}, + } + } + merged = resolve_extends(doc) + assert merged["services"]["app"]["env_file"] == ["base.env", "local.env"] diff --git a/tests/test_keys.py b/tests/test_keys.py index 0382e57..99915ed 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -5,10 +5,10 @@ SERVICE_KEYS, STRUCTURAL_KEYS, Expand, - _concat_list, _merge_map, _validate_list, _validate_ulimits, + concat_list, pairs_to_mapping, require_string_keys, validate_map, @@ -167,19 +167,19 @@ class TestMergeCallables: extends.py (Task 2) will call these through SERVICE_KEYS[key].merge, but that wiring doesn't exist yet — these tests exercise every branch of - _concat_list/_as_list/_merge_map/pairs_to_mapping on their own so Task 1 + concat_list/as_list/_merge_map/pairs_to_mapping on their own so Task 1 is fully covered without depending on Task 2. """ def test_concat_list_merges_list_forms(self) -> None: - assert _concat_list("web", "cap_add", ["NET_ADMIN"], ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"] + assert concat_list("web", "cap_add", ["NET_ADMIN"], ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"] def test_concat_list_normalizes_scalar_string_form(self) -> None: - assert _concat_list("web", "cap_add", "NET_ADMIN", ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"] + assert concat_list("web", "cap_add", "NET_ADMIN", ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"] def test_concat_list_refuses_incompatible_form(self) -> None: with pytest.raises(UnsupportedComposeError, match="cannot merge 'cap_add' across incompatible forms"): - _concat_list("web", "cap_add", ["NET_ADMIN"], {"bad": "shape"}) + concat_list("web", "cap_add", ["NET_ADMIN"], {"bad": "shape"}) def test_merge_map_merges_dict_forms(self) -> None: assert _merge_map("web", "labels", {"team": "core"}, {"tier": "web"}) == {"team": "core", "tier": "web"}