diff --git a/orca_filaments.py b/orca_filaments.py index c56a591..0afc001 100644 --- a/orca_filaments.py +++ b/orca_filaments.py @@ -9,8 +9,11 @@ them as a normalized list with (id, name, vendor, type, color). from __future__ import annotations import json +import logging import re +log = logging.getLogger("kobrax.filaments") + def first_str(value, default: str = "") -> str: """Orca profiles store some fields as ['value']. Returns the first @@ -69,7 +72,17 @@ def parse_profile(data: dict, by_name: dict | None = None, if system_index: for p in system_index: if isinstance(p, dict) and p.get("name"): - sys_by_name[p["name"]] = p + pname = p["name"] + if pname in sys_by_name and sys_by_name[pname] is not p: + # clean_name() deliberately collapses variant-suffixed + # names (e.g. "...@base" vs "...@Anycubic Kobra X 0.4 + # nozzle") onto the same cleaned name - expected, but the + # last-write-wins overwrite here was previously silent, + # making an unexpected inherits-parent resolution hard to + # debug. + log.debug("orca_filaments: duplicate system profile name %r - " + "overwriting %r with %r", pname, sys_by_name[pname].get("id"), p.get("id")) + sys_by_name[pname] = p def _resolve(key: str, depth: int = 5): cur_list = [data] @@ -119,7 +132,7 @@ def parse_profile(data: dict, by_name: dict | None = None, if not fid or not isinstance(fid, str): return None - name_raw = data.get("name", fid) + name_raw = first_str(data.get("name"), fid) name = clean_name(name_raw) vendor = first_str(_resolve_full("filament_vendor")) or (path_vendor or "Generic") ftype = first_str(_resolve_full("filament_type"), "") diff --git a/tests/test_orca_filaments_parser.py b/tests/test_orca_filaments_parser.py new file mode 100644 index 0000000..7b12b4a --- /dev/null +++ b/tests/test_orca_filaments_parser.py @@ -0,0 +1,182 @@ +"""orca_filaments.py parser robustness (code review finding). + +No dedicated test file existed for parse_profile()/parse_profile_bytes()/ +clean_name() before this - existing tests only used pre-parsed profile dicts +as fixtures, never exercised the actual parsing logic. +""" +import json +import logging + +from orca_filaments import clean_name, first_str, parse_profile, parse_profile_bytes + + +def test_clean_name_strips_base_suffix(): + assert clean_name("PolyTerra PLA @base") == "PolyTerra PLA" + + +def test_clean_name_strips_printer_and_nozzle_suffix(): + assert clean_name("Anycubic PLA @Anycubic Kobra X 0.4 nozzle") == "Anycubic PLA" + + +def test_clean_name_strips_bare_nozzle_suffix(): + assert clean_name("Anker Generic PLA 0.4 nozzle") == "Anker Generic PLA" + + +def test_clean_name_returns_raw_when_stripping_leaves_nothing(): + """An all-suffix name has nothing left after stripping - falls back to + the original raw string rather than returning an empty string.""" + assert clean_name("@base") == "@base" + + +def test_first_str_unwraps_single_element_list(): + assert first_str(["PLA"]) == "PLA" + + +def test_first_str_passes_through_plain_string(): + assert first_str("PLA") == "PLA" + + +def test_first_str_returns_default_for_empty_list(): + assert first_str([], "fallback") == "fallback" + + +def test_first_str_returns_default_for_other_types(): + assert first_str(42, "fallback") == "fallback" + assert first_str(None, "fallback") == "fallback" + + +def test_parse_profile_rejects_non_dict(): + assert parse_profile([1, 2, 3]) is None + assert parse_profile("not a dict") is None + assert parse_profile(None) is None + + +def test_parse_profile_rejects_stub_without_id_or_parent(): + data = {"type": "filament"} # no inherits, no filament_id + assert parse_profile(data) is None + + +def test_parse_profile_rejects_instantiation_false(): + data = {"type": "filament", "filament_id": "GFL01", "instantiation": "false"} + assert parse_profile(data) is None + + +def test_parse_profile_minimal_valid_profile(): + data = { + "type": "filament", + "filament_id": "GFL01", + "name": "Generic PLA", + "filament_vendor": ["Generic"], + "filament_type": ["PLA"], + "default_filament_colour": ["#FFFFFF"], + } + result = parse_profile(data) + assert result == { + "id": "GFL01", + "name": "Generic PLA", + "vendor": "Generic", + "type": "PLA", + "color": "#FFFFFF", + } + + +def test_parse_profile_name_as_list_does_not_crash(): + """Regression guard: `name` wasn't previously routed through first_str() + like the other fields are, unlike filament_vendor/filament_type/ + default_filament_colour just below it - a list value here used to raise + TypeError inside clean_name()'s re.sub().""" + data = { + "type": "filament", + "filament_id": "GFL02", + "name": ["Geeetech PLA Basic"], + "filament_vendor": ["Geeetech"], + "filament_type": ["PLA"], + } + result = parse_profile(data) + assert result is not None + assert result["name"] == "Geeetech PLA Basic" + + +def test_parse_profile_missing_name_falls_back_to_filament_id(): + data = {"type": "filament", "filament_id": "GFL03", "filament_vendor": ["Generic"]} + result = parse_profile(data) + assert result["name"] == "GFL03" + + +def test_parse_profile_missing_optional_fields_default_to_empty_string(): + data = {"type": "filament", "filament_id": "GFL04", "name": "Mystery Filament"} + result = parse_profile(data) + assert result["type"] == "" + assert result["color"] == "" + assert result["vendor"] == "Generic" # no path_vendor given either + + +def test_parse_profile_inherits_via_by_name(): + parent = {"type": "filament", "filament_id": "GFL05", "filament_vendor": ["Geeetech"], "filament_type": ["PLA"]} + child = {"type": "filament", "inherits": "Geeetech PLA @base", "name": "Geeetech PLA Basic"} + by_name = {"Geeetech PLA @base": [parent]} + result = parse_profile(child, by_name=by_name) + assert result is not None + assert result["id"] == "GFL05" + assert result["vendor"] == "Geeetech" + assert result["type"] == "PLA" + + +def test_parse_profile_inherits_via_system_index(): + system_index = [{ + "id": "GFL06", "name": "Geeetech PLA", "vendor": "Geeetech", "type": "PLA", "color": "", + }] + user_profile = {"type": "filament", "inherits": "Geeetech PLA @base", "name": "My Geeetech Override"} + result = parse_profile(user_profile, system_index=system_index) + assert result is not None + assert result["id"] == "GFL06" + assert result["vendor"] == "Geeetech" + + +def test_parse_profile_inherits_cycle_does_not_infinite_loop(): + """A inherits B, B inherits A - _resolve()'s hard depth=5 bound must + terminate this rather than recursing forever.""" + a = {"type": "filament", "inherits": "B"} + b = {"type": "filament", "inherits": "A"} + by_name = {"A": [a], "B": [b]} + # Neither profile has a filament_id anywhere in the cycle - must return + # None (not hang, not crash) after exhausting the depth limit. + result = parse_profile(a, by_name=by_name) + assert result is None + + +def test_parse_profile_duplicate_system_names_logs_and_uses_last(caplog): + """clean_name() deliberately collapses variant-suffixed names onto the + same cleaned name - sys_by_name's last-write-wins overwrite on collision + is expected, but must now be observable via a debug log instead of + silent.""" + system_index = [ + {"id": "GFL07", "name": "PolyTerra PLA", "vendor": "Polymaker", "type": "PLA", "color": ""}, + {"id": "GFL08", "name": "PolyTerra PLA", "vendor": "Polymaker", "type": "PLA", "color": ""}, + ] + user_profile = {"type": "filament", "inherits": "PolyTerra PLA @base", "name": "Override"} + with caplog.at_level(logging.DEBUG, logger="kobrax.filaments"): + result = parse_profile(user_profile, system_index=system_index) + assert result is not None + assert result["id"] == "GFL08" # last one wins, as before + assert any("duplicate system profile name" in r.message for r in caplog.records) + + +def test_parse_profile_bytes_valid_json(): + blob = json.dumps({ + "type": "filament", "filament_id": "GFL09", "name": "Test PLA", + "filament_vendor": ["Test"], "filament_type": ["PLA"], + }).encode("utf-8") + result = parse_profile_bytes(blob) + assert result is not None + assert result["id"] == "GFL09" + + +def test_parse_profile_bytes_malformed_json_returns_none(): + assert parse_profile_bytes(b"{not valid json") is None + + +def test_parse_profile_bytes_non_dict_json_returns_none(): + assert parse_profile_bytes(b"[1, 2, 3]") is None + assert parse_profile_bytes(b'"just a string"') is None + assert parse_profile_bytes(b"42") is None