From f39392fa1c084f954717f201efc42eddb593bc7a Mon Sep 17 00:00:00 2001 From: Ismael Leon Date: Sat, 12 Sep 2026 01:55:22 -0600 Subject: [PATCH] Search with the song's name, not the video's title MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `!lyrics` worked for some songs and did nothing at all for others. Two separate faults, and the second is what made the first invisible. **It searched with the raw video title and the channel name.** Reported against the live bot: "The Police - Every Breath You Take" got no answer. What the bot actually asked LRCLIB for was the title as YouTube stores it and the uploader as the artist, and measured against the real service the difference is total: "Music n Lyrics" / "The Police Every Breath You Take (Lyrics)" -> HTTP 404 "The Police" / "Every Breath You Take" -> synced So `search_terms` tidies a video title into something a lyrics database recognises: it drops video furniture — `(Official Video)`, `(Lyrics)`, `[4K]`, `(Remastered)` — drops featured credits and trailing album tags, and splits the artist off the front. It trusts the title over the channel when the title names both, because a channel called "Music n Lyrics" is not who recorded the song. A run of two or more spaces counts as a separator, which is exactly the shape the reported title had. It leaves alone what it cannot be sure about: `(Acoustic Version)` is part of a song's name, not furniture, and stays. **The command answered nothing because it crashed.** The cog's own class is called `Lyrics`, and it shadowed the dataclass imported under the same name, so the Genius fallback built a Cog and raised: TypeError: Lyrics.__init__() got an unexpected keyword argument 'title' No test reached that path — the follower's tests inject a loader and never call `_find`. `tests/test_lyrics_cog.py` covers it now, and the dataclass is reached through the module rather than imported bare. **And a crash should never be silence.** `utils.errors` logged unexpected failures and deliberately said nothing, on the grounds that a bug is the operator's problem. That is half right: the cause belongs in the log, but from the channel a crash was indistinguishable from the bot being offline — which is the exact thing that module's own docstring opens by warning about. Unexpected errors are now acknowledged with a plain sentence, no internals, traceback still logged. The test that asserted the old silence now asserts the new answer. Also: a typed query is tried both ways round. The help documents ` - <artist>`, but `The Police - Every Breath You Take` is how people actually type it, and the second order only costs a request when the first found nothing. Verified against the real LRCLIB with the reported track: 74 synced lines and the right window, where before there was no answer at all. Tests: 476, up from 427. --- cogs/lyrics.py | 49 ++++++++---- services/synced_lyrics.py | 53 ++++++++++++- tests/test_errors.py | 21 +++++- tests/test_lyrics_cog.py | 150 +++++++++++++++++++++++++++++++++++++ tests/test_search_terms.py | 127 +++++++++++++++++++++++++++++++ utils/errors.py | 7 +- 6 files changed, 385 insertions(+), 22 deletions(-) create mode 100644 tests/test_lyrics_cog.py create mode 100644 tests/test_search_terms.py diff --git a/cogs/lyrics.py b/cogs/lyrics.py index c4ed139..1251b36 100644 --- a/cogs/lyrics.py +++ b/cogs/lyrics.py @@ -8,11 +8,17 @@ from discord.ext import commands from services import lyrics_api, synced_lyrics -from services.synced_lyrics import Lyrics, index_at +# Deliberately not `from ... import Lyrics`: the cog class below is called +# Lyrics too, and importing the bare name let it shadow the dataclass. The +# Genius fallback then built a Cog instead of a result and raised TypeError, +# which the command reported as nothing at all. +from services.synced_lyrics import index_at, search_terms from utils.embeds import (error_embed, info_embed, lyrics_embed, lyrics_pages, synced_lyrics_embed) from utils.player import players +LyricsResult = synced_lyrics.Lyrics + log = logging.getLogger("loopify.lyrics") # Discord allows roughly five message edits per five seconds per channel, and a @@ -28,7 +34,7 @@ MAX_SLEEP = 2.0 MIN_SLEEP = 0.25 -Loader = Callable[[dict], Awaitable[Optional[Lyrics]]] +Loader = Callable[[dict], Awaitable[Optional[LyricsResult]]] class LyricsFollower: @@ -55,7 +61,7 @@ def stop(self) -> None: async def run(self) -> None: """Follow the music until the song, the player or the message runs out.""" track: Optional[dict] = None - lyrics: Optional[Lyrics] = None + lyrics: Optional[LyricsResult] = None shown: Optional[int] = None last_edit = float("-inf") @@ -80,7 +86,7 @@ async def run(self) -> None: await self._sleep(self._until_next_line(lyrics.lines, index, position)) - async def _show(self, lyrics: Lyrics, index: int, position: float, + async def _show(self, lyrics: LyricsResult, index: int, position: float, track: Optional[dict]) -> bool: """Redraw the message. False means it is gone and we should stop.""" try: @@ -183,7 +189,7 @@ def _split_query(query: str) -> tuple[str, str]: return query.strip(), "" async def _find(self, title: str, artist: str, - duration: Optional[float]) -> Optional[Lyrics]: + duration: Optional[float]) -> Optional[LyricsResult]: """LRCLIB first, since it is the only source with timings, then Genius.""" found = await synced_lyrics.fetch(self._session, title, artist, duration) if found is not None: @@ -191,13 +197,28 @@ async def _find(self, title: str, artist: str, fallback = await lyrics_api.fetch(title, artist) if fallback is None: return None - return Lyrics(title=fallback["title"], artist=fallback["artist"], - plain=fallback["lyrics"]) + return LyricsResult(title=fallback["title"], artist=fallback["artist"], + plain=fallback["lyrics"]) + + async def _search(self, query: str) -> Optional[LyricsResult]: + """ + Look up a typed query, in either order. - async def _for_track(self, track: dict) -> Optional[Lyrics]: - return await self._find(track.get("title", ""), - track.get("uploader") or "", - track.get("duration")) + The help documents ``<title> - <artist>``, but ``The Police - Every + Breath You Take`` is how people actually type it. The documented order + is tried first, and the other only if it found nothing. + """ + title, artist = self._split_query(query) + found = await self._find(title, artist, None) + if found is None and artist: + found = await self._find(artist, title, None) + return found + + async def _for_track(self, track: dict) -> Optional[LyricsResult]: + """Look a playing track up, with its title tidied into search terms.""" + title, artist = search_terms(track.get("title") or "", + track.get("uploader") or "") + return await self._find(title, artist, track.get("duration")) # -- Following ----------------------------------------------------- @@ -211,7 +232,7 @@ def stop_following(self, guild_id: int) -> None: if not task.done(): task.cancel() - async def _follow(self, ctx, player, lyrics: Lyrics) -> None: + async def _follow(self, ctx, player, lyrics: LyricsResult) -> None: """Post the live message and start keeping it up to date.""" self.stop_following(ctx.guild.id) # one per guild; the newest wins index = index_at(lyrics.lines, player.position) @@ -243,8 +264,8 @@ async def lyrics(self, ctx, *, query: str = None): async with ctx.typing(): player = players.get(ctx.guild.id) if query: - title, artist = self._split_query(query) - found = await self._find(title, artist, None) + title = self._split_query(query)[0] + found = await self._search(query) elif player and player.current: title = player.current.get("title", "") found = await self._for_track(player.current) diff --git a/services/synced_lyrics.py b/services/synced_lyrics.py index f5ca5f8..c42d79b 100644 --- a/services/synced_lyrics.py +++ b/services/synced_lyrics.py @@ -10,12 +10,12 @@ import logging import re - -import aiohttp from bisect import bisect_right from dataclasses import dataclass from typing import Optional +import aiohttp + log = logging.getLogger("loopify.synced") API_URL = "https://lrclib.net/api/get" @@ -56,6 +56,55 @@ def parse_lrc(body: str) -> tuple[Line, ...]: return tuple(sorted(lines, key=lambda line: line[0])) +# Words that mark a bracketed chunk as video furniture rather than part of the +# song's name. "(Acoustic Version)" is the song; "(Official Video)" is not. +_FURNITURE = re.compile( + r"[(\[][^)\]]*\b(?:official|video|audio|lyrics?|visuali[sz]er" + r"|remaster(?:ed)?|hd|hq|4k|8k|mv|explicit|clean|live|oficial|letra" + r"|legendado|sub(?:titulado)?|full album|color coded)\b[^)\]]*[)\]]", + re.IGNORECASE) +# "Song ft. Someone" — databases file a track under its lead artist alone. +_CREDITS = re.compile(r"\s*\b(?:ft|feat)\.?\s.*$", re.IGNORECASE) +# A trailing "| Album Name" or "| Official Video". +_TRAILING_PIPE = re.compile(r"\s*\|.*$") +# What separates an artist from a title. A run of two or more spaces counts: +# plenty of uploads write "The Police Every Breath You Take" with no dash. +_SEPARATOR = re.compile(r"\s[-–—]\s|\s{2,}") +# Suffixes YouTube itself adds to a channel name. +_CHANNEL_NOISE = re.compile(r"\s*-\s*Topic$|\s*VEVO$|\s*Official$", re.IGNORECASE) + + +def _tidy(text: str) -> str: + return " ".join(text.split()) + + +def search_terms(title: str, uploader: str = "") -> tuple[str, str]: + """ + Turn a video title into the ``(track, artist)`` a lyrics database expects. + + Raw YouTube titles are not song names: they carry video furniture, featured + credits and album tags, and the channel is often an aggregator rather than + the artist. Searching with them as-is is why some songs came back empty + while others worked — measured against LRCLIB, the raw form 404s where the + tidied one returns synced lyrics. + + The title is trusted over the channel when it names both, because a channel + called "Music n Lyrics" is not who recorded the song. + """ + # Whitespace is collapsed only at the end: a run of two or more spaces is + # itself a separator, and tidying first would erase it. + cleaned = _CREDITS.sub("", _TRAILING_PIPE.sub("", _FURNITURE.sub("", title))).strip() + if not cleaned: + cleaned = title.strip() # all furniture: better to search it than nothing + + artist = _CHANNEL_NOISE.sub("", uploader) + halves = _SEPARATOR.split(cleaned, maxsplit=1) + if len(halves) == 2 and halves[0].strip() and halves[1].strip(): + artist, cleaned = halves + + return _tidy(cleaned).strip(" -–—"), _tidy(artist.split(",")[0]) + + def index_at(lines: tuple[Line, ...], position: float) -> int: """ Index of the line playing at ``position``, or -1 before the first one. diff --git a/tests/test_errors.py b/tests/test_errors.py index d4d7c1b..e56c627 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -138,11 +138,24 @@ async def test_a_command_with_its_own_handler_is_left_alone(ctx): ctx.send.assert_not_awaited() -async def test_an_unexpected_error_is_logged_not_shown(ctx, caplog): - """Internal failures are the operator's problem, not the user's.""" +async def test_an_unexpected_error_is_acknowledged_and_logged(ctx, caplog): + """ + Staying quiet here was a mistake, and a real one: a TypeError in !lyrics was + logged and never answered, so from the channel the bot looked dead. The + details stay in the log, but the user gets told something happened. + """ await errors.handle(ctx, RuntimeError("something broke internally")) - ctx.send.assert_not_awaited() - assert "something broke internally" in caplog.text + + assert "went wrong" in sent_text(ctx).lower() + assert "something broke internally" in caplog.text, "the operator still needs it" + + +async def test_the_acknowledgement_leaks_no_internals(ctx): + """A stack trace or an exception message in the channel helps nobody.""" + await errors.handle(ctx, RuntimeError("AttributeError at /srv/secret/path.py")) + + reply = sent_text(ctx) + assert "AttributeError" not in reply and "/srv" not in reply async def test_the_original_exception_is_unwrapped(ctx): diff --git a/tests/test_lyrics_cog.py b/tests/test_lyrics_cog.py new file mode 100644 index 0000000..0607b17 --- /dev/null +++ b/tests/test_lyrics_cog.py @@ -0,0 +1,150 @@ +""" +How the command decides what to look up, and what it does with the answer. + +The gap these close is a real one: the cog's own class is called ``Lyrics`` and +it shadowed the dataclass of the same name, so the Genius fallback raised +``TypeError: Lyrics.__init__() got an unexpected keyword argument 'title'`` and +the command answered nothing at all. No test reached that path. +""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from cogs.lyrics import Lyrics as LyricsCog +from services import lyrics_api, synced_lyrics + +GENIUS_HIT = { + "title": "Every Breath You Take", + "artist": "The Police", + "lyrics": "Every breath you take", + "url": "https://genius.invalid/song", +} + + +@pytest.fixture +def cog(): + return LyricsCog(MagicMock()) + + +@pytest.fixture +def no_lrclib(monkeypatch): + monkeypatch.setattr(synced_lyrics, "fetch", AsyncMock(return_value=None)) + + +# -- the crash that produced silence ----------------------------------- + +async def test_the_genius_fallback_produces_lyrics(cog, no_lrclib, monkeypatch): + """This raised TypeError in production, and the user saw nothing at all.""" + monkeypatch.setattr(lyrics_api, "fetch", AsyncMock(return_value=GENIUS_HIT)) + + found = await cog._find("Every Breath You Take", "The Police", None) + + assert found.title == "Every Breath You Take" + assert found.artist == "The Police" + assert found.plain == "Every breath you take" + assert found.synced is False + + +async def test_nothing_anywhere_is_not_an_error(cog, no_lrclib, monkeypatch): + monkeypatch.setattr(lyrics_api, "fetch", AsyncMock(return_value=None)) + + assert await cog._find("zxqwv", "", None) is None + + +async def test_lrclib_wins_when_it_has_the_song(cog, monkeypatch): + """Genius has no timings, so it is only ever the fallback.""" + synced = synced_lyrics.Lyrics("T", "A", lines=((0.0, "line"),)) + monkeypatch.setattr(synced_lyrics, "fetch", AsyncMock(return_value=synced)) + genius = AsyncMock(return_value=GENIUS_HIT) + monkeypatch.setattr(lyrics_api, "fetch", genius) + + found = await cog._find("T", "A", None) + + assert found.synced is True + genius.assert_not_awaited() + + +# -- what gets searched for -------------------------------------------- + +async def test_a_track_is_looked_up_with_tidied_terms(cog, monkeypatch): + """ + The reported failure: the raw video title and the channel name 404 on + LRCLIB, where the tidied pair returns synced lyrics. + """ + seen = {} + + async def spy(session, title, artist, duration): + seen.update(title=title, artist=artist, duration=duration) + return None + + monkeypatch.setattr(synced_lyrics, "fetch", spy) + monkeypatch.setattr(lyrics_api, "fetch", AsyncMock(return_value=None)) + + await cog._for_track({ + "title": "The Police Every Breath You Take (Lyrics)", + "uploader": "Music n Lyrics", + "duration": 253, + }) + + assert seen == {"title": "Every Breath You Take", + "artist": "The Police", "duration": 253} + + +async def test_a_track_with_no_uploader_still_searches(cog, monkeypatch): + seen = {} + + async def spy(session, title, artist, duration): + seen.update(title=title, artist=artist) + return None + + monkeypatch.setattr(synced_lyrics, "fetch", spy) + monkeypatch.setattr(lyrics_api, "fetch", AsyncMock(return_value=None)) + + await cog._for_track({"title": "Some Song", "uploader": None, "duration": None}) + + assert seen == {"title": "Some Song", "artist": ""} + + +# -- a typed query, either way round ----------------------------------- + +async def test_a_typed_query_is_tried_both_ways_round(cog, monkeypatch): + """ + The help says `<title> - <artist>`, but `The Police - Every Breath You Take` + is the way people actually type it. Both should work. + """ + tried = [] + + async def spy(session, title, artist, duration): + tried.append((title, artist)) + return synced_lyrics.Lyrics("Every Breath You Take", "The Police", + plain="words") if artist == "The Police" else None + + monkeypatch.setattr(synced_lyrics, "fetch", spy) + monkeypatch.setattr(lyrics_api, "fetch", AsyncMock(return_value=None)) + + found = await cog._search("The Police - Every Breath You Take") + + assert found is not None, "the artist-first order found nothing" + assert tried[0] == ("The Police", "Every Breath You Take"), "documented order first" + + +async def test_the_documented_order_is_not_searched_twice(cog, monkeypatch): + """A hit on the first try must not cost a second request.""" + hit = synced_lyrics.Lyrics("Bohemian Rhapsody", "Queen", plain="words") + fetch = AsyncMock(return_value=hit) + monkeypatch.setattr(synced_lyrics, "fetch", fetch) + + await cog._search("Bohemian Rhapsody - Queen") + + assert fetch.await_count == 1 + + +async def test_a_query_without_a_separator_is_searched_once(cog, monkeypatch): + fetch = AsyncMock(return_value=None) + monkeypatch.setattr(synced_lyrics, "fetch", fetch) + monkeypatch.setattr(lyrics_api, "fetch", AsyncMock(return_value=None)) + + await cog._search("Bohemian Rhapsody") + + assert fetch.await_count == 1, "there is no other order to try" diff --git a/tests/test_search_terms.py b/tests/test_search_terms.py new file mode 100644 index 0000000..749897b --- /dev/null +++ b/tests/test_search_terms.py @@ -0,0 +1,127 @@ +""" +Turning a YouTube title into something a lyrics database will recognise. + +`!lyrics` worked for some songs and silently found nothing for others, and this +is why: the bot searched with the raw video title and the channel name. Checked +against the real LRCLIB, the difference is total — + + "Music n Lyrics" / "The Police Every Breath You Take (Lyrics)" -> HTTP 404 + "The Police" / "Every Breath You Take" -> synced + +Every sample below is the shape of a real YouTube title. +""" + +import pytest + +from services.synced_lyrics import search_terms + + +# -- the reported case ------------------------------------------------- + +def test_the_song_that_returned_nothing(): + """Reported against the live bot: no answer at all for this one.""" + assert search_terms("The Police Every Breath You Take (Lyrics)", + "Music n Lyrics") == ("Every Breath You Take", "The Police") + + +# -- splitting artist from title --------------------------------------- + +@pytest.mark.parametrize("raw,track,artist", [ + ("Queen - Bohemian Rhapsody", "Bohemian Rhapsody", "Queen"), + ("Queen – Bohemian Rhapsody", "Bohemian Rhapsody", "Queen"), # en dash + ("Queen — Bohemian Rhapsody", "Bohemian Rhapsody", "Queen"), # em dash + ("Queen Bohemian Rhapsody", "Bohemian Rhapsody", "Queen"), # padded gap +]) +def test_the_artist_comes_from_the_title_when_it_is_there(raw, track, artist): + """A channel is often a lyrics aggregator; the title is more trustworthy.""" + assert search_terms(raw, "Some Lyrics Channel") == (track, artist) + + +def test_only_the_first_separator_splits(): + """`Artist - Song - Remix` is an artist and a song, not three pieces.""" + assert search_terms("Artist - Song - Extended", "")[0] == "Song - Extended" + + +def test_the_first_of_several_credited_artists_is_used(): + """Databases file a song under its lead artist, not the whole credit list.""" + assert search_terms("KAROL G, Judeline, rusowsky - BbY WOW (Visualizer)", + "KAROL G") == ("BbY WOW", "KAROL G") + + +def test_a_title_with_no_separator_falls_back_to_the_channel(): + assert search_terms("Never Gonna Give You Up", "Rick Astley") == ( + "Never Gonna Give You Up", "Rick Astley") + + +def test_a_separator_with_nothing_on_one_side_is_not_a_separator(): + assert search_terms("- Just A Title", "Channel") == ("Just A Title", "Channel") + + +# -- stripping the noise ----------------------------------------------- + +@pytest.mark.parametrize("noise", [ + "(Official Video)", "(Official Music Video)", "[Official Video]", + "(Lyrics)", "(Lyric Video)", "(Visualizer)", "(Audio)", "(Official Audio)", + "[4K]", "(HD)", "(Remastered)", "(Remastered 2011)", "(Video Oficial)", + "(Letra)", "(Live)", "[Explicit]", "(MV)", +]) +def test_video_furniture_is_not_part_of_the_song_name(noise): + assert search_terms(f"Queen - Bohemian Rhapsody {noise}", "") == ( + "Bohemian Rhapsody", "Queen") + + +def test_a_meaningful_bracket_survives(): + """ + Not everything in brackets is furniture — some of it is the song's name. + """ + assert search_terms("Artist - Song (Acoustic Version)", "")[0] == \ + "Song (Acoustic Version)" + + +def test_a_trailing_pipe_segment_is_dropped(): + assert search_terms("Bad Bunny - Tití Me Preguntó | Un Verano Sin Ti", "") == ( + "Tití Me Preguntó", "Bad Bunny") + + +@pytest.mark.parametrize("credit", ["ft. Someone", "feat. Someone", + "Ft. Someone", "FEAT. Someone"]) +def test_featured_credits_are_dropped(credit): + """Databases file the song under the lead artist alone.""" + assert search_terms(f"Artist - Song {credit}", "")[0] == "Song" + + +def test_noise_and_credits_together(): + assert search_terms( + "Eminem - Lose Yourself ft. Someone [Official Music Video] [4K]", + "EminemMusic") == ("Lose Yourself", "Eminem") + + +# -- cleaning the channel name ----------------------------------------- + +@pytest.mark.parametrize("channel,artist", [ + ("Rick Astley - Topic", "Rick Astley"), # auto-generated music channels + ("QueenVEVO", "Queen"), + ("Queen VEVO", "Queen"), +]) +def test_channel_suffixes_are_not_part_of_the_artist(channel, artist): + assert search_terms("Some Song", channel)[1] == artist + + +def test_a_channel_that_is_only_a_suffix_leaves_no_artist(): + assert search_terms("Some Song", "- Topic")[1] == "" + + +# -- degenerate input -------------------------------------------------- + +def test_nothing_in_nothing_out(): + assert search_terms("", "") == ("", "") + + +def test_a_title_that_is_all_noise_keeps_something_to_search_for(): + """Stripping everything would search for an empty string.""" + track, _ = search_terms("(Official Video)", "Channel") + assert track == "(Official Video)" + + +def test_whitespace_is_collapsed(): + assert search_terms(" Artist - Song ", "") == ("Song", "Artist") diff --git a/utils/errors.py b/utils/errors.py index b9755f7..abcbf6f 100644 --- a/utils/errors.py +++ b/utils/errors.py @@ -69,9 +69,12 @@ async def handle(ctx, error: Exception) -> None: hint = f"\nUsage: `{usage(ctx)}`" if ctx.command is not None else "" return await _reply(ctx, f"{_input_detail(error)}{hint}") - # Anything left is a bug or an outage — the operator's problem, not the - # user's. Log it with a traceback and stay quiet in the channel. + # Anything left is a bug or an outage. The cause is the operator's problem + # and stays in the log — a stack trace in the channel helps nobody — but + # saying nothing at all is worse. A TypeError in !lyrics was logged and + # never answered, and from the channel the bot simply looked dead. log.warning("Error in command %s: %s", ctx.command, error, exc_info=error) + await _reply(ctx, "Something went wrong on my side. It has been logged.") async def _reply(ctx, message: str) -> None: