Skip to content

Commit db47ee5

Browse files
jawwad-aliclaude
andcommitted
fix(bundler): normalize the catalog entry id and cover the nested version field
Addresses two review comments. 1. `CatalogEntry.from_dict` still computed `entry_id` with `str(data.get("id", ""))`, so an explicit `id: null` remained the literal "None". That is truthy, so `load_catalog_payload` compared it against the mapping key and reported the wrong error: before: Catalog entry id mismatch: key 'demo' != entry id 'None'. after : Catalog entry for 'demo' is missing its 'id' field. Now routed through `_text` like every other text field. 2. `requires.speckit_version` was switched to `_text` in this PR but only the seven top-level attributes were tested, so that branch could regress while the suite still passed. Added a dedicated case. Mutation-verified: reverting only `entry_id` fails only the null-id test, and reverting only `requires_speckit_version` fails only the nested-field test -- each pins its own branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 92d5971 commit db47ee5

2 files changed

Lines changed: 39 additions & 1 deletion

File tree

‎src/specify_cli/bundler/models/catalog.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,11 @@ class CatalogEntry:
170170
def from_dict(cls, data: Any) -> "CatalogEntry":
171171
if not isinstance(data, dict):
172172
raise BundlerError("Each catalog entry must be a mapping.")
173-
entry_id = str(data.get("id", "")).strip()
173+
# ``_text`` here too: an ``id: null`` otherwise became the literal
174+
# "None", which is truthy, so ``load_catalog_payload`` reported it as an
175+
# id MISMATCH against the mapping key rather than the accurate
176+
# missing-id error.
177+
entry_id = _text(data.get("id"))
174178
# `or {}` would coerce a FALSY non-mapping (0, '', False, []) to {} before
175179
# the isinstance guard, silently accepting a corrupt catalog entry; only
176180
# an absent/None value means "not present".

‎tests/contract/test_catalog_schema.py‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,3 +404,37 @@ def test_catalog_entry_rejects_falsy_non_mapping(field, bad):
404404
data[field] = bad
405405
with pytest.raises(BundlerError, match=f"'{field}' must be a mapping"):
406406
CatalogEntry.from_dict(data)
407+
408+
409+
def test_catalog_entry_explicit_null_id_reports_missing_id():
410+
"""`id: null` must surface as the missing-id error, not an id mismatch.
411+
412+
`entry_id` was still computed with `str(data.get("id", ""))`, so an explicit
413+
null became the literal "None". That is truthy, so `load_catalog_payload`
414+
compared it against the mapping key and reported
415+
"id mismatch: key 'demo' != entry id 'None'" instead of the accurate
416+
missing-id error.
417+
"""
418+
from specify_cli.bundler.models.catalog import CatalogEntry
419+
420+
data = catalog_entry_dict("demo")
421+
data["id"] = None
422+
assert CatalogEntry.from_dict(data).id == ""
423+
424+
with pytest.raises(BundlerError, match="missing its 'id' field"):
425+
load_catalog_payload(catalog_payload({"demo": data}))
426+
427+
428+
def test_catalog_entry_explicit_null_requires_speckit_version_reads_as_empty():
429+
"""The nested `requires.speckit_version` branch is covered too.
430+
431+
It was switched to `_text` alongside the top-level fields, but only the
432+
top-level attributes were exercised — so this branch could regress while
433+
the suite still passed.
434+
"""
435+
from specify_cli.bundler.models.catalog import CatalogEntry
436+
437+
data = catalog_entry_dict("demo")
438+
data["requires"] = {"speckit_version": None}
439+
440+
assert CatalogEntry.from_dict(data).requires_speckit_version == ""

0 commit comments

Comments
 (0)