From 5145710a8a7f611c7817c236be1df61f3eb8b526 Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Mon, 22 Jun 2026 12:29:54 +0200 Subject: [PATCH] fix(stats): decode song_stats filenames so "Your best scores" reads real data (#564) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stats-recorder relays URL-encoded filenames (encodeURIComponent: '/'→'%2F', ' '→'%20') and POST /api/stats stored them verbatim, but the `songs` table — and every stats read that filters on `filename IN (SELECT filename FROM songs)` — keys on the decoded library path. So recorded plays landed under a non-matching key and were dropped by the filter: the profile "Your best scores" panel, the library accuracy badges (/api/stats/best) and "Jump back in" (/api/stats/recent) all read empty despite real history. PR #549/#550 wired the panel correctly; this fixes the data layer underneath it. - Canonicalize the filename to its decoded form on the write path (_decode_song_filename in api_record_stats). This also lets the arrangement-count bound resolve the real song. - One-time idempotent backfill (_migrate_decode_stat_filenames) that decodes existing rows, merging PK collisions with best=max / plays=sum / last-wins semantics. - Regression tests: encoded write surfaces in top/best/recent + per-song read; arrangement bound still applies; migration decodes + merges legacy rows and is idempotent. Verified against a copy of a real profile DB: top_stats went 0→5 rows, best-accuracy map 0→12, zero encoded ghosts left. Co-authored-by: Claude Opus 4.8 (1M context) --- server.py | 86 ++++++++++++++++++++++++++++ tests/test_song_stats_api.py | 105 +++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+) diff --git a/server.py b/server.py index b93aacd..4bd1f3a 100644 --- a/server.py +++ b/server.py @@ -580,6 +580,88 @@ class MetadataDB: self.conn.execute("INSERT OR IGNORE INTO wallet (id) VALUES (1)") self.conn.commit() self._lock = threading.Lock() + # One-time repair of pre-fix rows written under URL-encoded filenames + # (idempotent: a no-op once every row is canonical). + self._migrate_decode_stat_filenames() + + def _song_exists(self, filename: str) -> bool: + return self.conn.execute( + "SELECT 1 FROM songs WHERE filename = ?", (filename,)).fetchone() is not None + + def _canonical_song_filename(self, filename: str) -> str: + """Map a (possibly URL-encoded) filename to the `songs` library key. + + The recorder relays encodeURIComponent'd names ('/'→'%2F', ' '→'%20'), + but `songs` keys on the decoded on-disk path. Decoding is LIBRARY-AWARE so + a real filename that legitimately contains literal %XX is never corrupted: + prefer the form that already exists in `songs`, and decode only when the + decoded form resolves to a real song. When NEITHER form is in the library + (e.g. a play recorded before the library scan finishes) keep the stored + name unchanged — the next-startup migration canonicalizes it once the song + is scanned, rather than risk corrupting a real %XX name now.""" + if not isinstance(filename, str): + return filename + if self._song_exists(filename): + return filename # already a real library key (may contain %) + from urllib.parse import unquote + decoded = unquote(filename) + if decoded != filename and self._song_exists(decoded): + return decoded # encoded → real library key + return filename # neither in library: leave as-is (heals on migrate) + + def _migrate_decode_stat_filenames(self): + """Rewrite URL-encoded song_stats.filename rows to the decoded + library-path key (the form `songs` uses). Pre-fix, the recorder stored + encodeURIComponent'd names, so every recorded best was invisible to the + reads that filter on `filename IN (SELECT filename FROM songs)`. Merge on + collision — two encoded rows decoding to the same name, or an encoded row + meeting an already-decoded one — with the same best=max / plays=sum / + last-wins semantics as song_score.merge_stats, so the (filename, + arrangement) primary key is never violated. + + Library-aware via the shared _canonical_song_filename rule: only decode a + row when the decoded form is a real song, so a correctly-stored name + containing literal %XX is never rewritten, and dead-song/orphan rows + (neither form in the library) are left exactly as-is.""" + cols = self._STATS_COLS + with self._lock: + rows = [dict(zip(cols, r)) for r in self.conn.execute( + "SELECT " + ", ".join(cols) + " FROM song_stats").fetchall()] + canon = self._canonical_song_filename + if all(canon(r["filename"]) == r["filename"] for r in rows): + return # every row already canonical (or an untouchable orphan) + merged: dict = {} + for r in rows: + key = (canon(r["filename"]), int(r["arrangement"])) + cur = merged.get(key) + if cur is None: + merged[key] = dict(r, filename=key[0], arrangement=key[1]) + continue + # Most-recently-updated row wins the "last_*"/position fields. + def _stamp(x): + return str(x.get("updated_at") or x.get("last_played_at") or "") + newer = r if _stamp(r) >= _stamp(cur) else cur + merged[key] = { + "filename": key[0], "arrangement": key[1], + "plays": (cur["plays"] or 0) + (r["plays"] or 0), + "best_score": max(cur["best_score"] or 0, r["best_score"] or 0), + "best_accuracy": max(cur["best_accuracy"] or 0.0, r["best_accuracy"] or 0.0), + "last_score": newer["last_score"], "last_accuracy": newer["last_accuracy"], + "last_position": newer["last_position"], + "last_played_at": newer["last_played_at"], "updated_at": newer["updated_at"], + } + # Atomic swap: clear and reinsert the canonicalized set in one txn. + try: + self.conn.execute("DELETE FROM song_stats") + self.conn.executemany( + "INSERT INTO song_stats (" + ", ".join(cols) + ") VALUES (" + + ", ".join("?" * len(cols)) + ")", + [tuple(m[c] for c in cols) for m in merged.values()], + ) + self.conn.commit() + except Exception: + self.conn.rollback() + raise def is_favorite(self, filename: str) -> bool: return self.conn.execute("SELECT 1 FROM favorites WHERE filename = ?", (filename,)).fetchone() is not None @@ -4876,6 +4958,10 @@ def api_record_stats(data: dict): filename = _clean_str(data.get("filename")) if not filename: return JSONResponse({"error": "filename required"}, status_code=400) + # The recorder hands us URL-encoded filenames; canonicalize to the library + # key so stored rows line up with `songs` (and so the arrangement-count bound + # below resolves the real song). See MetadataDB._canonical_song_filename. + filename = meta_db._canonical_song_filename(filename) arr_raw = data.get("arrangement", 0) if arr_raw is None: arrangement = 0 diff --git a/tests/test_song_stats_api.py b/tests/test_song_stats_api.py index f9690c5..84542c6 100644 --- a/tests/test_song_stats_api.py +++ b/tests/test_song_stats_api.py @@ -339,6 +339,111 @@ def test_per_source_xp_reset_only_removes_that_source(client, server): assert db.reset_source_xp("minigames")["xp"] == 100 # idempotent +def test_encoded_filename_canonicalized_on_write(client, server): + # The recorder POSTs URL-encoded filenames (encodeURIComponent: '/'→'%2F', + # ' '→'%20'), but `songs` keys on the decoded path. The write path must + # canonicalize so the recorded play surfaces in every read that filters on + # `filename IN songs` — the original "Your best scores reads empty" bug. + decoded = "sloppak/My Song_Band.archive" + encoded = "sloppak%2FMy%20Song_Band.archive" + server.meta_db.put(decoded, 0, 0, {"title": "My Song", "artist": "Band"}) + r = client.post("/api/stats", json={"filename": encoded, "score": 700, "accuracy": 0.9}) + assert r.status_code == 200 + assert r.json()["stats"]["filename"] == decoded # stored under the decoded key + # Surfaces in the profile panel, the library badge map and Jump-back-in. + assert decoded in [x["filename"] for x in client.get("/api/stats/top").json()] + assert decoded in client.get("/api/stats/best").json() + assert decoded in [x["filename"] for x in client.get("/api/stats/recent").json()] + # The per-song read (path param Starlette already decodes) agrees. + assert client.get("/api/stats/" + decoded).json()["plays"] == 1 + + +def test_encoded_filename_arrangement_is_bounded(client, server): + # Canonicalizing first also lets the arrangement-count bound resolve the real + # song, so a bad index is still rejected when posted under an encoded name. + server.meta_db.put("sloppak/Two Arr.archive", 0, 0, + {"arrangements": [{"name": "Lead"}, {"name": "Bass"}]}) + enc = "sloppak%2FTwo%20Arr.archive" + assert client.post("/api/stats", json={"filename": enc, "arrangement": 5, + "score": 10, "accuracy": 0.5}).status_code == 400 + assert client.post("/api/stats", json={"filename": enc, "arrangement": 1, + "score": 10, "accuracy": 0.5}).status_code == 200 + + +def test_migration_decodes_and_merges_legacy_encoded_rows(client, server): + # Pre-fix rows were stored URL-encoded. The one-time migration must rewrite + # them to the decoded key AND merge any collision (best=max, plays=sum, + # last-wins) without violating the (filename, arrangement) PK. + db = server.meta_db + db.put("sloppak/Dup Song.archive", 0, 0, {"title": "Dup"}) + # A legacy encoded row + an already-decoded row for the SAME song/arr. + db.conn.execute( + "INSERT INTO song_stats (filename, arrangement, plays, best_score, best_accuracy, " + "last_score, last_accuracy, last_position, last_played_at, updated_at) " + "VALUES (?,?,?,?,?,?,?,?,?,?)", + ("sloppak%2FDup%20Song.archive", 0, 2, 500, 0.7, 500, 0.7, 0, "2025-01-01 00:00:00.000", "2025-01-01 00:00:00.000")) + db.conn.execute( + "INSERT INTO song_stats (filename, arrangement, plays, best_score, best_accuracy, " + "last_score, last_accuracy, last_position, last_played_at, updated_at) " + "VALUES (?,?,?,?,?,?,?,?,?,?)", + ("sloppak/Dup Song.archive", 0, 1, 900, 0.95, 900, 0.95, 12.0, "2025-02-02 00:00:00.000", "2025-02-02 00:00:00.000")) + db.conn.commit() + db._migrate_decode_stat_filenames() + # Exactly one merged row under the decoded key. + rows = db.conn.execute( + "SELECT plays, best_score, best_accuracy, last_score, last_position FROM song_stats " + "WHERE filename = ?", ("sloppak/Dup Song.archive",)).fetchall() + assert len(rows) == 1 + plays, best_score, best_acc, last_score, last_pos = rows[0] + assert plays == 3 # summed + assert best_score == 900 # max + assert best_acc == pytest.approx(0.95) # max + assert last_score == 900 and last_pos == pytest.approx(12.0) # newer (2025-02) wins + # No encoded ghost survives, and the row now shows up in the panel. + assert db.conn.execute( + "SELECT COUNT(*) FROM song_stats WHERE filename LIKE '%\\%2F%' ESCAPE '\\'").fetchone()[0] == 0 + assert "sloppak/Dup Song.archive" in [x["filename"] for x in client.get("/api/stats/top").json()] + # Idempotent: a second run is a clean no-op. + db._migrate_decode_stat_filenames() + assert db.conn.execute("SELECT COUNT(*) FROM song_stats WHERE filename = ?", + ("sloppak/Dup Song.archive",)).fetchone()[0] == 1 + + +def test_real_percent_filename_not_corrupted(client, server): + # A real on-disk song whose name legitimately contains "%20" must survive. + # The recorder encodeURIComponent's it (the literal '%' → '%25'), and the + # canonicalizer must resolve back to the REAL library key, not blindly + # unquote it into a different, non-existent name. + real = "sloppak/100%20Off_Band.archive" # literal %20 in the on-disk name + server.meta_db.put(real, 0, 0, {"title": "100% Off"}) + posted = "sloppak%2F100%2520Off_Band.archive" # encodeURIComponent(real) + r = client.post("/api/stats", json={"filename": posted, "score": 300, "accuracy": 0.8}) + assert r.status_code == 200 + assert r.json()["stats"]["filename"] == real # stored under the real key + assert real in [x["filename"] for x in client.get("/api/stats/top").json()] + + +def test_migration_leaves_real_percent_and_orphan_rows_untouched(client, server): + # Migration must NOT rewrite a correctly-stored name that happens to contain + # %XX (it's a real library key), nor a dead-song orphan whose neither form is + # in the library. + db = server.meta_db + real = "sloppak/50%20Pct.archive" + db.put(real, 0, 0, {"title": "Real"}) + for fn in (real, "ghost%2Fgone.archive"): # real-% key + orphan (no songs row either way) + db.conn.execute( + "INSERT INTO song_stats (filename, arrangement, plays, best_score, best_accuracy, " + "last_score, last_accuracy, last_position, last_played_at, updated_at) " + "VALUES (?,0,1,100,0.5,100,0.5,0,'2025-01-01 00:00:00.000','2025-01-01 00:00:00.000')", + (fn,)) + db.conn.commit() + db._migrate_decode_stat_filenames() + names = {r[0] for r in db.conn.execute("SELECT filename FROM song_stats").fetchall()} + assert real in names # real-% key preserved (not decoded away) + assert "ghost%2Fgone.archive" in names # orphan left exactly as-is + assert "sloppak/50 Pct.archive" not in names # never created the bogus decoded twin + + def test_award_xp_negative_reversal_clamps_at_zero(server): # A negative amount reverses a prior award (used when a minigames run's # profile-save fails) and never drives the total below zero.