Skip to content

Commit 274ae59

Browse files
authored
refactor: collapse extends' duplicate list helper onto keys.py (#56)
extends._as_list was byte-identical to keys._as_list -- same body, same error message -- with the first two parameters swapped. Each call site was correct only because it matched its own module's local signature; a transposition would have produced a garbled message and nothing would have caught it. keys.py's _as_list/_concat_list join its public cross-module primitives as as_list/concat_list, on the same (name, key, value) signature as the rest. extends drops its copy and its inline concat. extends._as_mapping stays: it is deliberately stricter than pairs_to_mapping, accepting list form only for environment and depends_on, so collapsing it would silently start coercing list-form extra_hosts. Its parameter order is corrected to match. Behavior-preserving: the emitted script for a document exercising every merge path is byte-identical.
1 parent f7a3d45 commit 274ae59

5 files changed

Lines changed: 138 additions & 22 deletions

File tree

compose2pod/extends.py

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from typing import Any
44

55
from compose2pod.exceptions import UnsupportedComposeError
6-
from compose2pod.keys import SERVICE_KEYS, pairs_to_mapping
6+
from compose2pod.keys import SERVICE_KEYS, concat_list, pairs_to_mapping
77

88

99
# 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
3939
return service
4040

4141

42-
def _as_mapping(key: str, name: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped
42+
def _as_mapping(name: str, key: str, value: Any) -> dict[str, Any]: # noqa: ANN401 - Compose values are untyped
43+
"""Normalize a structural mapping-merge key's value to a mapping.
44+
45+
Deliberately stricter than `keys.pairs_to_mapping`: list form is accepted
46+
only for `environment` and `depends_on`, the two keys Compose actually
47+
defines a list form for. `extra_hosts`/`healthcheck` in list form on a
48+
merged side are refused as an incompatible form rather than coerced.
49+
"""
4350
if isinstance(value, dict):
4451
return value
4552
if isinstance(value, list):
@@ -55,15 +62,6 @@ def _as_mapping(key: str, name: str, value: Any) -> dict[str, Any]: # noqa: ANN
5562
raise UnsupportedComposeError(msg)
5663

5764

58-
def _as_list(key: str, name: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped
59-
if isinstance(value, list):
60-
return list(value)
61-
if isinstance(value, str):
62-
return [value]
63-
msg = f"service {name!r}: cannot merge {key!r} across incompatible forms"
64-
raise UnsupportedComposeError(msg)
65-
66-
6765
def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str, Any]:
6866
"""Merge `local` onto `base` per key category: mapping-merge, sequence-concat, else override."""
6967
merged: dict[str, Any] = dict(base)
@@ -72,9 +70,9 @@ def _merge(base: dict[str, Any], local: dict[str, Any], name: str) -> dict[str,
7270
if key in base and spec is not None and spec.merge is not None:
7371
merged[key] = spec.merge(name, key, base[key], local_val)
7472
elif key in base and key in _STRUCTURAL_MERGE_KEYS:
75-
merged[key] = {**_as_mapping(key, name, base[key]), **_as_mapping(key, name, local_val)}
73+
merged[key] = {**_as_mapping(name, key, base[key]), **_as_mapping(name, key, local_val)}
7674
elif key in base and key in _STRUCTURAL_CONCAT_KEYS:
77-
merged[key] = _as_list(key, name, base[key]) + _as_list(key, name, local_val)
75+
merged[key] = concat_list(name, key, base[key], local_val)
7876
else:
7977
merged[key] = local_val
8078
return merged

compose2pod/keys.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ def validate_map(name: str, key: str, value: Any) -> None: # noqa: ANN401 - Com
147147
raise UnsupportedComposeError(msg)
148148

149149

150-
def _as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON
150+
def as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON
151151
"""Normalize list-or-scalar-string form to a list, for merging across extends."""
152152
if isinstance(value, list):
153153
return list(value)
@@ -157,9 +157,9 @@ def _as_list(name: str, key: str, value: Any) -> list[Any]: # noqa: ANN401 - Co
157157
raise UnsupportedComposeError(msg)
158158

159159

160-
def _concat_list(name: str, key: str, base: Any, local: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON
160+
def concat_list(name: str, key: str, base: Any, local: Any) -> list[Any]: # noqa: ANN401 - Compose values are untyped YAML/JSON
161161
"""Merge policy for list-shaped keys: concatenate base then local."""
162-
return _as_list(name, key, base) + _as_list(name, key, local)
162+
return as_list(name, key, base) + as_list(name, key, local)
163163

164164

165165
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
212212
tokens += [flag, Expand(value=str(item))]
213213
return tokens
214214

215-
return KeySpec(validate=_validate_list, emit=emit, merge=_concat_list)
215+
return KeySpec(validate=_validate_list, emit=emit, merge=concat_list)
216216

217217

218218
def _map(flag: str) -> KeySpec:
Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
---
2+
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.
3+
---
4+
5+
# Design: Collapse extends' duplicate merge helpers onto keys.py
6+
7+
## Summary
8+
9+
`extends.py` carries a private `_as_list` that is byte-identical to `keys.py`'s
10+
`_as_list` — same body, same error message — **with the first two parameters
11+
swapped**. Delete the copy, promote the `keys.py` original to the module's
12+
public primitive set, and have `extends` use it.
13+
14+
## Motivation
15+
16+
The two functions today:
17+
18+
```python
19+
# compose2pod/extends.py
20+
def _as_list(key: str, name: str, value: Any) -> list[Any]:
21+
if isinstance(value, list):
22+
return list(value)
23+
if isinstance(value, str):
24+
return [value]
25+
msg = f"service {name!r}: cannot merge {key!r} across incompatible forms"
26+
raise UnsupportedComposeError(msg)
27+
28+
# compose2pod/keys.py
29+
def _as_list(name: str, key: str, value: Any) -> list[Any]:
30+
...identical body, identical message...
31+
```
32+
33+
Same name, same behavior, opposite parameter order. Each call site is correct
34+
only because it happens to match its own module's local signature — nothing
35+
catches a mix-up but the error message coming out with `name` and `key`
36+
transposed, which no test asserts on. This is a latent footgun sitting in the
37+
`extends` merge path, and `extends` runs *ahead of the gate*
38+
(`cli.py` calls `resolve_extends()` before `validate()`), which is precisely
39+
where this codebase has already been bitten.
40+
41+
`extends._merge` also open-codes the same two merge policies `keys.py` already
42+
names: `_as_list(base) + _as_list(local)` is `keys._concat_list`.
43+
44+
## Design
45+
46+
`keys.py` is already the home of the cross-module primitives — `key_value_pairs`,
47+
`pairs_to_mapping`, `validate_map`, `require_string_keys`, `extra_host_pairs`,
48+
`is_number` — all on the same `(name, key, value)` signature
49+
(`2026-07-13.07-public-keys-primitives`). Two more join them:
50+
51+
- `_as_list`**`as_list(name, key, value)`**
52+
- `_concat_list`**`concat_list(name, key, base, local)`**
53+
54+
`extends.py` then deletes its `_as_list` and calls `concat_list` for its
55+
sequence-concatenate keys. One definition, one parameter order.
56+
57+
**`extends._as_mapping` stays.** It is *not* duplication: it is deliberately
58+
stricter than `keys.pairs_to_mapping`, accepting list form only for
59+
`environment` and `depends_on` and refusing it for `extra_hosts`/`healthcheck`
60+
rather than coercing. Collapsing it onto `pairs_to_mapping` would silently start
61+
coercing list-form `extra_hosts` on a merged side — a behavior change, not a
62+
dedup. Its parameter order is corrected to `(name, key, value)` to match every
63+
other helper, removing the second half of the footgun.
64+
65+
No structural-key registry (`decisions/2026-07-12-reject-structural-key-registry.md`
66+
stands): this moves two helpers, it does not build a dispatch table.
67+
68+
## Non-goals
69+
70+
- **No behavior change.** Every accepted document still merges identically and
71+
every rejected one still raises the same message. This is a pure refactor; if
72+
a test needs changing, the refactor is wrong.
73+
- Not unifying structural-key merge policy — the asymmetry where a registry key
74+
(`labels`) coerces list form on merge while a structural key (`extra_hosts`)
75+
refuses it is real, but it is a *policy* question, not a duplication one, and
76+
it stays deferred behind `decisions/2026-07-12`'s revisit trigger.
77+
78+
## Testing
79+
80+
`just test-ci` at 100%, unchanged. The existing `tests/test_extends.py` merge
81+
suite is the regression net: a pure refactor must leave all of it green without
82+
edits. Add one test pinning the error message's `name`/`key` order, so a future
83+
transposition fails loudly rather than silently producing a garbled message.
84+
85+
## Risk
86+
87+
- **A silent parameter transposition during the edit** — exactly the bug being
88+
removed. Mitigated by the message-order test above and by the existing merge
89+
suite, which covers both the concat and the incompatible-form paths.

tests/test_extends.py

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,3 +305,32 @@ def test_non_string_key_merged_into_environment_across_extends_passes_through(se
305305
}
306306
merged = resolve_extends(doc)
307307
assert merged["services"]["app"]["environment"] == {"A": "1", 3: "x"}
308+
309+
310+
class TestMergeErrorMessageOrder:
311+
"""The service name and the key must not be transposed in a merge error."""
312+
313+
def test_structural_concat_incompatible_form_names_service_then_key(self) -> None:
314+
# 'volumes' is a structural concat key: it goes through extends' own
315+
# list-normalizing path, not a KeySpec.merge. A swapped (name, key) would
316+
# render "service 'volumes': cannot merge 'app'" and go unnoticed.
317+
doc = {
318+
"services": {
319+
"base": {"image": "x", "volumes": ["./a:/a"]},
320+
"app": {"extends": {"service": "base"}, "volumes": {"not": "a list"}},
321+
}
322+
}
323+
with pytest.raises(UnsupportedComposeError) as excinfo:
324+
resolve_extends(doc)
325+
assert str(excinfo.value) == "service 'app': cannot merge 'volumes' across incompatible forms"
326+
327+
def test_structural_concat_merges_scalar_and_list_forms(self) -> None:
328+
# The normalize-then-concat path itself: a scalar string on one side.
329+
doc = {
330+
"services": {
331+
"base": {"image": "x", "env_file": "base.env"},
332+
"app": {"extends": {"service": "base"}, "env_file": ["local.env"]},
333+
}
334+
}
335+
merged = resolve_extends(doc)
336+
assert merged["services"]["app"]["env_file"] == ["base.env", "local.env"]

tests/test_keys.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
SERVICE_KEYS,
66
STRUCTURAL_KEYS,
77
Expand,
8-
_concat_list,
98
_merge_map,
109
_validate_list,
1110
_validate_ulimits,
11+
concat_list,
1212
pairs_to_mapping,
1313
require_string_keys,
1414
validate_map,
@@ -167,19 +167,19 @@ class TestMergeCallables:
167167
168168
extends.py (Task 2) will call these through SERVICE_KEYS[key].merge, but
169169
that wiring doesn't exist yet — these tests exercise every branch of
170-
_concat_list/_as_list/_merge_map/pairs_to_mapping on their own so Task 1
170+
concat_list/as_list/_merge_map/pairs_to_mapping on their own so Task 1
171171
is fully covered without depending on Task 2.
172172
"""
173173

174174
def test_concat_list_merges_list_forms(self) -> None:
175-
assert _concat_list("web", "cap_add", ["NET_ADMIN"], ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"]
175+
assert concat_list("web", "cap_add", ["NET_ADMIN"], ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"]
176176

177177
def test_concat_list_normalizes_scalar_string_form(self) -> None:
178-
assert _concat_list("web", "cap_add", "NET_ADMIN", ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"]
178+
assert concat_list("web", "cap_add", "NET_ADMIN", ["SYS_TIME"]) == ["NET_ADMIN", "SYS_TIME"]
179179

180180
def test_concat_list_refuses_incompatible_form(self) -> None:
181181
with pytest.raises(UnsupportedComposeError, match="cannot merge 'cap_add' across incompatible forms"):
182-
_concat_list("web", "cap_add", ["NET_ADMIN"], {"bad": "shape"})
182+
concat_list("web", "cap_add", ["NET_ADMIN"], {"bad": "shape"})
183183

184184
def test_merge_map_merges_dict_forms(self) -> None:
185185
assert _merge_map("web", "labels", {"team": "core"}, {"tier": "web"}) == {"team": "core", "tier": "web"}

0 commit comments

Comments
 (0)