From 2b2d5ee0a704dbb63dfb887bf2a111c3f7c66401 Mon Sep 17 00:00:00 2001 From: viewit Date: Tue, 4 Aug 2026 15:31:57 +0200 Subject: [PATCH] fix(filaments): guard against non-string profile names, log name collisions Found during a targeted code review, not from a user report. parse_profile()'s `name` field went straight through clean_name() without routing through first_str() first, unlike filament_vendor/filament_type/ default_filament_colour right below it - all of which handle the documented case where OrcaSlicer stores a field as ["value"] instead of a plain string. If `data["name"]` was ever a list, clean_name()'s re.sub() raised TypeError since it requires a string argument. Fixed by routing it through first_str() like its neighbors. Also added a debug log when sys_by_name (the system-profile lookup index) overwrites an entry due to a name collision - clean_name() deliberately collapses variant-suffixed profile names (e.g. "...@base" vs "...@Anycubic Kobra X 0.4 nozzle") onto the same cleaned name, so a collision is expected, but the resulting last-write-wins overwrite was previously silent, making an unexpected inherits-parent resolution hard to debug. New tests in tests/test_orca_filaments_parser.py add the first dedicated coverage for parse_profile()/parse_profile_bytes()/clean_name() - previous tests only used pre-parsed profile dicts as fixtures and never exercised the parsing logic itself. --- NIGHTLY_CHANGELOG.md | 1 + orca_filaments.py | 17 ++- tests/test_orca_filaments_parser.py | 182 ++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 tests/test_orca_filaments_parser.py diff --git a/NIGHTLY_CHANGELOG.md b/NIGHTLY_CHANGELOG.md index 2a9141e..833a009 100644 --- a/NIGHTLY_CHANGELOG.md +++ b/NIGHTLY_CHANGELOG.md @@ -3,3 +3,4 @@ - Fix: **combined ACE-RFID filament tags (e.g. "GEEETECH PLA Bas" from third-party RFID tools) still weren't auto-matching to imported OrcaSlicer profiles**, even after the first attempt at this in a previous nightly — the matching logic was only ever wired into the OrcaSlicer slicer-sync endpoint, never into the actual MQTT status path that feeds the dashboard and Happy-Hare gate data, so the dashboard kept showing the raw unmatched RFID string. Centralized the matching so all three places that resolve a slot's filament profile benefit identically. Also added variant-token disambiguation (e.g. "Bas" vs. "Matte") for when a vendor has multiple profiles of the same material, and fixed a related edge case where a manual per-slot override could be incorrectly treated as stale on an RFID-tagged spool (Issue #101, thanks @Blaim for the extensive debugging that pinpointed this). - Feat: new setting under Settings → Print — "Delete file from printer after successful print" — automatically removes a GCode file from the printer's own storage once it finishes printing successfully, keeping only the copy in the bridge's own GCode store. Off by default, and only ever applies to files that were uploaded through the bridge itself (so there's always a backup); files started directly from the printer or Anycubic Slicer are never touched. - Fix: **the dashboard could stay stuck showing a printer as online/"ready" indefinitely after it was physically switched off or unplugged**, discovered while testing the new smart-plug power-switch feature. Root cause was two-fold: the MQTT socket had no TCP keepalive, so a connection killed without a clean close (unplugged, not a graceful shutdown) could look alive to the OS for 15+ minutes; and even once the dead connection was detected, the status poll loop could get stuck waiting on a reconnect attempt that was already running elsewhere, so it never reached the code that flips the dashboard to "offline". Live-tested against a real printer, including that no sockets, threads, or file descriptors are left behind across repeated disconnect/reconnect cycles — a disconnected printer is now detected and reflected on the dashboard within about 15 seconds. +- Fix: a range of smaller robustness issues found in an internal code review — a single malformed MQTT message from the printer could get permanently stuck at the front of the receive buffer and force a reconnect on every subsequent poll; concurrent requests of the same type could occasionally have their responses mixed up; the camera stream could leak an orphaned ffmpeg process after a printer reboot rotated its stream URL while a new stream was already starting; `/api/settings` and `/api/update/apply` returned an unhandled server error instead of a clean "invalid request" for a malformed request body; a typo'd numeric value in `config.ini` (e.g. a stray character in the port number) could prevent the bridge from starting at all instead of falling back to the default; and a rare filament-profile-name collision during import is now logged instead of silently resolved. None of these were reported as user-facing bugs — added as defense-in-depth after a targeted review, with new tests covering each case. 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