diff --git a/README.md b/README.md index 4339d15..10ba4e7 100644 --- a/README.md +++ b/README.md @@ -7,22 +7,55 @@ which keeps an MP3 copy of a lossless library for an iPod. This one answers the question that mirror cannot: which of it is worth carrying, and which of it has not been played in years. -**This is stage one.** It ingests the scrobble history and nothing else. There -are no playlists yet, and nothing touches Lidarr or the music library. See -"Where this is going" below. +**This is stage two.** It ingests the scrobble history, indexes the library from +Lidarr, and matches one to the other. There are no playlists yet, and it writes +nothing back — every Lidarr call is a `GET`. See "Where this is going" below. ## What it does today - Pulls the full Last.fm scrobble history into SQLite, then keeps it current. - Tracks loved tracks separately, as the protected set for later stages. -- Reports what it holds, including the number that matters most: how many - scrobbles carry a MusicBrainz recording id. +- Indexes every artist, album and track Lidarr knows about, with file paths and + the date each file landed. +- Ties the two together and reports how well it managed. -That last figure decides the next stage. Lidarr exposes a `ForeignRecordingId` -on every track, which is the same identifier, so scrobbles carrying one can be -joined to the library exactly. The rest have to go through name matching, which -is where a curation tool goes wrong and starts recommending the deletion of -music you love. Measure the join rate before trusting the verdict. +## Matching + +Two tiers, and no third. + +| Tier | Key | Notes | +| ------ | --------------------------- | ----------------------------------------- | +| `mbid` | MusicBrainz recording id | Exact. Last.fm's per-scrobble `mbid` against Lidarr's `ForeignRecordingId` | +| `name` | Normalised artist and title | Everything the first tier could not carry | +| `none` | — | Recorded as a miss, never guessed at | + +The normalisation is the load-bearing part, because the two sides disagree in +predictable ways. It folds case and accents, drops guest credits (`Yellowcard +feat. Tay Jardine` against a tag of `Yellowcard`), strips a trailing +version suffix (`(Remastered 2011)`, `- Live`), expands `&`, and removes a +leading `The`. Punctuation gets two different rules that pull against each +other and are both required: apostrophes are **deleted**, so `Don't` meets +`Dont`, while every other mark becomes a **space**, so `AC/DC`, `AC-DC` and +`AC DC` all meet as well. + +It leans towards collapsing too much. A false match makes something look +played; a missed match makes something look abandoned. Only one of those +deletes music. + +### Reading the coverage report + +Matched against unmatched is the wrong comparison — most unmatched listening is +music that was never in the library, which says nothing at all about the +matcher. The line to watch is: + +``` +unmatched by an artist the library holds: N pairs, M plays +``` + +That is a track that was played, sitting beside a file it should have matched. +Those are the matcher's real misses, and every one is a candidate for being +wrongly called cold in stage four. The report lists the worst fifteen by play +count so they can be eyeballed. ## How the ingest works @@ -76,6 +109,9 @@ music-curator --report-only # report on the store, fetch nothing | `--interval` | `MUSIC_CURATOR_INTERVAL` | unset | Repeat forever, e.g. `45m`, `6h`, `1d` | | `--request-delay` | `MUSIC_CURATOR_REQUEST_DELAY` | `0.25` | Seconds between API requests | | `--backfill-limit` | `MUSIC_CURATOR_BACKFILL_LIMIT` | `0` | Cap backfill requests per pass; 0 for no cap | +| `--lidarr-url` | `MUSIC_CURATOR_LIDARR_URL` | unset | Lidarr base URL, e.g. `http://lidarr:8686` | +| `--lidarr-api-key` | `MUSIC_CURATOR_LIDARR_API_KEY` | unset | Lidarr API key | +| `--skip-index` | — | off | Match against the index already held | | `--report-only` | — | off | Report without fetching | A [Last.fm API key](https://www.last.fm/api/account/create) is all that is @@ -107,9 +143,11 @@ docker build --target test . # what CI runs pytest # needs pytest on PATH ``` -The suite runs against a fake transport that reproduces the real service's -paging, its `from`/`to` semantics and its awkward response shapes. No network, -no credentials, no rate limit. On a Nix machine: +The suite runs against fake transports for both services. The Last.fm one +reproduces its paging, its `from`/`to` semantics and its awkward response +shapes; the Lidarr one serves a canned library split across the same four +endpoints the indexer calls, so the stitching is exercised rather than +stubbed. No network, no credentials, no rate limit. On a Nix machine: ```sh nix shell nixpkgs#python3Packages.pytest -c pytest @@ -120,8 +158,8 @@ nix shell nixpkgs#python3Packages.pytest -c pytest | Stage | Status | | ------------------------------------------------ | ------------ | | Last.fm ingest and store | done | -| Lidarr index and the scrobble-to-track matcher | next | -| M3U playlists written into the mirror | after that | +| Lidarr index and the scrobble-to-track matcher | done | +| M3U playlists written into the mirror | next | | Cold-music report, unmonitoring what is not played | last | The cull will unmonitor cold albums in Lidarr and tag their artists. It will diff --git a/compose.yaml b/compose.yaml index 8485ab7..c0168d7 100644 --- a/compose.yaml +++ b/compose.yaml @@ -17,6 +17,10 @@ services: # enough, none of the endpoints used here authenticate a user. MUSIC_CURATOR_LASTFM_API_KEY: set-me-in-the-truenas-ui MUSIC_CURATOR_DB: /data/curator.db + # Lidarr, for indexing the library. Read-only: every call is a GET. + # Leave unset to ingest scrobbles and nothing else. + MUSIC_CURATOR_LIDARR_URL: http://lidarr:8686 + MUSIC_CURATOR_LIDARR_API_KEY: set-me-in-the-truenas-ui # How long to wait between passes. Each one catches up on new scrobbles # and continues the backfill if it has not finished. MUSIC_CURATOR_INTERVAL: 6h diff --git a/music_curator.py b/music_curator.py index 70dd132..2992d99 100644 --- a/music_curator.py +++ b/music_curator.py @@ -25,6 +25,7 @@ import signal import sqlite3 import sys import time +import unicodedata import urllib.error import urllib.parse import urllib.request @@ -54,7 +55,12 @@ REQUEST_DELAY_SECONDS = 0.25 BACKOFF_SECONDS = 2.0 BACKOFF_CEILING_SECONDS = 60.0 -SCHEMA_VERSION = "1" +SCHEMA_VERSION = "2" + +# Versions this build upgrades in place. Everything added since version 1 is a +# new table, and the schema script only ever creates what is missing, so running +# it is the whole migration -- an existing history is not re-downloaded. +MIGRATABLE_FROM = {"1"} SCHEMA = """ -- One row per scrobble. The primary key collapses two plays of the same track @@ -89,13 +95,149 @@ CREATE TABLE IF NOT EXISTS state ( key TEXT PRIMARY KEY, value TEXT NOT NULL ); + +-- The library as Lidarr sees it. Rebuilt from the API rather than by walking +-- the files, because Lidarr is the only thing holding the MusicBrainz ids that +-- make an exact join possible. +CREATE TABLE IF NOT EXISTS lidarr_artist ( + id INTEGER PRIMARY KEY, + mbid TEXT, + name TEXT NOT NULL, + norm_name TEXT NOT NULL, + path TEXT, + monitored INTEGER NOT NULL DEFAULT 1 +); +CREATE INDEX IF NOT EXISTS lidarr_artist_norm ON lidarr_artist (norm_name); + +CREATE TABLE IF NOT EXISTS lidarr_album ( + id INTEGER PRIMARY KEY, + artist_id INTEGER NOT NULL, + mbid TEXT, + title TEXT NOT NULL, + monitored INTEGER NOT NULL DEFAULT 1, + release_date TEXT +); +CREATE INDEX IF NOT EXISTS lidarr_album_artist ON lidarr_album (artist_id); + +CREATE TABLE IF NOT EXISTS lidarr_track ( + id INTEGER PRIMARY KEY, + artist_id INTEGER NOT NULL, + album_id INTEGER NOT NULL, + recording_mbid TEXT, + title TEXT NOT NULL, + norm_artist TEXT NOT NULL, + norm_title TEXT NOT NULL, + has_file INTEGER NOT NULL DEFAULT 0, + path TEXT, + added INTEGER, + duration INTEGER +); +CREATE INDEX IF NOT EXISTS lidarr_track_recording ON lidarr_track (recording_mbid); +CREATE INDEX IF NOT EXISTS lidarr_track_norm ON lidarr_track (norm_artist, norm_title); +CREATE INDEX IF NOT EXISTS lidarr_track_album ON lidarr_track (album_id); + +-- One row per distinct thing listened to, with the verdict on whether it could +-- be tied to the library. Matching belongs at this granularity rather than per +-- play: it is a property of the name pair, and there are three plays for every +-- pair. +CREATE TABLE IF NOT EXISTS scrobble_key ( + artist TEXT NOT NULL, + track TEXT NOT NULL, + norm_artist TEXT NOT NULL, + norm_track TEXT NOT NULL, + track_mbid TEXT, + plays INTEGER NOT NULL, + first_uts INTEGER NOT NULL, + last_uts INTEGER NOT NULL, + track_id INTEGER, + method TEXT NOT NULL DEFAULT 'none', + PRIMARY KEY (artist, track) +); +CREATE INDEX IF NOT EXISTS scrobble_key_track ON scrobble_key (track_id); """ +# Suffixes that a file tag carries and a scrobble does not, or the reverse. +# Stripped only from a trailing bracket or after a trailing dash, so a title +# like "(Don't Fear) The Reaper" keeps its opening parenthetical. +VERSION_WORDS = ( + "remaster", + "remastered", + "live", + "mono", + "stereo", + "version", + "edit", + "mix", + "remix", + "deluxe", + "bonus", + "explicit", + "acoustic", + "demo", + "radio", + "single", + "anniversary", + "reissue", + "instrumental", +) +_VERSIONS = "|".join(VERSION_WORDS) +BRACKETED_VERSION = re.compile( + rf"\s*[\(\[][^\)\]]*\b(?:{_VERSIONS})\b[^\)\]]*[\)\]]\s*$", re.IGNORECASE +) +TRAILING_VERSION = re.compile(rf"\s+-\s+[^-]*\b(?:{_VERSIONS})\b.*$", re.IGNORECASE) + +# Last.fm routinely carries the guest credit in the artist field where the file +# tag holds only the primary artist -- "Yellowcard feat. Tay Jardine" against a +# tag of "Yellowcard". `with` is deliberately absent: it appears in far too many +# real titles to cut on sight. +GUEST_CREDIT = re.compile(r"\s+(?:feat|ft|featuring)\b.*$", re.IGNORECASE) + +LEADING_ARTICLE = re.compile(r"^the\s+") +# Deleted rather than spaced, so "Don't" and "Dont" agree. Every other mark +# becomes a space instead, so "AC/DC", "AC-DC" and "AC DC" all agree too; the +# two rules pull opposite ways and both are needed. +APOSTROPHES = re.compile(r"['‘’ʼ`´]") +NOT_ALPHANUMERIC = re.compile(r"[^0-9a-z ]+") +WHITESPACE = re.compile(r"\s+") + + class LastfmError(Exception): """A Last.fm request that failed in a way retrying will not fix.""" +class LidarrError(Exception): + """A Lidarr request that failed.""" + + +def normalise(text): + """Return a comparison key for an artist or a track title. + + Scrobbles and file tags disagree in predictable ways, and every one of them + has to be flattened or a track that was played reads as one that was not. + That is the direction that costs music, so this leans towards collapsing too + much rather than too little: a false match makes something look played, a + missed match makes something look abandoned. + """ + if not text: + return "" + text = unicodedata.normalize("NFKD", str(text)).casefold() + text = "".join(character for character in text if not unicodedata.combining(character)) + text = GUEST_CREDIT.sub("", text) + # A title can carry more than one, e.g. "Song (Live) (Remastered)". + while True: + stripped = BRACKETED_VERSION.sub("", text) + if stripped == text: + break + text = stripped + text = TRAILING_VERSION.sub("", text) + text = text.replace("&", " and ") + text = APOSTROPHES.sub("", text) + text = NOT_ALPHANUMERIC.sub(" ", text) + text = WHITESPACE.sub(" ", text).strip() + return LEADING_ARTICLE.sub("", text) + + @dataclass(frozen=True) class Scrobble: """One play, as Last.fm recorded it.""" @@ -176,9 +318,10 @@ def parse_scrobble(entry): ) -def http_get(url, timeout=30): +def http_get(url, timeout=30, headers=None): """Fetch a URL and return its body. Replaced in tests.""" - with urllib.request.urlopen(url, timeout=timeout) as response: # noqa: S310 - fixed https root + request = urllib.request.Request(url, headers=headers or {}) + with urllib.request.urlopen(request, timeout=timeout) as response: # noqa: S310 return response.read().decode("utf-8") @@ -267,12 +410,17 @@ class Store: # the tail of it. self.connection.execute("PRAGMA journal_mode = WAL") self.connection.execute("PRAGMA synchronous = NORMAL") + # Registered so matching runs in SQL, rather than pulling twenty + # thousand name pairs into Python to compare them one at a time. + self.connection.create_function("normalise", 1, normalise, deterministic=True) self.connection.executescript(SCHEMA) self._check_version() def _check_version(self): held = self.get_state("schema_version") - if held is None: + if held is None or held in MIGRATABLE_FROM: + if held is not None: + logger.info("store migrated from schema version %s to %s", held, SCHEMA_VERSION) self.set_state("schema_version", SCHEMA_VERSION) elif held != SCHEMA_VERSION: raise RuntimeError( @@ -330,6 +478,58 @@ class Store: rows, ) + def replace_library(self, artists, albums, tracks): + """Swap in a freshly indexed library, in one transaction.""" + with self.connection: + for table in ("lidarr_track", "lidarr_album", "lidarr_artist"): + self.connection.execute(f"DELETE FROM {table}") + self.connection.executemany( + "INSERT INTO lidarr_artist (id, mbid, name, norm_name, path, monitored)" + " VALUES (?, ?, ?, ?, ?, ?)", + artists, + ) + self.connection.executemany( + "INSERT INTO lidarr_album" + " (id, artist_id, mbid, title, monitored, release_date)" + " VALUES (?, ?, ?, ?, ?, ?)", + albums, + ) + self.connection.executemany( + "INSERT INTO lidarr_track" + " (id, artist_id, album_id, recording_mbid, title, norm_artist, norm_title," + " has_file, path, added, duration)" + " VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + tracks, + ) + + def rebuild_keys(self): + """Collapse the scrobble history into one row per distinct track. + + Rows whose plays have not changed keep their existing verdict until the + matcher overwrites it; rows for tracks no longer in the history are + dropped, so an edited history does not leave orphans behind. + """ + with self.connection: + self.connection.execute( + "DELETE FROM scrobble_key WHERE (artist, track) NOT IN" + " (SELECT artist, track FROM scrobble)" + ) + self.connection.execute( + "INSERT INTO scrobble_key" + " (artist, track, norm_artist, norm_track, track_mbid, plays," + " first_uts, last_uts)" + " SELECT artist, track, normalise(artist), normalise(track)," + " MAX(track_mbid), COUNT(*), MIN(uts), MAX(uts)" + " FROM scrobble GROUP BY artist, track" + " ON CONFLICT (artist, track) DO UPDATE SET" + " norm_artist = excluded.norm_artist," + " norm_track = excluded.norm_track," + " track_mbid = excluded.track_mbid," + " plays = excluded.plays," + " first_uts = excluded.first_uts," + " last_uts = excluded.last_uts" + ) + def scalar(self, sql): return self.connection.execute(sql).fetchone()[0] @@ -445,6 +645,152 @@ def sync_loved(client, store, user): return len(rows) +class Lidarr: + """Minimal read-only Lidarr client. + + No retries: Lidarr is on the same LAN as this, and a failure there means it + is down or the key is wrong, neither of which improves on a second attempt. + """ + + def __init__(self, url, api_key, timeout=60, transport=None): + self.root = url.rstrip("/") + self.api_key = api_key + self.timeout = timeout + self.transport = transport or http_get + + def get(self, path, params=None): + """Return the decoded response for one API path.""" + query = urllib.parse.urlencode(params or {}) + url = f"{self.root}/api/v1/{path}" + (f"?{query}" if query else "") + try: + body = self.transport(url, timeout=self.timeout, headers={"X-Api-Key": self.api_key}) + except urllib.error.HTTPError as error: + raise LidarrError(f"{path}: HTTP {error.code}") from error + except (urllib.error.URLError, TimeoutError) as error: + raise LidarrError(f"{path}: {error}") from error + try: + return json.loads(body) + except json.JSONDecodeError as error: + raise LidarrError(f"{path}: malformed response") from error + + +def parse_added(value): + """Return a unix timestamp for a Lidarr ISO-8601 date, or None.""" + if not value: + return None + try: + return int(datetime.fromisoformat(str(value).replace("Z", "+00:00")).timestamp()) + except ValueError: + return None + + +def index_library(client, store): + """Rebuild the library index from Lidarr. Returns (artists, tracks). + + Rebuilt wholesale rather than reconciled: Lidarr is the authority, the index + is small, and a delete in Lidarr has to disappear here or it lingers as a + library entry with no file behind it. + """ + artists = client.get("artist") + artist_rows, album_rows, track_rows = [], [], [] + + for artist in artists: + artist_id = artist["id"] + name = artist.get("artistName") or "" + artist_rows.append( + ( + artist_id, + mbid_of(artist.get("foreignArtistId")), + name, + normalise(name), + artist.get("path"), + 1 if artist.get("monitored") else 0, + ) + ) + + for album in client.get("album", {"artistId": artist_id}): + album_rows.append( + ( + album["id"], + artist_id, + mbid_of(album.get("foreignAlbumId")), + album.get("title") or "", + 1 if album.get("monitored") else 0, + (album.get("releaseDate") or "")[:10] or None, + ) + ) + + # Paths and the date a track landed live on the file, not the track. + files = { + handle["id"]: handle for handle in client.get("trackfile", {"artistId": artist_id}) + } + for track in client.get("track", {"artistId": artist_id}): + handle = files.get(track.get("trackFileId") or 0) or {} + title = track.get("title") or "" + track_rows.append( + ( + track["id"], + artist_id, + track.get("albumId") or 0, + mbid_of(track.get("foreignRecordingId")), + title, + normalise(name), + normalise(title), + 1 if track.get("hasFile") else 0, + handle.get("path"), + parse_added(handle.get("dateAdded")), + track.get("duration"), + ) + ) + + store.replace_library(artist_rows, album_rows, track_rows) + logger.info( + "library indexed: %d artists, %d albums, %d tracks (%d with files)", + len(artist_rows), + len(album_rows), + len(track_rows), + sum(row[7] for row in track_rows), + ) + return len(artist_rows), len(track_rows) + + +def match_library(store): + """Tie every distinct thing listened to back to a library track. + + Two tiers. A MusicBrainz recording id is an exact join and is tried first; + everything else falls to the normalised name pair. There is no third tier on + purpose -- a guess that is nearly right is worse here than an admitted miss, + because the whole point of the number this produces is to say how far the + matching can be trusted. + """ + store.rebuild_keys() + with store.connection: + store.connection.execute("UPDATE scrobble_key SET track_id = NULL, method = 'none'") + store.connection.execute( + "UPDATE scrobble_key SET" + " track_id = (SELECT t.id FROM lidarr_track t" + " WHERE t.recording_mbid = scrobble_key.track_mbid" + " ORDER BY t.has_file DESC, t.id LIMIT 1)," + " method = 'mbid'" + " WHERE track_mbid IS NOT NULL" + " AND EXISTS (SELECT 1 FROM lidarr_track t" + " WHERE t.recording_mbid = scrobble_key.track_mbid)" + ) + store.connection.execute( + "UPDATE scrobble_key SET" + " track_id = (SELECT t.id FROM lidarr_track t" + " WHERE t.norm_artist = scrobble_key.norm_artist" + " AND t.norm_title = scrobble_key.norm_track" + " ORDER BY t.has_file DESC, t.id LIMIT 1)," + " method = 'name'" + " WHERE track_id IS NULL" + " AND norm_artist <> '' AND norm_track <> ''" + " AND EXISTS (SELECT 1 FROM lidarr_track t" + " WHERE t.norm_artist = scrobble_key.norm_artist" + " AND t.norm_title = scrobble_key.norm_track)" + ) + + def format_time(uts): """Return a UTC timestamp as a readable date.""" if uts is None: @@ -452,6 +798,74 @@ def format_time(uts): return datetime.fromtimestamp(uts, tz=timezone.utc).strftime("%Y-%m-%d %H:%M") +def coverage_report(store): + """Log how much of the listening history could be tied to the library. + + The split that matters is not matched against unmatched. Most unmatched + listening is music that was never in the library at all, which says nothing + about the matcher. The number to watch is unmatched listening by an artist + the library *does* hold: that is a track that was played, sitting next to a + file it should have matched, and every one of those is a candidate for being + wrongly called cold later on. + """ + tracks = store.scalar("SELECT COUNT(*) FROM lidarr_track") + if not tracks: + return + + pairs = store.scalar("SELECT COUNT(*) FROM scrobble_key") + if not pairs: + return + + logger.info("--- match coverage ---") + for method in ("mbid", "name", "none"): + row = store.connection.execute( + "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays" + " FROM scrobble_key WHERE method = ?", + (method,), + ).fetchone() + logger.info( + "%-5s: %6d pairs (%4.1f%%), %7d plays", + method, + row["pairs"], + 100 * row["pairs"] / pairs, + row["plays"], + ) + + suspect = store.connection.execute( + "SELECT COUNT(*) AS pairs, COALESCE(SUM(plays), 0) AS plays FROM scrobble_key k" + " WHERE k.track_id IS NULL" + " AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)" + ).fetchone() + logger.info( + "unmatched by an artist the library holds: %d pairs, %d plays -- these are the" + " matcher's misses, not music you do not own", + suspect["pairs"], + suspect["plays"], + ) + + with_files = store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE has_file = 1") + played = store.scalar( + "SELECT COUNT(DISTINCT k.track_id) FROM scrobble_key k" + " JOIN lidarr_track t ON t.id = k.track_id WHERE t.has_file = 1" + ) + logger.info( + "library tracks with a file: %d, of which played at least once: %d (%.1f%%)", + with_files, + played, + 100 * played / with_files if with_files else 0, + ) + + worst = store.connection.execute( + "SELECT k.artist, k.track, k.plays FROM scrobble_key k" + " WHERE k.track_id IS NULL" + " AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)" + " ORDER BY k.plays DESC, k.artist LIMIT 15" + ).fetchall() + for position, row in enumerate(worst, start=1): + label = f"{row['artist']} - {row['track']}" + logger.info(" unmatched %2d: %-60s %d plays", position, label[:60], row["plays"]) + + def report(store, now): """Log what the store holds. @@ -485,6 +899,8 @@ def report(store, now): for position, row in enumerate(top, start=1): logger.info(" top artist %2d: %-40s %d", position, row["artist"][:40], row["plays"]) + coverage_report(store) + recent = store.connection.execute( "SELECT artist, track, COUNT(*) AS plays FROM scrobble WHERE uts >= ?" " GROUP BY artist, track ORDER BY plays DESC, artist LIMIT 10", @@ -495,12 +911,20 @@ def report(store, now): logger.info(" top track 90d %2d: %-50s %d", position, label[:50], row["plays"]) -def run_once(client, store, user, now, backfill_limit): - """Run a single ingest pass. Returns the number of scrobbles added.""" +def run_once(client, store, user, now, backfill_limit, lidarr=None): + """Run a single pass. Returns the number of scrobbles added.""" started = time.monotonic() added = catch_up(client, store, user, now) added += backfill(client, store, user, now, limit=backfill_limit) loved = sync_loved(client, store, user) + + if lidarr is not None: + index_library(lidarr, store) + # Matching is worth redoing even without a fresh index: new scrobbles have + # arrived, and they need a verdict too. + if store.scalar("SELECT COUNT(*) FROM lidarr_track"): + match_library(store) + logger.info( "pass complete in %.1fs: %d scrobbles added, %d loved", time.monotonic() - started, @@ -562,6 +986,21 @@ def build_parser(): help="cap the backfill at this many requests per pass; 0 for no cap" " (env MUSIC_CURATOR_BACKFILL_LIMIT)", ) + parser.add_argument( + "--lidarr-url", + default=os.getenv("MUSIC_CURATOR_LIDARR_URL"), + help="base URL of Lidarr, e.g. http://lidarr:8686 (env MUSIC_CURATOR_LIDARR_URL)", + ) + parser.add_argument( + "--lidarr-api-key", + default=os.getenv("MUSIC_CURATOR_LIDARR_API_KEY"), + help="Lidarr API key (env MUSIC_CURATOR_LIDARR_API_KEY)", + ) + parser.add_argument( + "--skip-index", + action="store_true", + help="do not re-index the library this pass; match against the index already held", + ) parser.add_argument( "--report-only", action="store_true", @@ -601,6 +1040,12 @@ def main(argv=None, clock=time.time): client = Lastfm(args.api_key, delay=args.request_delay) + lidarr = None + if args.lidarr_url and args.lidarr_api_key and not args.skip_index: + lidarr = Lidarr(args.lidarr_url, args.lidarr_api_key) + elif not args.lidarr_url: + logger.info("no Lidarr URL configured; ingesting scrobbles only") + stopping = False def stop(signum, _frame): @@ -615,8 +1060,8 @@ def main(argv=None, clock=time.time): while True: now = int(clock()) try: - run_once(client, store, args.user, now, args.backfill_limit) - except LastfmError as error: + run_once(client, store, args.user, now, args.backfill_limit, lidarr) + except (LastfmError, LidarrError) as error: logger.error("%s", error) if interval is None: return 1 diff --git a/tests/conftest.py b/tests/conftest.py index a010d6e..e44e1cc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -120,6 +120,83 @@ class FakeLastfm: } +class FakeLidarr: + """A transport serving a canned library over the Lidarr v1 API surface. + + Built from a nested description -- artist, album, tracks -- and split back + out across the four endpoints the indexer actually calls, so the tests + exercise the same stitching the real thing does. + """ + + def __init__(self, artists=()): + self.artists, self.albums, self.tracks, self.files = [], [], [], [] + for artist_index, entry in enumerate(artists, start=1): + artist_id = artist_index + self.artists.append( + { + "id": artist_id, + "artistName": entry["name"], + "foreignArtistId": entry.get("mbid", f"artist-mbid-{artist_id}"), + "path": f"/music/{entry['name']}", + "monitored": entry.get("monitored", True), + } + ) + for album_index, album in enumerate(entry.get("albums", []), start=1): + album_id = artist_id * 100 + album_index + self.albums.append( + { + "id": album_id, + "artistId": artist_id, + "title": album["title"], + "foreignAlbumId": f"album-mbid-{album_id}", + "monitored": album.get("monitored", True), + "releaseDate": album.get("release_date", "2019-01-01T00:00:00Z"), + } + ) + for track_index, track in enumerate(album.get("tracks", []), start=1): + track_id = album_id * 100 + track_index + has_file = track.get("has_file", True) + self.tracks.append( + { + "id": track_id, + "artistId": artist_id, + "albumId": album_id, + "title": track["title"], + "foreignRecordingId": track.get("recording_mbid", ""), + "trackFileId": track_id if has_file else 0, + "hasFile": has_file, + "duration": 210000, + } + ) + if has_file: + self.files.append( + { + "id": track_id, + "artistId": artist_id, + "albumId": album_id, + "path": f"/music/{entry['name']}/{album['title']}/" + f"{track['title']}.flac", + "dateAdded": track.get("added", "2020-05-01T12:00:00Z"), + } + ) + self.calls = [] + + def __call__(self, url, timeout=None, headers=None): + parsed = urllib.parse.urlparse(url) + assert (headers or {}).get("X-Api-Key"), "Lidarr requires the API key header" + path = parsed.path.rsplit("/", 1)[-1] + query = { + key: value[0] for key, value in urllib.parse.parse_qs(parsed.query).items() + } + self.calls.append((path, query)) + + if path == "artist": + return json.dumps(self.artists) + artist_id = int(query.get("artistId", 0)) + source = {"album": self.albums, "track": self.tracks, "trackfile": self.files}[path] + return json.dumps([row for row in source if row["artistId"] == artist_id]) + + @pytest.fixture def now_playing(): """The entry Last.fm prepends for a track in progress: no `date` at all.""" diff --git a/tests/test_music_curator.py b/tests/test_music_curator.py index 6034479..56e05e0 100644 --- a/tests/test_music_curator.py +++ b/tests/test_music_curator.py @@ -1,7 +1,7 @@ import urllib.error import pytest -from conftest import FakeLastfm, make_loved, make_tracks +from conftest import FakeLastfm, FakeLidarr, make_loved, make_tracks import music_curator @@ -23,6 +23,62 @@ def ingest(store, api, backfill_limit=0): return music_curator.run_once(client, store, "lyra", NOW, backfill_limit) +def scrobble_of(artist, track, uts=NOW - 3600, mbid=""): + """One recent-tracks entry, in the shape Last.fm actually sends.""" + return { + "artist": {"#text": artist, "mbid": ""}, + "album": {"#text": "Some Album", "mbid": ""}, + "name": track, + "mbid": mbid, + "date": {"uts": str(uts)}, + } + + +LIBRARY = [ + { + "name": "Yellowcard", + "albums": [ + { + "title": "Lift a Sail", + "tracks": [ + {"title": "Here I Am Alive", "recording_mbid": "rec-alive"}, + {"title": "Transmission Home"}, + ], + } + ], + }, + { + "name": "AC/DC", + "albums": [{"title": "Back in Black", "tracks": [{"title": "Hells Bells"}]}], + }, + { + "name": "The Prodigy", + "albums": [ + {"title": "The Fat of the Land", "tracks": [{"title": "Breathe (Remastered)"}]} + ], + }, +] + + +def indexed(tmp_path, scrobbles): + """Ingest scrobbles, index the canned library, and match the two.""" + store = store_at(tmp_path) + ingest(store, FakeLastfm(scrobbles)) + music_curator.index_library( + music_curator.Lidarr("http://lidarr", "key", transport=FakeLidarr(LIBRARY)), store + ) + music_curator.match_library(store) + return store + + +def verdict(store, artist, track): + row = store.connection.execute( + "SELECT method, track_id FROM scrobble_key WHERE artist = ? AND track = ?", + (artist, track), + ).fetchone() + return (row["method"], row["track_id"]) if row else (None, None) + + def test_parse_interval_units(): assert music_curator.parse_interval("90") == 90 assert music_curator.parse_interval("30m") == 1800 @@ -241,3 +297,206 @@ def test_main_ingests_through_the_module_level_fetcher(tmp_path, monkeypatch): assert code == 0 assert store_at(tmp_path).count() == 20 + + +def test_normalise_flattens_the_ways_tags_and_scrobbles_disagree(): + assert music_curator.normalise("Yellowcard feat. Tay Jardine") == "yellowcard" + assert music_curator.normalise("BABYMETAL Feat. F.Hero") == "babymetal" + assert music_curator.normalise("AC/DC") == "ac dc" + assert music_curator.normalise("The Prodigy") == "prodigy" + assert music_curator.normalise("Beyoncé") == "beyonce" + assert music_curator.normalise("Simon & Garfunkel") == "simon and garfunkel" + assert music_curator.normalise("Breathe (Remastered 2011)") == "breathe" + assert music_curator.normalise("Breathe (Live) (Remastered)") == "breathe" + assert music_curator.normalise("Vice Grip - Live") == "vice grip" + + +def test_normalise_keeps_a_leading_parenthetical(): + """Stripping every bracket would destroy real titles.""" + assert music_curator.normalise("(Don't Fear) The Reaper") == "dont fear the reaper" + + +def test_normalise_treats_apostrophes_and_other_punctuation_differently(): + """The two rules pull opposite ways: one deletes, the other separates.""" + assert music_curator.normalise("Don't") == music_curator.normalise("Dont") + assert music_curator.normalise("Don’t") == music_curator.normalise("Dont") + assert music_curator.normalise("AC/DC") == music_curator.normalise("AC-DC") + assert music_curator.normalise("AC/DC") == music_curator.normalise("AC DC") + # ...and they must not collide with each other. + assert music_curator.normalise("AC/DC") != music_curator.normalise("ACDC") + + +def test_normalise_leaves_with_alone(): + """`with` appears in too many genuine titles to cut on sight.""" + assert music_curator.normalise("Dancing with Myself") == "dancing with myself" + + +def test_a_recording_mbid_matches_exactly(tmp_path): + store = indexed(tmp_path, [scrobble_of("Yellowcard", "Here I Am Alive", mbid="rec-alive")]) + + assert verdict(store, "Yellowcard", "Here I Am Alive")[0] == "mbid" + + +def test_a_guest_credit_still_matches_by_name(tmp_path): + """Last.fm puts the guest in the artist field; the file tag does not.""" + store = indexed( + tmp_path, [scrobble_of("Yellowcard feat. Tay Jardine", "Here I Am Alive")] + ) + + method, track_id = verdict(store, "Yellowcard feat. Tay Jardine", "Here I Am Alive") + assert method == "name" + assert track_id is not None + + +def test_a_remaster_suffix_on_the_library_side_still_matches(tmp_path): + store = indexed(tmp_path, [scrobble_of("The Prodigy", "Breathe")]) + + assert verdict(store, "The Prodigy", "Breathe")[0] == "name" + + +def test_music_that_is_not_in_the_library_stays_unmatched(tmp_path): + store = indexed(tmp_path, [scrobble_of("Some Band", "Some Song")]) + + assert verdict(store, "Some Band", "Some Song") == ("none", None) + + +def test_the_mbid_tier_is_preferred_over_the_name_tier(tmp_path): + store = indexed( + tmp_path, + [scrobble_of("Yellowcard", "Here I Am Alive", mbid="rec-alive")], + ) + method, track_id = verdict(store, "Yellowcard", "Here I Am Alive") + + expected = store.connection.execute( + "SELECT id FROM lidarr_track WHERE recording_mbid = 'rec-alive'" + ).fetchone()["id"] + assert (method, track_id) == ("mbid", expected) + + +def test_keys_carry_the_play_count_and_the_span(tmp_path): + store = indexed( + tmp_path, + [ + scrobble_of("AC/DC", "Hells Bells", uts=NOW - 7200), + scrobble_of("AC/DC", "Hells Bells", uts=NOW - 3600), + ], + ) + + row = store.connection.execute( + "SELECT plays, first_uts, last_uts FROM scrobble_key WHERE artist = 'AC/DC'" + ).fetchone() + assert row["plays"] == 2 + assert row["last_uts"] - row["first_uts"] == 3600 + + +def test_re_indexing_drops_what_lidarr_no_longer_has(tmp_path): + """The index is Lidarr's mirror, not an accumulation of everything ever seen.""" + store = indexed(tmp_path, [scrobble_of("AC/DC", "Hells Bells")]) + assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 3 + + music_curator.index_library( + music_curator.Lidarr("http://lidarr", "key", transport=FakeLidarr(LIBRARY[:1])), store + ) + + assert store.scalar("SELECT COUNT(*) FROM lidarr_artist") == 1 + assert store.scalar("SELECT COUNT(*) FROM lidarr_track WHERE artist_id NOT IN" + " (SELECT id FROM lidarr_artist)") == 0 + + +def test_the_index_records_paths_and_when_a_file_landed(tmp_path): + store = indexed(tmp_path, []) + + row = store.connection.execute( + "SELECT path, added, has_file FROM lidarr_track WHERE title = 'Hells Bells'" + ).fetchone() + assert row["path"].endswith("Hells Bells.flac") + assert row["has_file"] == 1 + assert row["added"] == music_curator.parse_added("2020-05-01T12:00:00Z") + + +def test_a_track_without_a_file_has_no_path(tmp_path): + library = [ + { + "name": "Ghost Artist", + "albums": [{"title": "Unowned", "tracks": [{"title": "Missing", "has_file": False}]}], + } + ] + store = store_at(tmp_path) + music_curator.index_library( + music_curator.Lidarr("http://lidarr", "key", transport=FakeLidarr(library)), store + ) + + row = store.connection.execute("SELECT path, has_file FROM lidarr_track").fetchone() + assert row["has_file"] == 0 + assert row["path"] is None + + +def test_lidarr_requires_the_api_key_header(tmp_path): + """The fake asserts on it, which is the point: a missing header is a 401.""" + client = music_curator.Lidarr("http://lidarr", "key", transport=FakeLidarr(LIBRARY)) + + assert client.get("artist") + + +def test_a_lidarr_failure_is_reported_not_swallowed(): + def broken(url, timeout=None, headers=None): + raise urllib.error.HTTPError(url, 401, "unauthorised", {}, None) + + client = music_curator.Lidarr("http://lidarr", "wrong", transport=broken) + + with pytest.raises(music_curator.LidarrError, match="HTTP 401"): + client.get("artist") + + +def test_an_existing_version_one_store_is_migrated_not_discarded(tmp_path): + """A rebuilt history is thousands of API requests; migration is additive.""" + store = store_at(tmp_path) + store.add_scrobbles([music_curator.Scrobble(uts=NOW, artist="A", track="B")]) + store.set_state("schema_version", "1") + store.close() + + reopened = store_at(tmp_path) + + assert reopened.count() == 1 + assert reopened.get_state("schema_version") == music_curator.SCHEMA_VERSION + + +def test_an_unmatched_track_by_a_known_artist_is_a_matcher_miss(tmp_path): + """The split that matters: music you own and played, that failed to match.""" + store = indexed( + tmp_path, + [ + scrobble_of("AC/DC", "A Track Lidarr Has Never Heard Of"), + scrobble_of("Some Band", "Some Song"), + ], + ) + + misses = store.connection.execute( + "SELECT k.artist FROM scrobble_key k WHERE k.track_id IS NULL" + " AND EXISTS (SELECT 1 FROM lidarr_artist a WHERE a.norm_name = k.norm_artist)" + ).fetchall() + + # Only the AC/DC one: "Some Band" is not in the library, so it says nothing + # about the matcher. + assert [row["artist"] for row in misses] == ["AC/DC"] + + +def test_the_report_runs_over_a_matched_store(tmp_path): + store = indexed(tmp_path, [scrobble_of("Yellowcard", "Here I Am Alive", mbid="rec-alive")]) + + music_curator.report(store, NOW) + + +def test_matching_is_redone_when_new_scrobbles_arrive(tmp_path): + """A pass with no re-index still has to give the new plays a verdict.""" + store = indexed(tmp_path, [scrobble_of("AC/DC", "Hells Bells")]) + api = FakeLastfm( + [ + scrobble_of("AC/DC", "Hells Bells"), + scrobble_of("Yellowcard", "Transmission Home", uts=NOW - 60), + ] + ) + + music_curator.run_once(client_for(api), store, "lyra", NOW, 0) + + assert verdict(store, "Yellowcard", "Transmission Home")[0] == "name"