Browse Source

fix(library): MakerWorld URL import honours external folder destinations (#1645)

  Reported and root-caused by @needo37. Importing a model via the MakerWorld
  URL-download feature into a writable external folder (e.g. SMB/NFS-mounted
  NAS) saved the 3MF into Bambuddy's internal managed library dir, not the
  external mount. The file card showed in the File Manager under the
  external folder, but the bytes never landed on the NAS, and the on-disk
  copy was UUID-renamed so a find by the original basename matched nothing.

  Root cause was save_3mf_bytes_to_library at backend/app/api/routes/library.py:422:
  it accepted folder_id but never loaded the folder, never inspected
  is_external / external_path, hardcoded the destination to
  get_library_files_dir() with a UUID name, and left the LibraryFile row
  with is_external=False. So the row's folder_id pointed at the external
  folder while its bytes and is_external flag both said "managed/internal".
  Same class of bug as #1112, which had been fixed for the multipart-upload
  and move paths but never applied to this byte-import path.

  Fix mirrors the multipart-upload path directly:
  - Load target_folder from folder_id when non-None.
  - Feed it to _resolve_upload_destination(target_folder, filename), which
    already returns (dest, is_external) and enforces the 403-read-only /
    400-unwritable-or-missing / 409-collision rejections.
  - Write bytes to dest (real filename for external, UUID for managed).
  - Persist the row with file_path=_stored_file_path(dest, is_external)
    and is_external=is_external.

  The route-layer read-only guard at makerworld.py:256-260 is preserved -
  it returns the friendlier error before the upstream download burns
  bandwidth - and _resolve_upload_destination's identical check stays as
  defence-in-depth for any future caller that skips the route gate.
  Thumbnails continue to live under the managed get_library_thumbnails_dir()
  regardless of the 3MF's location, matching the upload path.
maziggy 3 tháng trước cách đây
mục cha
commit
54389a54aa
3 tập tin đã thay đổi với 162 bổ sung8 xóa
  1. 0 0
      CHANGELOG.md
  2. 17 8
      backend/app/api/routes/library.py
  3. 145 0
      backend/tests/unit/test_makerworld_routes.py

Những thai đổi đã bị hủy bỏ vì nó quá lớn
+ 0 - 0
CHANGELOG.md


+ 17 - 8
backend/app/api/routes/library.py

@@ -449,13 +449,21 @@ async def save_3mf_bytes_to_library(
         if existing_row is not None:
             return existing_row, True
 
-    # Persist bytes to disk under a UUID-scoped filename; keep the original
-    # extension so downstream logic (ThreeMFParser, thumbnail viewer) works.
-    ext = os.path.splitext(filename)[1].lower() or ".3mf"
-    unique_filename = f"{uuid.uuid4().hex}{ext}"
-    file_path = (
-        get_library_files_dir() / unique_filename
-    )  # SEC-PATH-OK: unique_filename = uuid.uuid4().hex + ext, generated on the previous line
+    # Resolve target folder so writable-external destinations land on the
+    # mount with the real filename, instead of being silently misrouted to
+    # the internal library dir with a UUID name (#1645). Mirrors what the
+    # multipart-upload path has done since #1112. ``_resolve_upload_destination``
+    # also enforces the 403 read-only / 400 unwritable / 409 collision
+    # rejections — the makerworld route layer already pre-checks read-only,
+    # but the helper's checks remain as defence-in-depth for any future
+    # caller that skips that route gate.
+    target_folder: LibraryFolder | None = None
+    if folder_id is not None:
+        folder_q = await db.execute(select(LibraryFolder).where(LibraryFolder.id == folder_id))
+        target_folder = folder_q.scalar_one_or_none()
+
+    file_path, is_external = _resolve_upload_destination(target_folder, filename)
+    ext = file_path.suffix.lower() or ".3mf"
     with open(file_path, "wb") as fh:
         fh.write(file_bytes)
 
@@ -486,8 +494,9 @@ async def save_3mf_bytes_to_library(
 
     library_file = LibraryFile(
         folder_id=folder_id,
+        is_external=is_external,
         filename=filename,
-        file_path=to_relative_path(file_path),
+        file_path=_stored_file_path(file_path, is_external),
         file_type=classify_file_type(filename),
         file_size=len(file_bytes),
         file_hash=file_hash,

+ 145 - 0
backend/tests/unit/test_makerworld_routes.py

@@ -382,6 +382,151 @@ class TestImport:
         assert resp.status_code == 200, resp.text
         assert resp.json()["profile_id"] == 298919107
 
+    @pytest.mark.asyncio
+    async def test_import_to_writable_external_writes_bytes_to_mount(self, async_client, db_session, tmp_path):
+        """#1645: importing into a writable external folder writes the bytes to
+        ``<external_path>/<filename>`` and tags the row ``is_external=True`` —
+        same shape as the multipart-upload path (#1112). Previously the bytes
+        landed in the internal library dir under a UUID name while the row
+        showed up under the external folder in the UI, leaving a NAS/SMB user
+        unable to find their file on the mount."""
+        ext_dir = tmp_path / "nas-makerworld"
+        ext_dir.mkdir()
+        folder = LibraryFolder(
+            name="NAS Imports",
+            parent_id=None,
+            is_external=True,
+            external_path=str(ext_dir),
+            external_readonly=False,
+        )
+        db_session.add(folder)
+        await db_session.commit()
+        await db_session.refresh(folder)
+
+        svc = _fake_service(
+            get_design=_default_design(),
+            get_profile_download=_default_manifest("seed-starter.3mf"),
+            download_3mf=(self._FAKE_3MF_BYTES, "seed-starter.3mf"),
+        )
+
+        with patch("backend.app.api.routes.makerworld._build_service", AsyncMock(return_value=svc)):
+            resp = await async_client.post(
+                "/api/v1/makerworld/import",
+                json={"model_id": 1400373, "profile_id": 298919107, "folder_id": folder.id},
+            )
+        assert resp.status_code == 200, resp.text
+
+        from sqlalchemy import select
+
+        row = (
+            await db_session.execute(select(LibraryFile).where(LibraryFile.id == resp.json()["library_file_id"]))
+        ).scalar_one()
+        assert row.folder_id == folder.id
+        assert row.is_external is True, "Row must be tagged external so re-scan can reconcile it"
+        # External rows persist the absolute mount path (matches scan + upload paths).
+        assert row.file_path == str(ext_dir / "seed-starter.3mf")
+        on_disk = ext_dir / "seed-starter.3mf"
+        assert on_disk.is_file(), "Bytes must land on the external mount, not in the internal library dir"
+        assert on_disk.read_bytes() == self._FAKE_3MF_BYTES
+
+    @pytest.mark.asyncio
+    async def test_import_to_readonly_external_rejected_at_route(self, async_client, db_session, tmp_path):
+        """The route-layer gate at ``makerworld.py:256-260`` rejects read-only
+        externals with 403 before any download happens — so MakerWorld
+        credentials and the upstream download bandwidth aren't wasted."""
+        ext_dir = tmp_path / "nas-readonly"
+        ext_dir.mkdir()
+        folder = LibraryFolder(
+            name="NAS read-only",
+            parent_id=None,
+            is_external=True,
+            external_path=str(ext_dir),
+            external_readonly=True,
+        )
+        db_session.add(folder)
+        await db_session.commit()
+        await db_session.refresh(folder)
+
+        svc = _fake_service(
+            get_design=_default_design(),
+            get_profile_download=_default_manifest(),
+        )
+        svc.download_3mf = AsyncMock()
+
+        with patch("backend.app.api.routes.makerworld._build_service", AsyncMock(return_value=svc)):
+            resp = await async_client.post(
+                "/api/v1/makerworld/import",
+                json={"model_id": 1400373, "profile_id": 298919107, "folder_id": folder.id},
+            )
+        assert resp.status_code == 403, resp.text
+        svc.download_3mf.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_import_to_external_with_missing_path_returns_400(self, async_client, db_session, tmp_path):
+        """If the external folder's mount has gone away (NAS unplugged, SMB
+        share down), ``_resolve_upload_destination`` returns 400 before the
+        write so we don't silently fall back to the internal library dir."""
+        missing_dir = tmp_path / "vanished-mount"  # NOTE: deliberately not created
+        folder = LibraryFolder(
+            name="NAS gone",
+            parent_id=None,
+            is_external=True,
+            external_path=str(missing_dir),
+            external_readonly=False,
+        )
+        db_session.add(folder)
+        await db_session.commit()
+        await db_session.refresh(folder)
+
+        svc = _fake_service(
+            get_design=_default_design(),
+            get_profile_download=_default_manifest(),
+            download_3mf=(self._FAKE_3MF_BYTES, "benchy.3mf"),
+        )
+
+        with patch("backend.app.api.routes.makerworld._build_service", AsyncMock(return_value=svc)):
+            resp = await async_client.post(
+                "/api/v1/makerworld/import",
+                json={"model_id": 1400373, "profile_id": 298919107, "folder_id": folder.id},
+            )
+        assert resp.status_code == 400, resp.text
+        assert "not accessible" in resp.text.lower()
+
+    @pytest.mark.asyncio
+    async def test_import_to_external_with_name_collision_returns_409(self, async_client, db_session, tmp_path):
+        """A user-visible 409 fires when the filename already exists on the
+        external mount, instead of silently overwriting a file the user put
+        there outside Bambuddy."""
+        ext_dir = tmp_path / "nas-collide"
+        ext_dir.mkdir()
+        (ext_dir / "benchy.3mf").write_bytes(b"pre-existing")
+
+        folder = LibraryFolder(
+            name="NAS collide",
+            parent_id=None,
+            is_external=True,
+            external_path=str(ext_dir),
+            external_readonly=False,
+        )
+        db_session.add(folder)
+        await db_session.commit()
+        await db_session.refresh(folder)
+
+        svc = _fake_service(
+            get_design=_default_design(),
+            get_profile_download=_default_manifest("benchy.3mf"),
+            download_3mf=(self._FAKE_3MF_BYTES, "benchy.3mf"),
+        )
+
+        with patch("backend.app.api.routes.makerworld._build_service", AsyncMock(return_value=svc)):
+            resp = await async_client.post(
+                "/api/v1/makerworld/import",
+                json={"model_id": 1400373, "profile_id": 298919107, "folder_id": folder.id},
+            )
+        assert resp.status_code == 409, resp.text
+        # Pre-existing file's contents must not be clobbered by the failed write.
+        assert (ext_dir / "benchy.3mf").read_bytes() == b"pre-existing"
+
 
 class TestRecentImports:
     """GET /makerworld/recent-imports — sidebar feed on the MakerWorld page."""

Một số tệp đã không được hiển thị bởi vì quá nhiều tập tin thay đổi trong này khác