mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-18 11:56:01 +02:00
fix(cloud): log expected preset misses at DEBUG, keep real faults at WARNING
Failed to get cloud preset ... 400 {"message":"missing"} is the expected
answer, not a fault: many official presets are only addressable with a
printer-variant suffix (GFSL05 exists solely as GFSL05_07 @BBL A1), and
personal P-prefixed presets belong to the account that sliced the file.
Phase 3 already resolves both from local presets, so the lookup miss is
routine -- and one WARNING per AMS tray per tooltip refresh teaches
operators to ignore the log.
BambuCloudError now carries the upstream status_code. The preset lookup
logs HTTP 400 at DEBUG; expired tokens, 5xx and transport failures stay
at WARNING.
Not fixed here: resolving the variant suffix. It selects a printer profile
and the response carries that profile's pressure_advance, so guessing a
suffix would report another printer's K value.
This commit is contained in:
maziggy
+2
-1
@@ -37,7 +37,8 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **Sort File Manager folder tree by recent activity (#1770, requested by @Kingbuzz0)** — Until now the folder tree was always sorted alphabetically by name, both backend (`order_by(LibraryFolder.name)`) and frontend. The reporter — a user with a lot of nested cad / slicer directories — wanted "find folders that just got a new 3MF" without scrolling the whole alphabet. **What changed.** The folder sidebar header gains a small dropdown (**By name** / **By recent activity**) plus an asc / desc arrow button, sitting alongside the existing Collapse + Wrap toggles. Choice persists per-browser via `localStorage` (`library-folder-sort-field`, `library-folder-sort-direction`) so the preference survives reloads. **Activity semantics.** `latest_activity_at` per folder = `MAX(folder.updated_at, MAX(immediate-child file.updated_at))`. The DB had the data — `LibraryFile.updated_at` is `onupdate=func.now()` and `LibraryFolder.updated_at` the same — but `LibraryFolder.updated_at` alone only bumps on rename / move, not on file-add inside the folder, which is exactly the wrong signal for "did I just drop a new model in here." The aggregate fixes that. Recursion across subfolders is intentionally **NOT** computed — a deeply nested new 3MF bubbles its immediate parent, not every ancestor up to the root. This keeps the route a single `GROUP BY` rather than a recursive CTE, matching the existing file_counts subquery shape sibling at `library.py:746`. A future Tier 3 follow-up could add the recursive-CTE variant if anyone reports deeply-nested updates not bubbling far enough. **Backend.** New `latest_activity_at: datetime | None` field on `FolderResponse` and `FolderTreeItem` schemas. The `/folders` tree route picks up a sibling `func.max(LibraryFile.updated_at)` group-by alongside the existing file-count subquery; resolves the field per row. The `/folders/by-project/{id}` and `/folders/by-archive/{id}` routes collapse their per-row file-count subquery to fetch `count + max` in one trip (one extra column, zero extra round-trips). All 5 single-folder constructors (POST `/folders`, GET `/folders/{id}`, PUT `/folders/{id}`, POST `/folders/external`, the create flows) populate the field with `max(folder.updated_at, latest_file)` or fall back to `folder.updated_at` when there are no files, so the API surface is consistent across every route that returns a folder. **External folders.** `LibraryFile` rows are created for scanned external files too (`library.py:526`), so the MAX aggregate works on them — but the timestamp reflects when Bambuddy last *scanned / re-indexed* the file, not the filesystem mtime. For a NAS that gets new files added outside Bambuddy, the activity-sort lags until the next scan. Documented in the file-manager wiki page rather than papered over with `os.stat()` on every list call, which would stall the route on slow mounts. **Frontend.** A new recursive `sortedFolders` `useMemo` applies the comparator uniformly to top-level + every nested `children` level so sort order is consistent at every depth. Comparator falls back to name when activity timestamps tie or are both null, so an empty folder never elbows a recently-used one to a random place — empties go to the end of the activity bucket regardless of direction. Both the desktop sidebar render and the mobile selector dropdown consume `sortedFolders` so the order is identical across breakpoints. The single-folder `findFolder()` traversal and `selectedFolder` memo still operate on the unsorted `folders` because they index by ID — sort-order-independent. **Recursion safety.** The sort creates fresh object refs at every level on every memo invocation; the `FolderTreeItem` keys stay ID-based (`${folder.id}-${collapseFoldersByDefault ? 'c' : 'e'}`) so React reconciliation by ID preserves folder expansion state across sort flips. **i18n.** 3 new keys in `fileManager.*` (`folderSort`, `folderSortByName`, `folderSortByActivity`) translated in all 11 locales (de / en / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW), no English fallback. Parity 5238 leaves per locale. **Tests.** 2 new backend integration cases in `test_library_api.py` (file-in-folder bubbles `latest_activity_at` to the file's timestamp, empty folder falls back to `folder.updated_at`). All 152 library + folder + trash + slice integration tests still pass; 51/51 FileManagerPage frontend tests still pass; 26/26 QueuePage tests still pass; `npm run build` clean; `ruff` clean; i18n parity green.
|
||||
|
||||
### Fixed
|
||||
- **Enabling authentication silently disconnected Bambu Cloud (#2530, reporter @hburn7)** — Cloud credentials live in two different places depending on auth state: `get_stored_token()` reads the global `Settings` rows (`bambu_cloud_token` / `_email` / `_region`) when auth is off, and `User.cloud_token` when it's on. Completing `POST /auth/setup` flipped which store the `/cloud/*` routes consult, but nothing carried the token across — so an operator who linked their Bambu account *before* turning on auth found the freshly-created admin had `cloud_token = NULL`. `build_authenticated_cloud()` then returned `None` and every cloud route degraded: `get_filament_info` skipped its cloud phase entirely and answered `200` from the local-preset and built-in-name fallbacks, while `/cloud/devices` began returning `401`. Nothing surfaced the disconnect. The reporter observed the *symptom inverted* — cloud `400` warnings vanished after enabling auth — and reasonably read that as a fix; in fact the warnings stopped because Bambuddy had stopped calling the cloud at all. The tell is in their own timestamps: the pre-auth request spent ~960 ms on cloud round-trips, the post-auth one answered immediately. **Fix.** `setup_auth()` now migrates a globally-stored token onto the owning admin (and deletes the global rows, so a live credential isn't left at rest in a table nothing reads), and `disable_auth()` performs the mirror hand-off back to global storage. Both refuse to guess when ownership is ambiguous: setup migrates only when it creates the admin or exactly one admin already exists — with several admins it leaves the credential in place and logs a warning rather than handing one admin another's Bambu session; disable declines to overwrite a pre-existing global token. The `region` survives both hops rather than silently resetting to `global`. **Note for existing installs.** The migration runs at the auth on/off transition, so instances that already crossed it must re-link their Bambu account once from Settings → Bambu Cloud; the stranded `bambu_cloud_*` rows in `settings` can then be deleted. **Tests.** 7 integration cases pinning both directions, the two refuse-to-guess paths, the `auth_enabled=false` no-op, and region preservation. **Scope.** No DB migration, no schema change, no new permission, no i18n change. Separately tracked: the cloud `400 missing` warnings themselves are unrelated to auth and are logged at WARNING for an expected, already-handled fallback.
|
||||
- **Enabling authentication silently disconnected Bambu Cloud (#2530, reporter @hburn7)** — Cloud credentials live in two different places depending on auth state: `get_stored_token()` reads the global `Settings` rows (`bambu_cloud_token` / `_email` / `_region`) when auth is off, and `User.cloud_token` when it's on. Completing `POST /auth/setup` flipped which store the `/cloud/*` routes consult, but nothing carried the token across — so an operator who linked their Bambu account *before* turning on auth found the freshly-created admin had `cloud_token = NULL`. `build_authenticated_cloud()` then returned `None` and every cloud route degraded: `get_filament_info` skipped its cloud phase entirely and answered `200` from the local-preset and built-in-name fallbacks, while `/cloud/devices` began returning `401`. Nothing surfaced the disconnect. The reporter observed the *symptom inverted* — cloud `400` warnings vanished after enabling auth — and reasonably read that as a fix; in fact the warnings stopped because Bambuddy had stopped calling the cloud at all. The tell is in their own timestamps: the pre-auth request spent ~960 ms on cloud round-trips, the post-auth one answered immediately. **Fix.** `setup_auth()` now migrates a globally-stored token onto the owning admin (and deletes the global rows, so a live credential isn't left at rest in a table nothing reads), and `disable_auth()` performs the mirror hand-off back to global storage. Both refuse to guess when ownership is ambiguous: setup migrates only when it creates the admin or exactly one admin already exists — with several admins it leaves the credential in place and logs a warning rather than handing one admin another's Bambu session; disable declines to overwrite a pre-existing global token. The `region` survives both hops rather than silently resetting to `global`. **Note for existing installs.** The migration runs at the auth on/off transition, so instances that already crossed it must re-link their Bambu account once from Settings → Bambu Cloud; the stranded `bambu_cloud_*` rows in `settings` can then be deleted. **Tests.** 7 integration cases pinning both directions, the two refuse-to-guess paths, the `auth_enabled=false` no-op, and region preservation. **Scope.** No DB migration, no schema change, no new permission, no i18n change.
|
||||
- **Routine cloud preset misses no longer log at WARNING (#2530)** — The `Failed to get cloud preset … 400 {"message":"missing"}` lines that led to #2530 being filed are an *expected* answer, not a fault, and `get_filament_info`'s Phase 3 already resolves the name from local presets (a bare `GFL05` lands on "Overture Matte PLA" without the cloud). Two routine causes, both confirmed against the live Bambu catalog: many official presets are only addressable with a printer-variant suffix — `GFSA00` and `GFSL99` resolve bare, but `GFSL05` and `GFSG00` exist *only* as `GFSL05_07` (`@BBL A1`), `GFSG00_06` and so on, while the AMS reports the bare ID; and personal presets (`P…`, e.g. `Pb5b7d17`) belong to whichever Bambu account sliced the file, so no other account will ever resolve them. Emitting a WARNING per tray on every AMS tooltip refresh trains operators to ignore the log. `BambuCloudError` now carries the upstream `status_code`, and the preset lookup logs an HTTP 400 at DEBUG while leaving every other failure — expired token, 5xx, connection error — at WARNING, so a genuine fault is still loud. Deliberately *not* fixed here: resolving the variant suffix. The suffix selects a printer profile, and the endpoint returns `pressure_advance` (the K value), which is per-printer — picking a suffix arbitrarily would populate AMS tooltips with another printer's K value, which is worse than the current blank. Doing that correctly requires threading the tray's printer model into `get_filament_info`, which changes the endpoint contract. **Tests.** 4 parametrised cases drive the real route with a stubbed cloud and assert the 400 lands at DEBUG while 401 / 502 / transport failures stay at WARNING; verified by mutation (forcing the classification off fails the 400 case).
|
||||
- **Printer FTPS and MQTT connections inherited their TLS floor from the OpenSSL build instead of declaring one** — `ImplicitFTP_TLS` (`bambu_ftp.py`) and the MQTT client (`bambu_mqtt.py`) both built their context with `ssl.create_default_context()`, which leaves `minimum_version` at `MINIMUM_SUPPORTED`. What that resolves to is a property of the interpreter's OpenSSL build, not of Bambuddy: measured on identical `OpenSSL 3.5.6`, the `python:3.13-slim-trixie` Docker base reports `TLSVersion.TLSv1_2` while a bare-metal venv reports `MINIMUM_SUPPORTED` — so Docker users have always been floored at TLS 1.2, while bare-metal and appliance installs could in principle negotiate TLS 1.0 or 1.1 with a printer that offered them. **Fix.** Both contexts now set `minimum_version = ssl.TLSVersion.TLSv1_2` explicitly. On the two FTP profiles that also cap `maximum_version` (P2S, X2D — see #1401) this yields an exact TLS 1.2 pin rather than a ceiling over an inherited floor. **Verified against hardware, not just tests.** Probing an X1C and an H2D on both `:990` and `:8883`, each printer completes only on TLS 1.2 and rejects 1.0, 1.1 *and* 1.3 with a `handshake_failure` alert; a live FTPS login through the changed code path succeeds on both with `TLSv1.2` negotiated. Since the shipped Docker image already enforced this floor across the whole install base, no printer or firmware reachable today can be affected by making it explicit. **Also corrected** a stale comment in `ftp_profiles.py` claiming X1C / H2D installs "stay on the negotiated TLS 1.3" — both models refuse 1.3 outright, so `cap_tls_v1_2` is a no-op there; the P2S evidently does offer 1.3, which is why it alone surfaced the vsFTPd session-reuse bug. **Scope.** Two lines plus a comment. No behaviour change on Docker, no DB migration, no new permission, no i18n change; certificate verification is unchanged (printers use self-signed certs, so `check_hostname`/`CERT_NONE` remain by necessity).
|
||||
- **Backend failed to start on fastapi < 0.116: `AssertionError: Status code 204 must not have a response body`** — `uvicorn backend.app.main:app` aborted at import time while registering `DELETE /api/v1/library/tags/{tag_id}`. The route is declared `status_code=204` with a `-> None` return annotation, and `library_tags.py` uses `from __future__ import annotations` — so the annotation reaches FastAPI as the *string* `"None"`, which `get_typed_annotation()` resolves via `evaluate_forwardref()` to `NoneType`. `NoneType` is a class and therefore truthy, so `APIRoute.__init__` took the `if self.response_model:` branch and asserted that a 204 may carry no response body. fastapi **0.116** added an `if annotation is type(None): return None` guard that makes this benign, which is why CI and the Docker image (both resolve the top of the `>=0.109.0,<0.136.0` range) never saw it — only installs pinned to an older release inside that supported range, such as a venv created before the tag catalog landed in #1268, hit the crash. **Fix.** The route declares `response_model=None` explicitly, which short-circuits the annotation inference on every fastapi version in the supported range. The sibling 204 route (`DELETE /slicer/pipelines/{pipeline_id}`) is unaffected — its module has no `from __future__ import annotations` and no return annotation. **Scope.** Backend-only, one decorator. No behaviour change on fastapi >= 0.116, no DB migration, no new permission, no i18n change. Existing installs can equivalently unblock themselves with `pip install -U -r requirements.txt`.
|
||||
- **Dependency floors permitted resolutions the code can't run on: `sqlalchemy>=2.0.38`, exact ruff pin** — Two more instances of the same class of defect as the 204 crash above: `requirements.txt` declared floors low enough that a legitimate `pip install -r requirements.txt` could produce an environment Bambuddy fails to start or lint in. CI never caught either, because a fresh runner always resolves to the *top* of every range — only a longer-lived venv resolving lower hits them. **sqlalchemy.** `core/database._create_engine()` passes `pool_size` / `max_overflow` on the SQLite branch. SQLAlchemy **2.0.38** changed the aiosqlite dialect's default pool for file databases from `NullPool` (which rejects both kwargs) to `AsyncAdaptedQueuePool` (which accepts them); on 2.0.0-2.0.37 the module-level `engine = _create_engine()` raises `TypeError: Invalid argument(s) 'pool_size','max_overflow' sent to create_engine()` at import, taking down every SQLite install and the whole test suite (`conftest.py` imports the module). Postgres installs were never affected — `is_sqlite()` is False and the branch is dead. Floor raised to `sqlalchemy>=2.0.38`. **ruff.** The lint job ran a bare `pip install ruff` (always the newest release) while `requirements-dev.txt` said `ruff>=0.8.0`, so CI's linter and a contributor's were routinely *different programs enforcing different rule sets*. A venv holding ruff 0.8.4 reported 32 errors against a tree current ruff calls clean — 30 of them `UP038`, a rule ruff has since **removed** (PEP 604 syntax in `isinstance()` is slower than the tuple form it wanted you to replace). ruff is now pinned exactly (`ruff==0.15.20`) and the CI lint job installs that pin from `requirements-dev.txt`, so local and CI enforce the same rules and `format --check` can't disagree across machines. **Scope.** Packaging + CI only; no application code, no DB migration, no permission, no i18n change. Existing environments should re-run `pip install -U -r requirements.txt -r requirements-dev.txt`.
|
||||
|
||||
@@ -861,9 +861,24 @@ async def get_filament_info(
|
||||
if not name:
|
||||
still_unresolved.append(setting_id)
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
f"Failed to get cloud preset {setting_id} "
|
||||
f"(API ID: {_filament_id_to_setting_id(setting_id)}): {e}"
|
||||
# A 400 here is the *expected* answer, not a fault, and Phase 3
|
||||
# exists to handle it (#2530). Two routine causes:
|
||||
# * Many official presets are only addressable with a printer
|
||||
# variant suffix — "GFSA00" resolves, "GFSL05" does not, only
|
||||
# "GFSL05_07" (@BBL A1) does. The bare ID is all the AMS
|
||||
# reports, so the lookup legitimately misses.
|
||||
# * Personal presets ("P…") belong to the Bambu account that
|
||||
# sliced the file; another account will never resolve them.
|
||||
# Logging those at WARNING on every AMS tooltip refresh trains
|
||||
# users to ignore the log. Anything else — expired token, 5xx,
|
||||
# a connection failure — stays at WARNING because it is a fault.
|
||||
expected_miss = isinstance(e, BambuCloudError) and e.status_code == 400
|
||||
logger.log(
|
||||
logging.DEBUG if expected_miss else logging.WARNING,
|
||||
"Failed to get cloud preset %s (API ID: %s): %s",
|
||||
setting_id,
|
||||
_filament_id_to_setting_id(setting_id),
|
||||
e,
|
||||
)
|
||||
still_unresolved.append(setting_id)
|
||||
|
||||
|
||||
@@ -79,9 +79,17 @@ _SLICER_API_VERSION = "1.0.0.0"
|
||||
|
||||
|
||||
class BambuCloudError(Exception):
|
||||
"""Base exception for Bambu Cloud errors."""
|
||||
"""Base exception for Bambu Cloud errors.
|
||||
|
||||
pass
|
||||
``status_code`` carries the upstream HTTP status when the failure came from
|
||||
a response rather than from the transport, so callers can tell an expected
|
||||
"this preset isn't in the catalog" 400 apart from an expired token or a
|
||||
cloud outage. It stays ``None`` for connection-level failures.
|
||||
"""
|
||||
|
||||
def __init__(self, message: str, *, status_code: int | None = None):
|
||||
super().__init__(message)
|
||||
self.status_code = status_code
|
||||
|
||||
|
||||
class BambuCloudAuthError(BambuCloudError):
|
||||
@@ -408,7 +416,10 @@ class BambuCloudService:
|
||||
|
||||
# Include body so a future contract change is self-diagnostic from logs.
|
||||
body = (response.text or "")[:200]
|
||||
raise BambuCloudError(f"Failed to get setting detail: {response.status_code} {body}")
|
||||
raise BambuCloudError(
|
||||
f"Failed to get setting detail: {response.status_code} {body}",
|
||||
status_code=response.status_code,
|
||||
)
|
||||
|
||||
except httpx.RequestError as e:
|
||||
raise BambuCloudError(f"Request failed: {e}")
|
||||
|
||||
@@ -12,11 +12,14 @@ These tests pin the hand-off in both directions, and — just as importantly —
|
||||
pin the two cases where Bambuddy must refuse to guess who owns a credential.
|
||||
"""
|
||||
|
||||
import logging
|
||||
|
||||
import pytest
|
||||
from httpx import AsyncClient
|
||||
from sqlalchemy import select
|
||||
from sqlalchemy.ext.asyncio import AsyncSession
|
||||
|
||||
from backend.app.api.routes import cloud as cloud_routes
|
||||
from backend.app.api.routes.cloud import (
|
||||
CLOUD_EMAIL_KEY,
|
||||
CLOUD_REGION_KEY,
|
||||
@@ -26,6 +29,7 @@ from backend.app.api.routes.cloud import (
|
||||
from backend.app.core.auth import get_password_hash
|
||||
from backend.app.models.settings import Settings
|
||||
from backend.app.models.user import User
|
||||
from backend.app.services.bambu_cloud import BambuCloudError
|
||||
|
||||
|
||||
async def _seed_global_token(db: AsyncSession, token: str = "tok-global", region: str = "china") -> None:
|
||||
@@ -199,3 +203,75 @@ async def test_disable_auth_with_no_cloud_token_is_a_noop(async_client: AsyncCli
|
||||
resp = await async_client.post("/api/v1/auth/disable", headers={"Authorization": f"Bearer {bearer}"})
|
||||
assert resp.status_code == 200, resp.text
|
||||
assert await _global_rows(db_session) == {}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Log-level classification for cloud preset misses (#2530)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_bambu_cloud_error_carries_status_code():
|
||||
"""The 400-vs-fault distinction depends on this attribute existing."""
|
||||
from backend.app.services.bambu_cloud import BambuCloudError
|
||||
|
||||
assert BambuCloudError("boom").status_code is None
|
||||
assert BambuCloudError("boom", status_code=400).status_code == 400
|
||||
# Every pre-existing raise site passes a bare message; must not break.
|
||||
assert str(BambuCloudError("Request failed: timeout")) == "Request failed: timeout"
|
||||
|
||||
|
||||
class _StubCloud:
|
||||
"""Minimal stand-in for BambuCloudService that fails every preset lookup."""
|
||||
|
||||
def __init__(self, error: Exception):
|
||||
self._error = error
|
||||
self.is_authenticated = True
|
||||
|
||||
async def get_setting_detail(self, setting_id: str) -> dict:
|
||||
raise self._error
|
||||
|
||||
async def close(self) -> None:
|
||||
return None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
("error", "expected_level"),
|
||||
[
|
||||
(BambuCloudError("missing", status_code=400), logging.DEBUG),
|
||||
(BambuCloudError("unauthorized", status_code=401), logging.WARNING),
|
||||
(BambuCloudError("bad gateway", status_code=502), logging.WARNING),
|
||||
(BambuCloudError("Request failed: timeout"), logging.WARNING),
|
||||
],
|
||||
ids=["expected-400-miss", "expired-token", "cloud-outage", "transport-failure"],
|
||||
)
|
||||
async def test_preset_miss_logs_at_debug_but_faults_stay_at_warning(
|
||||
monkeypatch, caplog, db_session: AsyncSession, error: Exception, expected_level: int
|
||||
):
|
||||
"""A preset the catalog doesn't carry is routine; an expired token is a fault.
|
||||
|
||||
Drives the real ``get_filament_info`` route so the classification is exercised
|
||||
where it lives, not re-derived in the test.
|
||||
"""
|
||||
monkeypatch.setattr(cloud_routes, "build_authenticated_cloud", _stub_builder(error))
|
||||
# Phase 1 would otherwise short-circuit the cloud call on a warm cache.
|
||||
monkeypatch.setattr(cloud_routes, "_filament_cache", {})
|
||||
|
||||
with caplog.at_level(logging.DEBUG, logger="backend.app.api.routes.cloud"):
|
||||
result = await cloud_routes.get_filament_info(setting_ids=["GFL05"], db=db_session, current_user=None)
|
||||
|
||||
records = [r for r in caplog.records if "Failed to get cloud preset" in r.getMessage()]
|
||||
assert len(records) == 1, "the miss must be logged exactly once"
|
||||
assert records[0].levelno == expected_level
|
||||
assert "GFL05" in records[0].getMessage()
|
||||
assert "GFSL05" in records[0].getMessage(), "the translated API ID stays in the message"
|
||||
|
||||
# Whatever the level, the endpoint still answers and falls through to Phase 3.
|
||||
assert isinstance(result, dict)
|
||||
|
||||
|
||||
def _stub_builder(error: Exception):
|
||||
async def _build(db, user):
|
||||
return _StubCloud(error)
|
||||
|
||||
return _build
|
||||
|
||||
Reference in New Issue
Block a user