Ask Spoolman which extra fields it has, once (issue #2983)

Bambuddy checked whether one of its four custom spool fields existed with
GET /field/spool/{name}. Spoolman has never served that. Its API declares
only POST and DELETE at that path -- confirmed against the live server's
own OpenAPI document -- so the probe answered 405 Method Not Allowed
every time and the check could not succeed on any version.

Every call therefore fell through to POST /field/spool/{name}, and that
endpoint is an upsert rather than a create. It answers 200 whether or not
the field is already there, so a field the user had renamed, retyped or
given a default to in Spoolman's own UI was reset to Bambuddy's version
of it, and an untrue "Created Spoolman extra field" was logged beside it.
The reporter's log carried 60 of those lines over three days -- once per
field per client init, which is every restart and every settings save.

Existence now comes from GET /field/spool, the listing endpoint, matched
on each row's `key`. Matching on `key` rather than the display `name` is
the part that fixes the overwrite: a renamed field is the same field, and
reading it as a missing one is what re-created it. A field that already
exists is now left completely alone.

The listing is read once per client and banked, so registering all four
fields costs one request instead of four, and a client that has already
looked makes none at all. Only a successful read is banked -- a client
that could not reach the listing asks again for the next field it has not
seen, so one transient failure does not leave it posting blind, and
overwriting, for the rest of its life.

An unreadable listing still falls back to attempting the POST. Registration
is best-effort by contract: it must not turn a write that might still
succeed into one that never happens, so an unexpected Spoolman build is no
worse off than before.

Measured against Spoolman 0.23.1: a field renamed to "Bambu RFID Tag"
survives a full registration pass that previously reset it, the pass makes
one GET and one POST for the single genuinely-missing field where it used
to make four POSTs, and a second pass on the same client makes no requests
at all.

The fake in test_spoolman_extra_field_registration_2903 modelled the
per-field path as a working probe, which is the assumption this bug was
built on; it now answers 405 as the real server does, and its assertions
follow the listing. 18 new tests cover the rest, 10 of which fail against
the old code.

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 <name>.'), 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,

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:

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()