diff --git a/core/musicbrainz_client.py b/core/musicbrainz_client.py index a802373d..f8c4a178 100644 --- a/core/musicbrainz_client.py +++ b/core/musicbrainz_client.py @@ -7,6 +7,19 @@ from utils.logging_config import get_logger logger = get_logger("musicbrainz_client") +# Lucene query-syntax characters that must be backslash-escaped when a +# user-supplied value is interpolated into a query (e.g. artist names like +# "Sunn O)))", "Anthony Green (Saosin)", "Therapy?", "!!!"). Without this an +# unbalanced paren or a stray ?/* either breaks the field group (returning +# unrelated results) or yields zero hits. +_LUCENE_SPECIAL = set('+-&|!(){}[]^"~*?:\\/') + + +def _escape_lucene(text: str) -> str: + """Backslash-escape Lucene special characters in a user-supplied term.""" + return ''.join('\\' + ch if ch in _LUCENE_SPECIAL else ch for ch in text) + + # Global rate limiting variables _last_api_call_time = 0 _api_call_lock = threading.Lock() @@ -158,14 +171,14 @@ class MusicBrainzClient: safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') query += f' AND artist:"{safe_artist}"' else: - # Bare query — MB tokenizes against title + artist credit + - # alias + sortname indexes together with diacritic folding. - # Recovers cases like "Bjork" → "Björk" that strict phrase - # queries miss. - parts = [album_name] - if artist_name: - parts.append(artist_name) - query = ' '.join(p for p in parts if p) + # Loose title terms (no phrase quotes → diacritic folding and + # alias/sortname recall, e.g. "Bjork" → "Björk"), but + # FIELD-SCOPE the artist so it constrains rather than floating + # as a free fuzzy term that lets unrelated releases whose titles + # echo the artist name rank first (same root cause as #754). + query = album_name + if artist_name and artist_name.strip(): + query += f' AND artist:({_escape_lucene(artist_name.strip())})' params = { 'query': query, @@ -223,18 +236,24 @@ class MusicBrainzClient: safe_artist = artist_name.replace('\\', '\\\\').replace('"', '\\"') query += f' AND artist:"{safe_artist}"' else: - # Bare query — see search_release for rationale. - parts = [track_name] - if artist_name: - parts.append(artist_name) - query = ' '.join(p for p in parts if p) + # Loose track terms (no phrase quotes → tolerant of bracketed + # suffixes and diacritics, hitting MB's alias/sortname indexes), + # but FIELD-SCOPE the artist so it actually constrains results. + # A bare "track artist" blob let the artist float as a free + # fuzzy term, so covers/karaoke whose TITLES contain the artist + # name outranked the real recording (#754: "Sweet Child O Mine" + # / "Guns N Roses" returned only covers). Scoping still folds + # diacritics — artist:(Bjork) matches "Björk". + query = track_name + if artist_name and artist_name.strip(): + query += f' AND artist:({_escape_lucene(artist_name.strip())})' params = { 'query': query, 'fmt': 'json', 'limit': limit } - + response = self.session.get( f"{self.BASE_URL}/recording", params=params, diff --git a/tests/metadata/test_musicbrainz_client_query.py b/tests/metadata/test_musicbrainz_client_query.py new file mode 100644 index 00000000..d3e2a806 --- /dev/null +++ b/tests/metadata/test_musicbrainz_client_query.py @@ -0,0 +1,122 @@ +"""Query-construction tests for core/musicbrainz_client.py. + +Regression guard for #754: the user-facing "Search for Match" / Fix popup +runs non-strict (strict=False) MusicBrainz searches. The old non-strict path +built a bare "track artist" blob with NO field scoping, so the artist was just +a free fuzzy term — covers/karaoke whose TITLES contained the artist name +outranked the real recording (e.g. "Sweet Child O Mine" / "Guns N Roses" +returned only covers, never the Guns N' Roses original). + +The fix keeps the track/album side loose (diacritic + bracket recall) but +field-scopes the artist (artist:(...)) so it actually constrains. These tests +pin the query STRING the client sends — no network — by capturing the params +passed to session.get. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from core.musicbrainz_client import MusicBrainzClient, _escape_lucene + + +@pytest.fixture +def client(): + c = MusicBrainzClient("SoulSync", "2") + # Replace the HTTP session with a mock returning an empty result set, so + # we can inspect the query string without touching the network. + resp = MagicMock() + resp.raise_for_status = MagicMock() + resp.json = MagicMock(return_value={"recordings": [], "releases": []}) + c.session = MagicMock() + c.session.get = MagicMock(return_value=resp) + return c + + +def _query_of(client) -> str: + """The 'query' param of the last session.get call.""" + _, kwargs = client.session.get.call_args + return kwargs["params"]["query"] + + +# ── recording: non-strict must field-scope the artist ────────────────────── + +def test_recording_nonstrict_scopes_artist(client): + client.search_recording("Sweet Child O Mine", artist_name="Guns N Roses", strict=False) + q = _query_of(client) + assert "artist:(Guns N Roses)" in q # artist is a CONSTRAINT, not a loose term + assert q.startswith("Sweet Child O Mine") # track side stays loose (no phrase quotes) + assert '"' not in q # no phrase-quoting that kills bracket/diacritic recall + + +def test_recording_nonstrict_without_artist_is_bare_track(client): + client.search_recording("Hyperballad", strict=False) + assert _query_of(client) == "Hyperballad" # no artist → no AND clause + + +def test_recording_nonstrict_whitespace_artist_is_bare_track(client): + # A whitespace-only artist must not produce a malformed artist:( ) group. + client.search_recording("Hyperballad", artist_name=" ", strict=False) + assert _query_of(client) == "Hyperballad" + + +def test_recording_nonstrict_strips_artist_padding(client): + client.search_recording("Money", artist_name=" Pink Floyd ", strict=False) + assert _query_of(client) == "Money AND artist:(Pink Floyd)" + + +def test_recording_strict_unchanged(client): + client.search_recording("Say You Will", artist_name="Foreigner", strict=True) + q = _query_of(client) + assert q == 'recording:"Say You Will" AND artist:"Foreigner"' # strict path untouched + + +def test_recording_nonstrict_escapes_lucene_specials_in_artist(client): + # Artist names with parens/?/! must NOT break the artist:(...) group. + # Without escaping, "Sunn O)))" closes the group early and returns + # unrelated artists; "Anthony Green (Saosin)" returns nothing. + client.search_recording("Hunting Season", artist_name="Sunn O)))", strict=False) + q = _query_of(client) + assert "artist:(Sunn O\\)\\)\\))" in q # every ) escaped, group stays balanced + # The clause is well-formed: opening "artist:(" is closed by exactly one + # unescaped ")", so paren depth returns to zero. + depth = 0 + after = q.split("artist:(", 1)[1] + i = 0 + while i < len(after): + ch = after[i] + if ch == "\\": + i += 2 + continue + if ch == "(": + depth += 1 + elif ch == ")": + if depth == 0: + break # this is the closing paren of artist:( + depth -= 1 + i += 1 + assert depth == 0 + + +def test_escape_lucene_helper(): + assert _escape_lucene("Sunn O)))") == "Sunn O\\)\\)\\)" + assert _escape_lucene("Therapy?") == "Therapy\\?" + assert _escape_lucene("AC/DC") == "AC\\/DC" + assert _escape_lucene("Foreigner") == "Foreigner" # plain text untouched + + +# ── release: same fix, same guarantees ───────────────────────────────────── + +def test_release_nonstrict_scopes_artist(client): + client.search_release("Nevermind", artist_name="Nirvana", strict=False) + q = _query_of(client) + assert "artist:(Nirvana)" in q + assert q.startswith("Nevermind") + assert '"' not in q + + +def test_release_strict_unchanged(client): + client.search_release("Nevermind", artist_name="Nirvana", strict=True) + assert _query_of(client) == 'release:"Nevermind" AND artist:"Nirvana"'