Browse Source

fix(spoolman): filter external library lookup by Bambu Lab manufacturer (#1330)

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) <noreply@anthropic.com>
Co-authored-by: MartinNYHC <mz@v8w.de>
Kouki Ojima 1 week ago
parent
commit
a1d6fb22ae
2 changed files with 256 additions and 10 deletions
  1. 27 10
      backend/app/services/spoolman.py
  2. 229 0
      backend/tests/unit/services/test_spoolman_service.py

+ 27 - 10
backend/app/services/spoolman.py

@@ -1044,34 +1044,50 @@ class SpoolmanClient:
 
 
     async def _find_or_create_filament(self, tray: AMSTray) -> dict | None:
     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."""
         """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()
         bambu_vendor_id = await self.ensure_bambu_vendor()
         color_hex = tray.tray_color[:6]  # Strip alpha channel
         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
         # Search internal filaments - only match Bambu Lab vendor
         filaments = await self.get_filaments()
         filaments = await self.get_filaments()
         for filament in 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")
             fil_vendor_id = filament.get("vendor_id") or filament.get("vendor", {}).get("id")
             if fil_vendor_id != bambu_vendor_id:
             if fil_vendor_id != bambu_vendor_id:
                 continue
                 continue
-
-            # Match by material and color (handle None values)
             fil_material = filament.get("material") or ""
             fil_material = filament.get("material") or ""
             fil_color = filament.get("color_hex") 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
                 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()
         external = await self.get_external_filaments()
+        sub_brand = (tray.tray_sub_brands or "").strip().lower()
+        bambu_candidates = []
         for filament in external:
         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_material = filament.get("material") or ""
             fil_color = filament.get("color_hex") 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)
+
+        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 - create new Bambu Lab filament
+        # Not found in either source - create a new Bambu Lab filament from scratch.
         return await self.create_filament(
         return await self.create_filament(
             name=tray.tray_sub_brands or tray.tray_type,
             name=tray.tray_sub_brands or tray.tray_type,
             vendor_id=bambu_vendor_id,
             vendor_id=bambu_vendor_id,
@@ -1089,6 +1105,7 @@ class SpoolmanClient:
             material=external.get("material", tray.tray_type),
             material=external.get("material", tray.tray_type),
             color_hex=external.get("color_hex", tray.tray_color[:6]),
             color_hex=external.get("color_hex", tray.tray_color[:6]),
             weight=external.get("weight", tray.tray_weight),
             weight=external.get("weight", tray.tray_weight),
+            density=external.get("density"),
         )
         )
 
 
 
 

+ 229 - 0
backend/tests/unit/services/test_spoolman_service.py

@@ -671,3 +671,232 @@ class TestInitSpoolmanClientSSRFGuard:
             client = await init_spoolman_client("http://spoolman.example.com:7912/")
             client = await init_spoolman_client("http://spoolman.example.com:7912/")
         mock_cls.assert_called_once_with("http://spoolman.example.com:7912/")
         mock_cls.assert_called_once_with("http://spoolman.example.com:7912/")
         assert client is mock_instance
         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