diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index 7a9b60a45..fa590fb16 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -113,6 +113,12 @@ class SpoolmanClient: # grows with spool count; scoped to the instance so a client pointed at # a different Spoolman starts over. self._ensured_extra_fields: set[str] = set() + # Whether Spoolman's extra-field listing has been read once. Separate + # from the set above because the two answer different questions: the + # set is "which fields are known to exist", this is "have we asked". + # Without it a client registering three brand-new fields re-read the + # whole listing before each one. + self._extra_fields_listed = False self._ensure_extra_lock = asyncio.Lock() async def _get_client(self) -> httpx.AsyncClient: @@ -953,6 +959,36 @@ class SpoolmanClient: """Register the 'tag' extra field in Spoolman if not present; returns True on success.""" return await self.ensure_extra_field("tag") + async def _load_existing_extra_field_keys(self) -> set[str] | None: + """Keys of the spool extra fields Spoolman already has, or ``None`` when + the listing could not be read. + + ``None`` and ``set()`` mean different things and the caller acts on the + difference: an empty set is "Spoolman has no extra fields", which means + every field Bambuddy needs must be created; ``None`` is "we could not + find out", where the only safe move is to fall back to attempting the + write blind. + """ + try: + client = await self._get_client() + response = await client.get(f"{self.api_url}/field/spool") + if response.status_code != 200: + logger.debug( + "Spoolman extra-field listing returned %s; falling back to blind registration", + response.status_code, + ) + return None + fields = response.json() + except Exception as e: # noqa: BLE001 — registration is best-effort, see _ensure_extra_fields + logger.debug("Could not read Spoolman extra-field listing: %s", e) + return None + if not isinstance(fields, list): + return None + # Match on `key`, not `name`: `key` is the identifier the extra dict is + # written under and the one Bambuddy cares about, while `name` is the + # free-text label a user is free to change in Spoolman's UI. + return {f["key"] for f in fields if isinstance(f, dict) and isinstance(f.get("key"), str)} + async def ensure_extra_field(self, name: str, field_type: str = "text") -> bool: """Register a custom extra field in Spoolman if not present. @@ -960,18 +996,40 @@ class SpoolmanClient: with HTTP 400 ('Unknown extra field .'), so any custom field Bambuddy persists alongside spools needs to be pre-registered. Idempotent — returns True if the field already exists. + + Existence is read from ``GET /field/spool``, the whole-listing endpoint. + This used to probe ``GET /field/spool/{name}`` for one field at a time, + which Spoolman has never served: it declares only POST and DELETE at + that path, so the probe answered 405 every time and the check could + never succeed (issue #2983, reported by @ngreatorex). + + Falling through to the POST on every call was worse than a wasted + request, because that endpoint is an upsert rather than a create. It + answered 200 whether or not the field was already there, so a field a + user had renamed, retyped or given a default in Spoolman's own UI was + silently reset to Bambuddy's version of it on every restart. Reading + the listing first is what lets an existing field be left alone. """ try: - client = await self._get_client() - - # Check if field already exists - response = await client.get(f"{self.api_url}/field/spool/{name}") - if response.status_code == 200: - logger.debug("Spoolman extra field %r already exists", name) - self._ensured_extra_fields.add(name) + if name in self._ensured_extra_fields: return True - # Field doesn't exist - create it + if not self._extra_fields_listed: + existing = await self._load_existing_extra_field_keys() + if existing is not None: + # Bank the whole listing: the caller registers several + # fields in a row, and each one it already has is a request + # not sent and a user customisation not overwritten. Read + # once per client — every field created after this point is + # added to the set as it is created, so re-reading would + # only ever confirm what we already know. + self._ensured_extra_fields |= existing + self._extra_fields_listed = True + if name in existing: + logger.debug("Spoolman extra field %r already exists", name) + return True + + client = await self._get_client() field_data = { "name": name, "field_type": field_type, diff --git a/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py b/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py index 0fc9f9389..3c005c0b1 100644 --- a/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py +++ b/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py @@ -49,12 +49,26 @@ class FakeSpoolman: self.log.append(f"{request.method} {path}") body = json.loads(request.content) if request.content else {} + if path == "/field/spool" and request.method == "GET": + if self.field_status != 200: + return httpx.Response(self.field_status) + return httpx.Response( + 200, + json=[ + {"key": name, "name": name, "field_type": "text", "entity_type": "spool"} + for name in sorted(self.registered) + ], + ) + if path.startswith("/field/spool/"): name = path.rsplit("/", 1)[-1] - if request.method == "GET": - if self.field_status != 200: - return httpx.Response(self.field_status) - return httpx.Response(200) if name in self.registered else httpx.Response(404) + # Spoolman declares only POST and DELETE here -- its own OpenAPI + # document says so, and the live server answers 405. Modelling this + # as a working existence probe is what let the bug in issue #2983 + # sit unnoticed: the check could never succeed, and the POST that + # followed silently overwrote whatever the user had customised. + if request.method != "POST": + return httpx.Response(405, json={"detail": "Method Not Allowed"}) if self.field_status != 200: return httpx.Response(self.field_status) self.registered.add(name) @@ -93,7 +107,9 @@ class FakeSpoolman: return httpx.Response(200, json=[]) def field_calls(self, name: str) -> list[str]: - return [entry for entry in self.log if entry.endswith(f"/field/spool/{name}")] + """Every request this client made about ``name``: the listing read that + answers "does it exist", plus any creation of that specific field.""" + return [entry for entry in self.log if entry == "GET /field/spool" or entry.endswith(f"/field/spool/{name}")] def _client(fake: FakeSpoolman) -> SpoolmanClient: @@ -167,7 +183,7 @@ class TestItAsksSpoolmanOnlyOnce: await client.create_spool(filament_id=7, extra={"tag": json.dumps("A")}) await client.create_spool(filament_id=7, extra={"tag": json.dumps("B")}) - assert fake.field_calls("tag") == ["GET /field/spool/tag", "POST /field/spool/tag"] + assert fake.field_calls("tag") == ["GET /field/spool", "POST /field/spool/tag"] @pytest.mark.asyncio async def test_an_already_registered_field_is_never_created(self): @@ -176,7 +192,7 @@ class TestItAsksSpoolmanOnlyOnce: await client.create_spool(filament_id=7, extra={"tag": json.dumps("A")}) - assert fake.field_calls("tag") == ["GET /field/spool/tag"] + assert fake.field_calls("tag") == ["GET /field/spool"] @pytest.mark.asyncio async def test_a_write_carrying_no_extra_asks_nothing(self): @@ -204,7 +220,7 @@ class TestItAsksSpoolmanOnlyOnce: client.create_spool(filament_id=7, extra={"tag": json.dumps("B")}), ) - assert fake.field_calls("tag") == ["GET /field/spool/tag", "POST /field/spool/tag"] + assert fake.field_calls("tag") == ["GET /field/spool", "POST /field/spool/tag"] @pytest.mark.asyncio async def test_another_client_does_not_inherit_the_answer(self): @@ -215,7 +231,7 @@ class TestItAsksSpoolmanOnlyOnce: second_fake = FakeSpoolman() await _client(second_fake).create_spool(filament_id=7, extra={"tag": json.dumps("B")}) - assert "GET /field/spool/tag" in second_fake.log + assert "GET /field/spool" in second_fake.log class TestEveryWritePathThatCarriesExtra: diff --git a/backend/tests/unit/services/test_spoolman_extra_field_registration_2983.py b/backend/tests/unit/services/test_spoolman_extra_field_registration_2983.py new file mode 100644 index 000000000..d0a3a0252 --- /dev/null +++ b/backend/tests/unit/services/test_spoolman_extra_field_registration_2983.py @@ -0,0 +1,235 @@ +"""Extra-field registration reads Spoolman's listing (#2983, reported by @ngreatorex). + +``ensure_extra_field`` used to probe ``GET /field/spool/{name}`` per field. +Spoolman declares only POST and DELETE at that path -- verified against its own +OpenAPI document -- so the probe answered 405 and the existence check could +never succeed. Every call fell through to ``POST /field/spool/{name}``, which is +an *upsert*: it answered 200 whether or not the field existed, so a field a user +had renamed or retyped in Spoolman's UI was reset to Bambuddy's version of it +on every client init, and an untrue "Created Spoolman extra field" was logged +each time. + +Existence now comes from ``GET /field/spool``, matched on ``key``. + +These drive ``ensure_extra_field``'s own branches directly, with the HTTP layer +stubbed, so the awkward answers can be posed one at a time -- a listing that +isn't a list, a row with no key, a transport error mid-read. +``test_spoolman_extra_field_registration_2903`` covers the other half from the +outside, through a fake Spoolman that enforces the real API's rules; its fake +answers the per-field path 405, as the real server does. +""" + +import json +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from backend.app.services.spoolman import SpoolmanClient + +BAMBU_FIELDS = ("tag", "bambu_slicer_filament", "bambu_slicer_filament_name", "bambu_color_name") + + +def _response(status: int, payload=None, text: str = "") -> MagicMock: + r = MagicMock() + r.status_code = status + r.text = text if payload is None else json.dumps(payload) + r.json = MagicMock(return_value=payload) + return r + + +def _field(key: str, name: str | None = None) -> dict: + """A Spoolman field row. `name` is the user-editable label, `key` the + identifier the spool's extra dict is written under.""" + return { + "key": key, + "name": name if name is not None else key, + "field_type": "text", + "entity_type": "spool", + "order": 0, + } + + +LISTING_URL = "http://spoolman.test/api/v1/field/spool" + + +def _client_with(listing, post_status: int = 200) -> tuple[SpoolmanClient, MagicMock]: + """A SpoolmanClient whose HTTP calls are recorded. + + ``listing`` is either the parsed body of ``GET /field/spool`` or a + ready-made response for the failure cases. + + GET is routed by URL rather than answering everything with the listing. + That matters: a mock that returns the listing for any GET also answers the + old per-field probe with it, which would let the pre-fix code pass these + tests. The real server answers ``GET /field/spool/{key}`` with **405**, + because it declares only POST and DELETE there -- so that is what any URL + other than the listing gets here. + """ + listing_response = listing if isinstance(listing, MagicMock) else _response(200, listing) + + async def get(url, *args, **kwargs): + if url == LISTING_URL: + return listing_response + return _response(405, None, '{"detail":"Method Not Allowed"}') + + http = MagicMock() + http.get = AsyncMock(side_effect=get) + http.post = AsyncMock(return_value=_response(post_status, {})) + client = SpoolmanClient("http://spoolman.test") + client._get_client = AsyncMock(return_value=http) + return client, http + + +class TestTheExistenceCheck: + @pytest.mark.asyncio + async def test_reads_the_listing_endpoint_not_the_per_field_one(self): + client, http = _client_with([_field("tag")]) + await client.ensure_extra_field("tag") + (url,), _ = http.get.call_args + assert url == "http://spoolman.test/api/v1/field/spool" + + @pytest.mark.asyncio + async def test_an_existing_field_is_never_posted(self): + """The whole point: POST is an upsert, so posting an existing field is + what reset the user's customisation of it.""" + client, http = _client_with([_field("tag", "Bambu RFID Tag")]) + assert await client.ensure_extra_field("tag") is True + http.post.assert_not_awaited() + + @pytest.mark.asyncio + async def test_matches_on_key_not_on_the_user_facing_name(self): + """A renamed field is still the same field. Matching on `name` would + make a rename look like a deletion and re-create it -- clobbering the + rename, which is exactly the reported behaviour.""" + client, http = _client_with([_field("bambu_color_name", "Bambu Colour")]) + assert await client.ensure_extra_field("bambu_color_name") is True + http.post.assert_not_awaited() + + @pytest.mark.asyncio + async def test_a_missing_field_is_created(self): + client, http = _client_with([_field("tag")]) + assert await client.ensure_extra_field("bambu_color_name") is True + http.post.assert_awaited_once() + (url,), kwargs = http.post.call_args + assert url == "http://spoolman.test/api/v1/field/spool/bambu_color_name" + assert kwargs["json"] == { + "name": "bambu_color_name", + "field_type": "text", + "default_value": None, + } + + @pytest.mark.asyncio + async def test_creates_every_field_when_spoolman_has_none(self): + """An empty listing is a real answer -- "no extra fields yet" -- and + must be acted on, unlike an unreadable one.""" + client, http = _client_with([]) + for name in BAMBU_FIELDS: + assert await client.ensure_extra_field(name) is True + assert http.post.await_count == len(BAMBU_FIELDS) + + @pytest.mark.asyncio + async def test_honours_a_non_default_field_type_when_creating(self): + client, http = _client_with([]) + await client.ensure_extra_field("some_number", field_type="integer") + assert http.post.call_args.kwargs["json"]["field_type"] == "integer" + + +class TestRequestCount: + @pytest.mark.asyncio + async def test_registering_every_field_costs_one_listing_read(self): + client, http = _client_with([_field(k) for k in BAMBU_FIELDS]) + await client._ensure_extra_fields(dict.fromkeys(BAMBU_FIELDS, "value")) + assert http.get.await_count == 1 + http.post.assert_not_awaited() + + @pytest.mark.asyncio + async def test_creating_several_new_fields_still_reads_the_listing_once(self): + client, http = _client_with([]) + await client._ensure_extra_fields(dict.fromkeys(BAMBU_FIELDS, "value")) + assert http.get.await_count == 1 + assert http.post.await_count == len(BAMBU_FIELDS) + + @pytest.mark.asyncio + async def test_a_second_pass_on_the_same_client_makes_no_requests(self): + client, http = _client_with([_field(k) for k in BAMBU_FIELDS]) + await client._ensure_extra_fields(dict.fromkeys(BAMBU_FIELDS, "value")) + http.get.reset_mock() + await client._ensure_extra_fields(dict.fromkeys(BAMBU_FIELDS, "value")) + http.get.assert_not_awaited() + http.post.assert_not_awaited() + + +class TestDegradingWhenTheListingCannotBeRead: + """An unreadable listing must not stop a write that might still succeed -- + registration is best-effort, and falling back to the blind POST is exactly + what shipped before.""" + + @pytest.mark.asyncio + async def test_a_non_200_listing_falls_back_to_posting(self): + client, http = _client_with(_response(404, None, "Not Found")) + assert await client.ensure_extra_field("tag") is True + http.post.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_transport_error_on_the_listing_falls_back_to_posting(self): + client, http = _client_with([]) + http.get = AsyncMock(side_effect=OSError("connection reset")) + assert await client.ensure_extra_field("tag") is True + http.post.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_listing_that_is_not_a_list_falls_back_to_posting(self): + client, http = _client_with(_response(200, {"detail": "Method Not Allowed"})) + assert await client.ensure_extra_field("tag") is True + http.post.assert_awaited_once() + + @pytest.mark.asyncio + async def test_rows_without_a_usable_key_are_skipped_not_fatal(self): + client, http = _client_with([{"name": "orphan"}, "junk", _field("tag")]) + assert await client.ensure_extra_field("tag") is True + http.post.assert_not_awaited() + + @pytest.mark.asyncio + async def test_a_field_created_blind_is_not_looked_up_again(self): + """The blind POST succeeded, so that field demonstrably exists now. + Re-reading the listing to confirm it would be a wasted request.""" + client, http = _client_with([]) + http.get = AsyncMock(side_effect=[_response(503), _response(200, [_field("tag")])]) + assert await client.ensure_extra_field("tag") is True + assert await client.ensure_extra_field("tag") is True + assert http.get.await_count == 1 + + @pytest.mark.asyncio + async def test_a_later_field_still_gets_a_listing_read(self): + """Only a *successful* read is banked. A client that could not reach the + listing once must not spend the rest of its life posting blind -- the + next field it has never seen has to ask again, or a transient blip + would clobber every remaining field's customisation.""" + client, http = _client_with([]) + http.get = AsyncMock( + side_effect=[_response(503), _response(200, [_field("bambu_color_name")])], + ) + await client.ensure_extra_field("tag") + http.post.reset_mock() + assert await client.ensure_extra_field("bambu_color_name") is True + assert http.get.await_count == 2 + http.post.assert_not_awaited() + + +class TestFailureReporting: + @pytest.mark.asyncio + async def test_a_rejected_creation_returns_false(self): + client, _ = _client_with([], post_status=400) + assert await client.ensure_extra_field("tag") is False + + @pytest.mark.asyncio + async def test_a_rejected_creation_is_not_remembered_as_ensured(self): + client, _ = _client_with([], post_status=400) + await client.ensure_extra_field("tag") + assert "tag" not in client._ensured_extra_fields + + @pytest.mark.asyncio + async def test_ensure_tag_extra_field_goes_through_the_same_path(self): + client, http = _client_with([_field("tag", "Bambu RFID Tag")]) + assert await client.ensure_tag_extra_field() is True + http.post.assert_not_awaited()