diff --git a/backend/melodies/router.py b/backend/melodies/router.py index 0b65aa8..7b48309 100644 --- a/backend/melodies/router.py +++ b/backend/melodies/router.py @@ -151,8 +151,9 @@ async def upload_file( if file_type == "binary": content_type = "application/octet-stream" + if not melody.pid: + raise HTTPException(status_code=400, detail="Melody must have a PID before uploading a binary") url = service.save_binary_for_melody( - melody_uid=melody.uid, pid=melody.pid, file_bytes=contents, ) @@ -191,7 +192,7 @@ async def delete_file( raise HTTPException(status_code=400, detail="file_type must be 'binary' or 'preview'") melody = await service.get_melody(melody_id) - service.delete_file(melody_id, file_type, melody.uid) + await service.delete_file(melody_id, file_type, melody.uid, melody.pid) @router.get("/{melody_id}/files") @@ -228,7 +229,7 @@ async def download_binary_file( ): """Download current melody binary with a PID-based filename.""" melody = await service.get_melody(melody_id) - file_bytes, content_type = service.get_binary_file_bytes(melody_id, melody.uid) + file_bytes, content_type = service.get_binary_file_bytes(melody_id, melody.pid) filename = f"{(melody.pid or 'binary')}.bsm" headers = {"Content-Disposition": f'attachment; filename="{filename}"'} return Response(content=file_bytes, media_type=content_type, headers=headers) diff --git a/backend/melodies/service.py b/backend/melodies/service.py index 543d539..7490cbd 100644 --- a/backend/melodies/service.py +++ b/backend/melodies/service.py @@ -24,8 +24,16 @@ def _ensure_binary_storage_dir(): BINARY_STORAGE_DIR.mkdir(parents=True, exist_ok=True) -def _binary_file_path(melody_uid: str) -> Path: - return BINARY_STORAGE_DIR / f"{melody_uid}.bsm" +def _binary_file_path(pid: str) -> Path: + """Path to the local .bsm file for a given archetype PID. + + Keyed on pid, not melody uid: pid identifies the underlying archetype binary + (raw note sequence). Multiple melodies can legitimately share one pid — each + melody remaps those notes to actual bells via its own noteAssignments/speed/ + duration settings — so several melodies writing/reading the same {pid}.bsm + file is expected, not a collision. + """ + return BINARY_STORAGE_DIR / f"{pid}.bsm" def _parse_localized_string(value: str) -> dict: @@ -143,25 +151,8 @@ def _sanitize_metadata_for_update(existing: dict | None, incoming: dict | None, return merged -async def _check_pid_unique(pid: str, exclude_melody_id: str | None = None) -> None: - """Raise 409 if another melody already uses this PID. - - PIDs are the public lookup key for the /melodies/download/{pid} device-facing - route, so two melodies sharing a PID would make that route resolve ambiguously. - """ - if not pid: - return - rows = await melody_db.list_melodies() - for row in rows: - if exclude_melody_id and row["id"] == exclude_melody_id: - continue - if row["data"].get("pid") == pid: - raise HTTPException(status_code=409, detail=f"A melody with PID '{pid}' already exists.") - - async def create_melody(data: MelodyCreate, publish: bool = False, actor_name: str | None = None) -> MelodyInDB: """Create a new melody. If publish=True, also push to Firestore.""" - await _check_pid_unique(data.pid) melody_id = str(uuid.uuid4()) doc_data = data.model_dump() doc_data["metadata"] = _sanitize_metadata_for_create(doc_data.get("metadata"), actor_name) @@ -184,9 +175,6 @@ async def update_melody(melody_id: str, data: MelodyUpdate, actor_name: str | No if not row: raise NotFoundError("Melody") - if data.pid is not None and data.pid != row["data"].get("pid"): - await _check_pid_unique(data.pid, exclude_melody_id=melody_id) - existing_data = row["data"] update_data = data.model_dump(exclude_none=True) @@ -268,43 +256,46 @@ async def delete_melody(melody_id: str) -> None: doc_ref.delete() # Delete storage files - _delete_storage_files(melody_id, row["data"].get("uid")) + await _delete_storage_files(melody_id, row["data"].get("uid"), row["data"].get("pid")) # Delete from SQLite await melody_db.delete_melody(melody_id) -def save_binary_for_melody(melody_uid: str, pid: str, file_bytes: bytes) -> str: - """Save a melody's .bsm binary to local disk, replacing any previous binary for this melody. +def save_binary_for_melody(pid: str, file_bytes: bytes) -> str: + """Save an archetype binary to local disk under its pid, replacing any previous + file for that pid. Multiple melodies can share one pid (they remap the same + underlying note sequence via their own noteAssignments/speed/duration), so + writing the same {pid}.bsm from a different melody is expected — not a collision. Returns the plain-HTTP download URL devices use (served from melody_download_base_url, not Firebase — ESP32 devices can't afford the RAM for a TLS client). """ _ensure_binary_storage_dir() - path = _binary_file_path(melody_uid) + path = _binary_file_path(pid) path.write_bytes(file_bytes) return f"{settings.melody_download_base_url}/{pid}" async def get_binary_path_by_pid(pid: str) -> Path: - """Resolve a melody's PID to its local .bsm file path. Used by the unauthenticated + """Resolve a pid to its local .bsm file path. Used by the unauthenticated device-facing download route.""" - rows = await melody_db.list_melodies() - for row in rows: - if row["data"].get("pid") == pid: - melody_uid = row["data"].get("uid") - path = _binary_file_path(melody_uid) - if path.exists(): - return path - raise NotFoundError("Binary file") - raise NotFoundError("Melody") + path = _binary_file_path(pid) + if path.exists(): + return path + raise NotFoundError("Binary file") -def delete_local_binary(melody_uid: str) -> None: - """Delete the local .bsm binary file for a melody, if present.""" - if not melody_uid: +def delete_local_binary(pid: str) -> None: + """Delete the local .bsm binary file for a pid, if present. + + Only call this when no other melody still references this pid — since the + file may be shared by multiple melodies, deleting one melody shouldn't + reflexively delete a binary others still use. + """ + if not pid: return - path = _binary_file_path(melody_uid) + path = _binary_file_path(pid) if path.exists(): path.unlink() @@ -405,18 +396,30 @@ def upload_file_for_melody(melody_id: str, melody_uid: str | None, melody_pid: s return blob.public_url -def get_binary_file_bytes(melody_id: str, melody_uid: str | None = None) -> tuple[bytes, str]: +async def _pid_used_by_other_melody(pid: str, exclude_melody_id: str) -> bool: + """Check whether any melody other than exclude_melody_id still references this pid.""" + if not pid: + return False + rows = await melody_db.list_melodies() + return any( + row["id"] != exclude_melody_id and row["data"].get("pid") == pid + for row in rows + ) + + +def get_binary_file_bytes(melody_id: str, melody_pid: str | None = None) -> tuple[bytes, str]: """Fetch current binary bytes for a melody from local disk storage.""" - path = _binary_file_path(melody_uid) if melody_uid else None + path = _binary_file_path(melody_pid) if melody_pid else None if not path or not path.exists(): raise NotFoundError("Binary file") return path.read_bytes(), "application/octet-stream" -def delete_file(melody_id: str, file_type: str, melody_uid: str | None = None) -> None: +async def delete_file(melody_id: str, file_type: str, melody_uid: str | None = None, melody_pid: str | None = None) -> None: """Delete a specific file from storage. file_type is 'binary' or 'preview'.""" if file_type == "binary": - delete_local_binary(melody_uid) + if melody_pid and not await _pid_used_by_other_melody(melody_pid, melody_id): + delete_local_binary(melody_pid) return bucket = get_bucket() @@ -431,9 +434,14 @@ def delete_file(melody_id: str, file_type: str, melody_uid: str | None = None) - blob.delete() -def _delete_storage_files(melody_id: str, melody_uid: str | None = None) -> None: - """Delete all storage files for a melody (local binary + Firebase preview).""" - delete_local_binary(melody_uid) +async def _delete_storage_files(melody_id: str, melody_uid: str | None = None, melody_pid: str | None = None) -> None: + """Delete all storage files for a melody (local binary + Firebase preview). + + The local binary is only deleted if no other melody still references the same + pid — multiple melodies can share one archetype binary by design. + """ + if melody_pid and not await _pid_used_by_other_melody(melody_pid, melody_id): + delete_local_binary(melody_pid) bucket = get_bucket() if not bucket: @@ -452,7 +460,7 @@ def get_storage_files(melody_id: str, melody_uid: str | None = None, melody_pid: preview still comes from Firebase Storage.""" result = {"binary_url": None, "preview_url": None} - if melody_uid and melody_pid and _binary_file_path(melody_uid).exists(): + if melody_pid and _binary_file_path(melody_pid).exists(): result["binary_url"] = f"{settings.melody_download_base_url}/{melody_pid}" bucket = get_bucket() diff --git a/backend/scripts/migrate_melody_binaries_to_local.py b/backend/scripts/migrate_melody_binaries_to_local.py new file mode 100644 index 0000000..ef7c734 --- /dev/null +++ b/backend/scripts/migrate_melody_binaries_to_local.py @@ -0,0 +1,262 @@ +""" +One-time migration: move every melody's .bsm binary off Firebase Storage and +onto local disk, updating melody.url to point at our own plain-HTTP download +endpoint instead of the Firebase public URL. + +Background: ESP32 devices can't afford the RAM for a TLS client, so melody +binaries are now served from ./storage/melody_binaries via +GET /api/melodies/download/{pid} (exposed publicly as +melodies.bellsystems.net/download/{pid}) instead of Firebase Storage's +HTTPS-only URLs. This script backfills existing melodies created before that +change — melodies created/edited after the change already use the new path +automatically (via "Select Archetype" / "Build on the Fly"). + +What this script does, for every melody whose url points at Firebase Storage: + 1. Downloads the .bsm bytes from the Firebase Storage URL + 2. Writes them to ./storage/melody_binaries/{pid}.bsm + 3. Updates melody.url -> http://melodies.bellsystems.net/download/{pid} + 4. Updates both SQLite (melody_drafts) and Firestore (if published) + +Storage is keyed on pid, not melody uid: pid identifies the underlying +archetype binary, and multiple melodies legitimately share one pid (each +remaps the same note sequence to different bells/speed/duration). The script +downloads each distinct pid's binary only once (from whichever melody it +encounters first) and reuses it for every other melody sharing that pid. + +CAVEAT: some existing melodies share a pid despite pointing at *different* +Firebase source files (observed in practice — e.g. voice-count variants like +1N_/2N_/3N_/4N_-prefixed filenames all filed under one pid). This script does +NOT attempt to detect or fix that — it flags it in the output (see "WARNING: +also seen with a different source file") so you can review and re-assign the +correct archetype per melody afterward using the in-app playback button. This +is a pre-existing PID data issue, not something safe to silently resolve here. + +Melodies with no pid are skipped and reported, since the new URL scheme +requires one (pid is the public lookup key for the download route). + +Run from the backend/ directory (or scripts/ — it searches upward for .env): + + docker exec -it bellsystems-backend python scripts/migrate_melody_binaries_to_local.py --dry-run + docker exec -it bellsystems-backend python scripts/migrate_melody_binaries_to_local.py + +Requires: firebase-admin, requests (both already in the backend image). +""" + +import argparse +import json +import os +import sqlite3 +import sys +from pathlib import Path + +import requests + +# Melody names may contain Greek/non-ASCII text — force UTF-8 stdout so this +# doesn't crash on Windows consoles defaulting to cp1252. +if hasattr(sys.stdout, "reconfigure"): + sys.stdout.reconfigure(encoding="utf-8", errors="replace") + + +# --------------------------------------------------------------------------- +# .env loader — searches upward from script location for a .env file +# --------------------------------------------------------------------------- + +def _load_env() -> dict: + search = Path(__file__).resolve().parent + for _ in range(4): + env_file = search / ".env" + if env_file.exists(): + result = {} + for line in env_file.read_text(encoding="utf-8").splitlines(): + line = line.strip() + if not line or line.startswith("#") or "=" not in line: + continue + key, _, val = line.partition("=") + result[key.strip()] = val.strip().strip('"').strip("'") + print(f"[INFO] Loaded config from {env_file}") + return result + search = search.parent + print("[WARN] No .env file found — relying on environment variables") + return {} + + +_env = _load_env() + + +def _cfg(key: str, default: str = "") -> str: + return _env.get(key) or os.environ.get(key) or default + + +# --------------------------------------------------------------------------- +# Firebase (only needed to read Firestore for published melodies) +# --------------------------------------------------------------------------- +try: + import firebase_admin + from firebase_admin import credentials, firestore + + _fb_app = None + + def _init_firebase(sa_path: str, bucket_name: str): + global _fb_app + if _fb_app is not None: + return + cred = credentials.Certificate(sa_path) + _fb_app = firebase_admin.initialize_app(cred, {"storageBucket": bucket_name}) + + def get_firestore(sa_path: str, bucket_name: str): + _init_firebase(sa_path, bucket_name) + return firestore.client() + + FIREBASE_AVAILABLE = True +except Exception as _fb_err: + print(f"[WARN] Firebase unavailable: {_fb_err}") + FIREBASE_AVAILABLE = False + + def get_firestore(sa_path: str, bucket_name: str): + return None + + +def _is_firebase_url(url: str) -> bool: + return bool(url) and ("firebasestorage" in url or "storage.googleapis.com" in url) + + +# --------------------------------------------------------------------------- +# Main migration +# --------------------------------------------------------------------------- + +def run(dry_run: bool = False, db_path: str = "", storage_dir: str = "", base_url: str = ""): + label = "[DRY-RUN]" if dry_run else "[LIVE]" + db_path = db_path or _cfg("SQLITE_DB_PATH", "./data/database.db") + storage_dir = Path(storage_dir or _cfg("MELODY_BINARIES_STORAGE_PATH", "./storage/melody_binaries")) + base_url = base_url or _cfg("MELODY_DOWNLOAD_BASE_URL", "http://melodies.bellsystems.net/download") + sa_path = _cfg("FIREBASE_SERVICE_ACCOUNT_PATH", "./firebase-service-account.json") + bucket_name = _cfg("FIREBASE_STORAGE_BUCKET") + + print(f"\n{label} Database: {db_path}") + print(f"{label} Local binary storage: {storage_dir}") + print(f"{label} New base URL: {base_url}\n") + + firestore_db = get_firestore(sa_path, bucket_name) if FIREBASE_AVAILABLE and bucket_name else None + + con = sqlite3.connect(db_path) + con.row_factory = sqlite3.Row + + rows = con.execute("SELECT * FROM melody_drafts").fetchall() + candidates = [] + for row in rows: + data = json.loads(row["data"]) + if _is_firebase_url(data.get("url", "")): + candidates.append((dict(row), data)) + + if not candidates: + print("No melodies with a Firebase Storage url found. Nothing to do.") + con.close() + return + + print(f"Found {len(candidates)} melody(ies) with a Firebase Storage binary url:\n") + + if not dry_run: + storage_dir.mkdir(parents=True, exist_ok=True) + + migrated = 0 + skipped_no_pid = 0 + failed = 0 + downloaded_pids: dict[str, bool] = {} # pid -> whether the local file is ready to use + pid_source_urls: dict[str, str] = {} # pid -> first-seen Firebase source url, to detect mismatches + mismatch_warnings = [] + + for row, data in candidates: + melody_id = row["id"] + pid = data.get("pid") + old_url = data.get("url") + name = (data.get("information") or {}).get("name", "") + + label_id = f"[{melody_id[:8]}]" + + if not pid: + print(f" {label_id} SKIPPED: no pid set — the new download route requires one (name: {name})") + skipped_no_pid += 1 + continue + + print(f" {label_id} {name!r} pid={pid}") + print(f" {old_url}") + + first_seen_url = pid_source_urls.get(pid) + if first_seen_url and first_seen_url != old_url: + warning = (f"pid '{pid}': melody {label_id} points at a DIFFERENT Firebase file " + f"than the one already saved for this pid — only the first one encountered " + f"is kept as {pid}.bsm. Verify with the playback button after migrating.") + print(f" WARNING: also seen with a different source file! {warning}") + mismatch_warnings.append(warning) + else: + pid_source_urls[pid] = old_url + + dest = storage_dir / f"{pid}.bsm" + new_url = f"{base_url}/{pid}" + + if dry_run: + if pid in downloaded_pids: + print(f" -> pid already handled by another melody in this run, would reuse {dest}") + else: + print(f" -> would download and save to {dest}") + print(f" -> would set url = {new_url}") + downloaded_pids[pid] = True + continue + + if pid not in downloaded_pids: + try: + resp = requests.get(old_url, timeout=30) + resp.raise_for_status() + except Exception as e: + print(f" ERROR downloading binary: {e}") + failed += 1 + continue + dest.write_bytes(resp.content) + print(f" saved {len(resp.content)} bytes -> {dest}") + downloaded_pids[pid] = True + else: + print(f" reusing already-downloaded {dest} (shared pid)") + + print(f" url -> {new_url}") + + data["url"] = new_url + con.execute( + "UPDATE melody_drafts SET data=? WHERE id=?", + (json.dumps(data), melody_id), + ) + con.commit() + + if row["status"] == "published": + if firestore_db: + try: + firestore_db.collection("melodies").document(melody_id).update({"url": new_url}) + print(f" Firestore updated") + except Exception as e: + print(f" ERROR updating Firestore: {e}") + else: + print(f" WARNING: melody is published but Firestore unavailable!") + + migrated += 1 + + con.close() + print(f"\n{'-'*60}") + print(f"{label} Done. Migrated: {migrated}, skipped (no pid): {skipped_no_pid}, failed: {failed}") + if mismatch_warnings: + print(f"\n{len(mismatch_warnings)} pid(s) had melodies pointing at different source files " + f"— please review these with the playback button:") + for w in mismatch_warnings: + print(f" - {w}") + if dry_run: + print("\nThis was a dry run. No changes were made. Run without --dry-run to apply.") + + +if __name__ == "__main__": + parser = argparse.ArgumentParser( + description="Migrate melody .bsm binaries from Firebase Storage to local disk" + ) + parser.add_argument("--dry-run", action="store_true", help="Preview changes without writing anything") + parser.add_argument("--db", default="", help="Override SQLite database path (default: read from .env)") + parser.add_argument("--storage-dir", default="", help="Override local binary storage dir (default: read from .env)") + parser.add_argument("--base-url", default="", help="Override download base URL (default: read from .env)") + args = parser.parse_args() + run(dry_run=args.dry_run, db_path=args.db, storage_dir=args.storage_dir, base_url=args.base_url) diff --git a/docs/melody-binary-serving.md b/docs/melody-binary-serving.md index f54160c..ddc5b54 100644 --- a/docs/melody-binary-serving.md +++ b/docs/melody-binary-serving.md @@ -12,28 +12,74 @@ This does **not** affect the audio preview file (`information.previewURL`) — t only ever fetched by the browser admin UI, which is already HTTPS, so it stays on Firebase Storage unchanged. +## PID vs UID — why storage is keyed on pid, not uid + +- **`pid`** ("Playback ID") identifies the underlying **archetype binary** — the raw + note sequence (A, B, C...) baked into a `.bsm` file. Multiple melodies can + legitimately share one `pid`: each melody remaps those notes to actual bells via its + own `noteAssignments`, speed, and duration settings. One binary can back many + different "remix" melodies. **Duplicate PIDs across melodies are correct and + expected, not a bug.** +- **`uid`** is meant to be each melody's own unique database identity. It is *not* + currently populated anywhere in `MelodyForm.jsx` — this is a pre-existing, separate + bug, deferred as its own task. Because of this, local binary storage is keyed on + `pid`, not `uid`. + ## What changed -- **Storage**: `.bsm` binaries are written to `./data/melody_binaries/{melody_uid}.bsm` - on the host (mounted into the backend container at `/app/storage/melody_binaries`, - same pattern as `./data/firmware` and `./data/built_melodies`). +- **Storage**: `.bsm` binaries are written to `./data/melody_binaries/{pid}.bsm` on + the host (mounted into the backend container at `/app/storage/melody_binaries`, + same pattern as `./data/firmware` and `./data/built_melodies`). Because storage is + keyed on `pid`, multiple melodies sharing a `pid` share one file — saving from any + of them overwrites the same file, which is expected. - **Upload path**: `SelectArchetypeModal` / `BuildOnTheFlyModal` still call `POST /api/melodies/{melodyId}/upload/binary` exactly as before. The backend (`backend/melodies/service.py::save_binary_for_melody`) now writes the bytes to local disk instead of uploading to Firebase Storage, and returns a URL like - `http://melodies.bellsystems.net/download/{pid}`. + `http://melodies.bellsystems.net/download/{pid}`. The melody must have a `pid` set + before uploading a binary (the endpoint returns 400 if not). - **Stored URL**: the melody's `url` field (Firestore + SQLite) now holds that plain-HTTP URL instead of a Firebase public URL. No schema change — `url` was already a plain string field. - **Download route**: `GET /api/melodies/download/{pid}` (`backend/melodies/router.py`) - is unauthenticated (devices have no login token) and resolves `pid` → the melody's - `{uid}.bsm` file on disk, same pattern as the existing firmware download route + is unauthenticated (devices have no login token) and resolves `pid` directly to + `{pid}.bsm` on disk, same pattern as the existing firmware download route (`backend/firmware/router.py::download_firmware`). -- **PID uniqueness**: since `pid` is now the public lookup key for the download route, - `backend/melodies/service.py::_check_pid_unique` rejects creating/renaming a melody - to a PID already used by another melody (409 Conflict). -- **Cleanup**: deleting a melody or its binary file now also deletes the local - `.bsm` file (`service.delete_local_binary`). +- **Deletion is share-aware**: deleting a melody or its binary + (`service.delete_file` / `service._delete_storage_files`) only removes the local + `.bsm` file if no *other* melody still references the same `pid` + (`service._pid_used_by_other_melody`). This prevents one melody's deletion from + breaking playback for other melodies sharing its archetype binary. + +## Migrating existing melodies + +Melodies created before this change still have a Firebase Storage URL in their `url` +field — deploying this code does not touch existing data. They keep working exactly +as before (still HTTPS to Firebase) until migrated. + +Two ways to move a melody to the new plain-HTTP URL: + +1. **Per-melody, via the UI**: open the melody and re-run "Select Archetype" or + "Build on the Fly" — this naturally re-uploads through the same endpoint, which now + writes to local disk and updates `url`. +2. **Bulk, via script**: `backend/scripts/migrate_melody_binaries_to_local.py` + downloads each melody's current Firebase binary and writes it to local disk under + its `pid`, then updates `url` (SQLite + Firestore if published). Run with + `--dry-run` first: + + ``` + docker exec -it bellsystems-backend python scripts/migrate_melody_binaries_to_local.py --dry-run + docker exec -it bellsystems-backend python scripts/migrate_melody_binaries_to_local.py + ``` + + **Known caveat**: some existing melodies share a `pid` despite their Firebase URLs + pointing at *different* source files (observed on real data — e.g. voice-count + variant filenames like `1N_`/`2N_`/`3N_`/`4N_` all filed under one `pid`). The + script downloads only the first melody's file for each `pid` and reuses it for + every other melody sharing that `pid` — it does not attempt to detect which file is + "correct". It prints a warning list of every `pid` where this happened; use each + melody's playback button afterward to verify the right archetype plays, and + manually re-assign the correct one via "Select Archetype" if it's wrong. ## Infrastructure (outside this repo) @@ -57,8 +103,8 @@ the NPM proxy host exists. ## Verification -1. Build/select an archetype for a melody → confirm a file appears at - `./data/melody_binaries/{uid}.bsm` and the melody's `url` becomes +1. Build/select an archetype for a melody with a `pid` set → confirm a file appears + at `./data/melody_binaries/{pid}.bsm` and the melody's `url` becomes `http://melodies.bellsystems.net/download/{pid}`. 2. `curl http://localhost:8000/api/melodies/download/{pid}` (direct to backend, bypassing nginx) → returns the `.bsm` bytes, no auth header needed. @@ -66,6 +112,8 @@ the NPM proxy host exists. console server block) → same result. 4. Once NPM is configured, `curl http://melodies.bellsystems.net/download/{pid}` → same result, over plain HTTP. -5. Delete the melody → confirm the local `.bsm` file is removed. +5. Delete a melody whose `pid` is *not* shared with any other melody → confirm the + local `.bsm` file is removed. Delete one of two melodies sharing a `pid` → confirm + the file survives until the last one is deleted. 6. Upload a preview audio file → confirm it still lands in Firebase Storage and `previewURL` still populates (regression check).