From a1d6fb22aee55aec60007de7c7e26beba2b63857 Mon Sep 17 00:00:00 2001 From: Kouki Ojima Date: Thu, 14 May 2026 16:14:24 +0900 Subject: [PATCH] fix(spoolman): filter external library lookup by Bambu Lab manufacturer (#1330) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bambuddy's external SpoolmanDB lookup in `_find_or_create_filament` matched on material+color only, with no manufacturer filter. Because SpoolmanDB is a multi-vendor catalog and entries are roughly ID-sorted, the first hit for any common combination is almost always a competitor — `bambulab_pla_black_1000_175_n` is the 15th entry for PLA + `#000000`. Bambu Lab RFID spools were being labeled with competitor product names (`3DJAKE Black`, `3DXTECH™ Black`, etc). Restrict the external-library loop to entries whose manufacturer is `"Bambu Lab"` (with `id.startswith("bambulab_")` as a defensive fallback for schema drift). When multiple Bambu Lab candidates exist, prefer the entry whose `name` equals the AMS `tray_sub_brands` so `"PLA Basic"` wins over generic `"Black"` when both are present. Forward `density` from the chosen external entry so it is no longer overwritten by the PLA-default 1.24 in `create_filament`. Six unit tests added: internal short-circuit preserved, non-Bambu external entries skipped, PLA Basic > generic PLA tiebreaker, no-match fallback, id-prefix defensive fallback, density propagation. Fixes #1309 Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: MartinNYHC --- backend/app/services/spoolman.py | 37 ++- .../unit/services/test_spoolman_service.py | 229 ++++++++++++++++++ 2 files changed, 256 insertions(+), 10 deletions(-) diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index 65055ac96..8fa7ac9cf 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -1044,34 +1044,50 @@ class SpoolmanClient: async def _find_or_create_filament(self, tray: AMSTray) -> dict | None: """Return a Bambu Lab filament matching the tray's material/color, creating it if absent.""" - # Get Bambu Lab vendor ID for filtering bambu_vendor_id = await self.ensure_bambu_vendor() color_hex = tray.tray_color[:6] # Strip alpha channel + material_upper = tray.tray_type.upper() + color_upper = color_hex.upper() # Search internal filaments - only match Bambu Lab vendor filaments = await self.get_filaments() for filament in filaments: - # Only match filaments from Bambu Lab vendor fil_vendor_id = filament.get("vendor_id") or filament.get("vendor", {}).get("id") if fil_vendor_id != bambu_vendor_id: continue - - # Match by material and color (handle None values) fil_material = filament.get("material") or "" fil_color = filament.get("color_hex") or "" - if fil_material.upper() == tray.tray_type.upper() and fil_color.upper() == color_hex.upper(): + if fil_material.upper() == material_upper and fil_color.upper() == color_upper: return filament - # Search external filaments (Bambu library) + # Search external filaments (SpoolmanDB) — restrict to Bambu Lab only. + # The /api/v1/external/filament endpoint returns the full multi-vendor catalog + # with no server-side filter, so without a manufacturer check the first PLA/black + # hit is typically 3DJAKE or 3DXTECH, not Bambu Lab. external = await self.get_external_filaments() + sub_brand = (tray.tray_sub_brands or "").strip().lower() + bambu_candidates = [] for filament in external: + manufacturer = (filament.get("manufacturer") or "").strip().lower() + ext_id = (filament.get("id") or "").strip().lower() + if manufacturer != "bambu lab" and not ext_id.startswith("bambulab_"): + continue fil_material = filament.get("material") or "" fil_color = filament.get("color_hex") or "" - if fil_material.upper() == tray.tray_type.upper() and fil_color.upper() == color_hex.upper(): - # Found in external library - need to create internal copy - return await self._create_filament_from_external(filament, tray) + if fil_material.upper() == material_upper and fil_color.upper() == color_upper: + bambu_candidates.append(filament) - # Not found - create new Bambu Lab filament + if bambu_candidates: + # Prefer the entry whose `name` matches the AMS `tray_sub_brands` + # (e.g. "PLA Basic", "Support for PLA/PETG Black") so the more specific + # variant wins over a generic "Black" entry when both are present. + chosen = next( + (f for f in bambu_candidates if (f.get("name") or "").strip().lower() == sub_brand), + bambu_candidates[0], + ) + return await self._create_filament_from_external(chosen, tray) + + # Not found in either source - create a new Bambu Lab filament from scratch. return await self.create_filament( name=tray.tray_sub_brands or tray.tray_type, vendor_id=bambu_vendor_id, @@ -1089,6 +1105,7 @@ class SpoolmanClient: material=external.get("material", tray.tray_type), color_hex=external.get("color_hex", tray.tray_color[:6]), weight=external.get("weight", tray.tray_weight), + density=external.get("density"), ) diff --git a/backend/tests/unit/services/test_spoolman_service.py b/backend/tests/unit/services/test_spoolman_service.py index 3196cea6c..5e6d223a7 100644 --- a/backend/tests/unit/services/test_spoolman_service.py +++ b/backend/tests/unit/services/test_spoolman_service.py @@ -671,3 +671,232 @@ class TestInitSpoolmanClientSSRFGuard: client = await init_spoolman_client("http://spoolman.example.com:7912/") mock_cls.assert_called_once_with("http://spoolman.example.com:7912/") assert client is mock_instance + + +class TestFindOrCreateFilament: + """Tests for SpoolmanClient._find_or_create_filament — the auto-create path + that runs when AMS sync sees an RFID spool that isn't already in Spoolman. + + Regression tests for #1309 (Bambu Lab RFID spools getting competitor names + like "3DXTECH™ Black" from the unfiltered SpoolmanDB lookup). + """ + + @pytest.fixture + def client(self): + return SpoolmanClient("http://localhost:7912") + + @pytest.fixture + def tray_pla_black(self): + """A typical Bambu PLA Basic Black RFID read.""" + return AMSTray( + ams_id=0, + tray_id=0, + tray_type="PLA", + tray_sub_brands="PLA Basic", + tray_color="000000FF", + remain=100, + tag_uid="", + tray_uuid="A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4", + tray_info_idx="GFA00", + tray_weight=1000, + ) + + @pytest.mark.asyncio + async def test_returns_existing_internal_bambu_lab_filament(self, client, tray_pla_black): + """When a Bambu Lab filament matching material+color already exists internally, + return it as-is — never touch the external library or create a new entry. + + This is the short-circuit that makes the workaround on #1309 necessary: once + a wrong name is on disk, subsequent AMS reads keep reusing it and the user has + to delete the mis-named entry manually for the corrected name to take effect. + """ + existing = { + "id": 6, + "name": "Black", + "material": "PLA", + "color_hex": "000000", # alpha stripped by create_filament at insert time + "vendor_id": 2, + } + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), + patch.object(client, "get_external_filaments", AsyncMock()) as mock_external, + patch.object(client, "create_filament", AsyncMock()) as mock_create, + ): + result = await client._find_or_create_filament(tray_pla_black) + + assert result is existing + mock_external.assert_not_called() + mock_create.assert_not_called() + + @pytest.mark.asyncio + async def test_skips_non_bambu_lab_external_entries(self, client, tray_pla_black): + """Regression for #1309: the external-library loop must filter out non-Bambu-Lab + manufacturers. PLA black 000000 is offered by 3DJAKE, 3DXTECH (and 60+ others) + in SpoolmanDB before Bambu Lab's entry; without the filter the first hit wins + and Bambu Lab spools get labeled with competitor names. + """ + external = [ + { + "id": "3djake_pla_black_1000_175_n", + "manufacturer": "3DJAKE", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.24, + }, + { + "id": "3dxtech_pla_carbonxcarbonfiberblack_500_175_p", + "manufacturer": "3DXTECH", + "name": "CarbonX™ Carbon Fiber Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.29, + }, + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.26, + }, + ] + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(tray_pla_black) + + mock_create.assert_called_once() + kwargs = mock_create.call_args.kwargs + # The Bambu Lab entry must win — not 3DJAKE / 3DXTECH which sort earlier. + assert kwargs["name"] == "Black" + assert kwargs["density"] == 1.26 + + @pytest.mark.asyncio + async def test_prefers_external_entry_matching_tray_sub_brands(self, client, tray_pla_black): + """When SpoolmanDB has multiple Bambu Lab entries for the same material+color + (e.g. a "PLA Basic" variant alongside a generic "Black"), prefer the entry + whose `name` equals the AMS `tray_sub_brands` so the more specific variant wins. + Per maintainer's request on #1309. + """ + external = [ + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.24, + }, + { + "id": "bambulab_plabasic_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "PLA Basic", + "material": "PLA", + "color_hex": "000000", + "density": 1.26, + }, + ] + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(tray_pla_black) + + mock_create.assert_called_once() + kwargs = mock_create.call_args.kwargs + # "PLA Basic" wins over generic "Black" because it matches tray_sub_brands. + assert kwargs["name"] == "PLA Basic" + assert kwargs["density"] == 1.26 + + @pytest.mark.asyncio + async def test_falls_back_to_create_when_no_bambu_match_anywhere(self, client, tray_pla_black): + """If no internal Bambu Lab filament exists AND SpoolmanDB has no Bambu Lab + entry for this material+color (e.g. the catalog hasn't been updated yet for a + brand-new BL product), fall back to creating a fresh filament from the tray's + own RFID data — without leaking a competitor's name in. + """ + external = [ + { + "id": "3djake_pla_black_1000_175_n", + "manufacturer": "3DJAKE", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + }, + ] + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(tray_pla_black) + + mock_create.assert_called_once() + kwargs = mock_create.call_args.kwargs + # The 3DJAKE entry was rejected by the manufacturer filter; tray_sub_brands wins. + assert kwargs["name"] == "PLA Basic" + assert kwargs["material"] == "PLA" + assert kwargs["color_hex"] == "000000" # alpha channel stripped from tray_color + assert kwargs["vendor_id"] == 2 + + @pytest.mark.asyncio + async def test_accepts_external_entry_via_id_prefix_when_manufacturer_missing(self, client, tray_pla_black): + """Defensive fallback: if `manufacturer` is absent or empty but the entry's `id` + starts with `bambulab_`, treat it as a Bambu Lab entry. Keeps the filter robust + against SpoolmanDB schema drift or stale catalog snapshots that omit the field. + """ + external = [ + { + "id": "bambulab_pla_black_1000_175_n", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.24, + }, # no `manufacturer` key at all + ] + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(tray_pla_black) + + mock_create.assert_called_once() + assert mock_create.call_args.kwargs["name"] == "Black" + + @pytest.mark.asyncio + async def test_external_density_propagates_to_create_filament(self, client, tray_pla_black): + """The chosen external entry's `density` must be forwarded to `create_filament` + instead of being silently replaced by the PLA-default 1.24 fallback inside + `create_filament` itself. Verified end-to-end via the public + `_find_or_create_filament` entry point. + """ + external = [ + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.31, + }, + ] + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(tray_pla_black) + + mock_create.assert_called_once() + assert mock_create.call_args.kwargs["density"] == 1.31