#775: links only — reject bare IDs (ambiguous), add not-found hint
Follow-up to the bare-ID footgun: a bare number like 525046 carries no source and no entity type, so it resolved to whatever album happened to own that id (a user pasting Kendrick's Deezer artist id got an unrelated album). Now the resolver accepts provider URLs (and the explicit spotify: URI) only; a bare/unrecognized string is rejected and the dropdown surfaces a hint to paste a full link. URL parsing + album/track resolution are unchanged.
This commit is contained in:
BoulderBadgeDad
parent
9772d5313c
commit
e05979ea07
5 changed files with 87 additions and 136 deletions
|
|
@ -6,13 +6,14 @@ entity up *directly* on the owning source — no scoring, no guessing.
|
|||
|
||||
Design notes
|
||||
------------
|
||||
- **Source-agnostic input.** A full URL carries its source in the domain
|
||||
- **Links only.** A full URL carries its source in the domain
|
||||
(``open.spotify.com`` → spotify, ``musicbrainz.org`` → musicbrainz, …)
|
||||
and its kind in the path (``/album/`` vs ``/track/``), so it resolves to
|
||||
exactly one lookup. A *bare* ID is disambiguated by format where we can
|
||||
(UUID → MusicBrainz, 22-char base62 → Spotify) and otherwise fanned out
|
||||
across the numeric-ID sources (Deezer, iTunes); the first source that
|
||||
returns a hit wins. Paste the full link to remove that ambiguity.
|
||||
exactly one unambiguous lookup. The ``spotify:album:ID`` URI is accepted
|
||||
too since it's equally explicit. Bare IDs are intentionally rejected: a
|
||||
bare number like ``525046`` carries no source and no entity type, so it
|
||||
would resolve to whatever album/track happens to own that id on some
|
||||
source — often an unrelated entity. Paste the link instead.
|
||||
|
||||
- **Reuses existing per-source get-by-id.** Spotify/iTunes/MusicBrainz all
|
||||
expose ``get_album``; Deezer exposes ``get_album_metadata``; all four
|
||||
|
|
@ -32,7 +33,6 @@ an injected ``client_resolver`` (defaulting to the orchestrator's
|
|||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
import re
|
||||
from typing import Any, Callable, NamedTuple, Optional
|
||||
from urllib.parse import parse_qs, urlparse
|
||||
|
||||
|
|
@ -44,15 +44,6 @@ logger = logging.getLogger(__name__)
|
|||
# return raw API shapes and aren't metadata-link sources, so they're omitted.
|
||||
SUPPORTED_SOURCES = ('spotify', 'itunes', 'musicbrainz', 'deezer')
|
||||
|
||||
_UUID_RE = re.compile(
|
||||
r'^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$', re.I
|
||||
)
|
||||
_SPOTIFY_ID_RE = re.compile(r'^[0-9A-Za-z]{22}$')
|
||||
_NUMERIC_RE = re.compile(r'^\d+$')
|
||||
|
||||
# Numeric-ID sources, tried in this order when a bare number is pasted.
|
||||
_NUMERIC_SOURCES = ('deezer', 'itunes')
|
||||
|
||||
# Domains we recognize — used to detect a pasted URL even when the user
|
||||
# omitted the scheme (e.g. "open.spotify.com/album/…").
|
||||
_KNOWN_HOSTS = (
|
||||
|
|
@ -64,9 +55,10 @@ _KNOWN_HOSTS = (
|
|||
class LookupTarget(NamedTuple):
|
||||
"""One (source, kind, id) lookup to attempt.
|
||||
|
||||
``kind`` is ``'album'`` or ``'track'`` when the input pins it (a URL path
|
||||
or URI type), or ``None`` when unknown — the resolver then tries album
|
||||
first, then track.
|
||||
``kind`` is ``'album'`` or ``'track'`` — always pinned by the URL path or
|
||||
URI type (links-only input). The ``Optional`` typing is kept defensively:
|
||||
the resolver falls back to album-then-track if a future parser path ever
|
||||
yields ``None``.
|
||||
"""
|
||||
|
||||
source: str
|
||||
|
|
@ -139,14 +131,12 @@ def _parse_url(raw: str) -> list[LookupTarget]:
|
|||
return []
|
||||
|
||||
|
||||
def parse_metadata_identifier(
|
||||
raw: str, preferred_source: Optional[str] = None
|
||||
) -> list[LookupTarget]:
|
||||
"""Parse a pasted link/ID into an ordered list of lookup targets.
|
||||
def parse_metadata_identifier(raw: str) -> list[LookupTarget]:
|
||||
"""Parse a pasted provider link (or ``spotify:`` URI) into lookup targets.
|
||||
|
||||
Returns the candidates to try in order; the first that resolves wins.
|
||||
``preferred_source`` (the user's active source) only reorders the
|
||||
fan-out for ambiguous *bare numeric* IDs.
|
||||
Links only — a bare ID has no source/type and is rejected (returns ``[]``).
|
||||
A URL resolves to exactly one target; the list type is kept for the
|
||||
``spotify:`` URI path and future multi-target patterns.
|
||||
"""
|
||||
raw = (raw or '').strip()
|
||||
if not raw:
|
||||
|
|
@ -164,34 +154,9 @@ def parse_metadata_identifier(
|
|||
)
|
||||
if looks_like_url:
|
||||
url = raw if '://' in raw else f'https://{raw}'
|
||||
targets = _parse_url(url)
|
||||
if targets:
|
||||
return targets
|
||||
# Fall through: maybe a bare id slipped in with a stray scheme.
|
||||
|
||||
# Bare identifier — disambiguate by format.
|
||||
if _UUID_RE.match(raw):
|
||||
# MusicBrainz MBID: could be release / release-group (album) or
|
||||
# recording (track). Try album first, then track.
|
||||
return [LookupTarget('musicbrainz', 'album', raw),
|
||||
LookupTarget('musicbrainz', 'track', raw)]
|
||||
|
||||
if _NUMERIC_RE.match(raw):
|
||||
# Numeric IDs collide across Deezer/iTunes — fan out, first hit wins.
|
||||
order = list(_NUMERIC_SOURCES)
|
||||
if preferred_source in order:
|
||||
order.remove(preferred_source)
|
||||
order.insert(0, preferred_source)
|
||||
targets: list[LookupTarget] = []
|
||||
for src in order:
|
||||
targets.append(LookupTarget(src, 'album', raw))
|
||||
targets.append(LookupTarget(src, 'track', raw))
|
||||
return targets
|
||||
|
||||
if _SPOTIFY_ID_RE.match(raw):
|
||||
return [LookupTarget('spotify', 'album', raw),
|
||||
LookupTarget('spotify', 'track', raw)]
|
||||
return _parse_url(url)
|
||||
|
||||
# Bare ID (or anything we don't recognize as a link) — rejected.
|
||||
return []
|
||||
|
||||
|
||||
|
|
@ -286,28 +251,37 @@ def _fetch_track(client: Any, source: str, identifier: str) -> Optional[dict]:
|
|||
return client.get_track_details(identifier)
|
||||
|
||||
|
||||
def _empty_result(raw: str, source: str = '') -> dict:
|
||||
# Shown in the dropdown's empty state so the user knows what to do next.
|
||||
_MSG_NOT_A_LINK = (
|
||||
'Paste a full link from Spotify, Apple Music, MusicBrainz, or Deezer '
|
||||
'(a bare ID is ambiguous).'
|
||||
)
|
||||
_MSG_NOT_FOUND = "Couldn't resolve that link — double-check it's correct."
|
||||
|
||||
|
||||
def _empty_result(raw: str, source: str = '', message: str = '') -> dict:
|
||||
return {
|
||||
'source': source,
|
||||
'albums': [],
|
||||
'tracks': [],
|
||||
'available': False,
|
||||
'query': raw,
|
||||
'message': message,
|
||||
}
|
||||
|
||||
|
||||
def resolve_identifier(
|
||||
raw: str,
|
||||
deps: Any,
|
||||
preferred_source: Optional[str] = None,
|
||||
client_resolver: Optional[Callable[[str], Any]] = None,
|
||||
) -> dict:
|
||||
"""Resolve a pasted link/ID to a single album or track card.
|
||||
"""Resolve a pasted provider link to a single album or track card.
|
||||
|
||||
Returns a dropdown-compatible dict:
|
||||
``{source, albums, tracks, available, query}``. ``available`` is True iff
|
||||
a source returned a hit. The first resolving target wins, so the result
|
||||
carries exactly one card (and the ``source`` that owns it).
|
||||
``{source, albums, tracks, available, query, message}``. ``available`` is
|
||||
True iff a source returned a hit; the first resolving target wins, so the
|
||||
result carries exactly one card (and the ``source`` that owns it).
|
||||
``message`` is a user-facing hint when nothing resolved.
|
||||
|
||||
``client_resolver`` maps a source name to a client (or None). It defaults
|
||||
to the orchestrator's ``resolve_client``; tests inject fakes.
|
||||
|
|
@ -318,10 +292,10 @@ def resolve_identifier(
|
|||
def client_resolver(source: str) -> Any: # noqa: E306
|
||||
return resolve_client(source, deps)[0]
|
||||
|
||||
targets = parse_metadata_identifier(raw, preferred_source=preferred_source)
|
||||
targets = parse_metadata_identifier(raw)
|
||||
if not targets:
|
||||
logger.info(f"Link/ID resolve: unrecognized identifier {raw!r}")
|
||||
return _empty_result(raw, preferred_source or '')
|
||||
logger.info(f"Link/ID resolve: not a recognized link {raw!r}")
|
||||
return _empty_result(raw, message=_MSG_NOT_A_LINK)
|
||||
|
||||
for target in targets:
|
||||
try:
|
||||
|
|
@ -361,4 +335,4 @@ def resolve_identifier(
|
|||
)
|
||||
|
||||
logger.info(f"Link/ID resolve: no source resolved {raw!r}")
|
||||
return _empty_result(raw, preferred_source or '')
|
||||
return _empty_result(raw, source=targets[0].source, message=_MSG_NOT_FOUND)
|
||||
|
|
|
|||
|
|
@ -135,42 +135,23 @@ def test_parse_spotify_url_without_scheme_or_www():
|
|||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# parse_metadata_identifier — bare IDs
|
||||
# parse_metadata_identifier — bare IDs are rejected (links only)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_parse_bare_uuid_is_musicbrainz_album_then_track():
|
||||
mbid = 'a1b2c3d4-e5f6-7890-abcd-ef1234567890'
|
||||
out = by_id.parse_metadata_identifier(mbid)
|
||||
assert out == [
|
||||
LookupTarget('musicbrainz', 'album', mbid),
|
||||
LookupTarget('musicbrainz', 'track', mbid),
|
||||
]
|
||||
def test_parse_bare_numeric_id_rejected():
|
||||
# The footgun case (#775 follow-up): a bare number has no source/type, so
|
||||
# it must NOT resolve to whatever album happens to own that id.
|
||||
assert by_id.parse_metadata_identifier('525046') == []
|
||||
|
||||
|
||||
def test_parse_bare_base62_is_spotify_album_then_track():
|
||||
sid = '4aawyAB9vmqN3uQ7FjRGTy' # 22 chars, has letters
|
||||
out = by_id.parse_metadata_identifier(sid)
|
||||
assert out == [
|
||||
LookupTarget('spotify', 'album', sid),
|
||||
LookupTarget('spotify', 'track', sid),
|
||||
]
|
||||
def test_parse_bare_uuid_rejected():
|
||||
assert by_id.parse_metadata_identifier(
|
||||
'a1b2c3d4-e5f6-7890-abcd-ef1234567890'
|
||||
) == []
|
||||
|
||||
|
||||
def test_parse_bare_numeric_fans_out_deezer_then_itunes():
|
||||
out = by_id.parse_metadata_identifier('302127')
|
||||
assert out == [
|
||||
LookupTarget('deezer', 'album', '302127'),
|
||||
LookupTarget('deezer', 'track', '302127'),
|
||||
LookupTarget('itunes', 'album', '302127'),
|
||||
LookupTarget('itunes', 'track', '302127'),
|
||||
]
|
||||
|
||||
|
||||
def test_parse_bare_numeric_biases_preferred_source_first():
|
||||
out = by_id.parse_metadata_identifier('302127', preferred_source='itunes')
|
||||
# iTunes pulled to the front of the fan-out.
|
||||
assert out[0] == LookupTarget('itunes', 'album', '302127')
|
||||
assert out[1] == LookupTarget('itunes', 'track', '302127')
|
||||
def test_parse_bare_base62_rejected():
|
||||
assert by_id.parse_metadata_identifier('4aawyAB9vmqN3uQ7FjRGTy') == []
|
||||
|
||||
|
||||
def test_parse_empty_and_garbage_return_empty():
|
||||
|
|
@ -301,40 +282,35 @@ def test_resolve_deezer_album_uses_get_album_metadata():
|
|||
assert client.album_calls == ['302127']
|
||||
|
||||
|
||||
def test_resolve_kind_unknown_falls_back_album_then_track():
|
||||
# Bare base62 → spotify, kind unknown. Album returns None, track resolves.
|
||||
def test_resolve_track_link():
|
||||
# A track URL pins kind=track — only get_track_details is called.
|
||||
client = _FakeClient(album=None, track={
|
||||
'id': 't1', 'name': 'Creep', 'artists': ['Radiohead'],
|
||||
'album': {'name': 'Pablo Honey'}, 'duration_ms': 238000,
|
||||
})
|
||||
sid = '4aawyAB9vmqN3uQ7FjRGTy'
|
||||
res = by_id.resolve_identifier(
|
||||
sid, deps=None, client_resolver=_resolver_from({'spotify': client}),
|
||||
'https://open.spotify.com/track/t1', deps=None,
|
||||
client_resolver=_resolver_from({'spotify': client}),
|
||||
)
|
||||
assert res['available'] is True
|
||||
assert res['tracks'][0]['name'] == 'Creep'
|
||||
assert res['albums'] == []
|
||||
assert client.album_calls == [sid] # tried album first
|
||||
assert client.track_calls == [sid] # then track
|
||||
assert client.album_calls == [] # kind pinned to track — no album probe
|
||||
assert client.track_calls == ['t1']
|
||||
|
||||
|
||||
def test_resolve_numeric_fanout_first_hit_wins():
|
||||
# Deezer has no such id (returns None), iTunes does — resolver moves on.
|
||||
deezer = _FakeClient(album=None, track=None)
|
||||
itunes = _FakeClient(album={'id': '99', 'name': 'Thriller',
|
||||
'artists': ['Michael Jackson'], 'images': [],
|
||||
'release_date': '1982', 'total_tracks': 9,
|
||||
'album_type': 'album', 'external_urls': {}})
|
||||
def test_resolve_bare_id_rejected_with_hint():
|
||||
# The #775 follow-up regression: a bare number must not resolve; it
|
||||
# returns not-found with a link hint instead of an unrelated album.
|
||||
called = []
|
||||
res = by_id.resolve_identifier(
|
||||
'99', deps=None,
|
||||
client_resolver=_resolver_from({'deezer': deezer, 'itunes': itunes}),
|
||||
'525046', deps=None,
|
||||
client_resolver=lambda s: called.append(s), # must never be invoked
|
||||
)
|
||||
assert res['available'] is True
|
||||
assert res['source'] == 'itunes'
|
||||
assert res['albums'][0]['name'] == 'Thriller'
|
||||
# Deezer was tried (album + track) before iTunes resolved.
|
||||
assert deezer.album_calls == ['99']
|
||||
assert deezer.track_calls == ['99']
|
||||
assert res['available'] is False
|
||||
assert res['albums'] == [] and res['tracks'] == []
|
||||
assert 'link' in res['message'].lower()
|
||||
assert called == [] # no source was even probed
|
||||
|
||||
|
||||
def test_resolve_unavailable_client_is_skipped():
|
||||
|
|
@ -345,7 +321,9 @@ def test_resolve_unavailable_client_is_skipped():
|
|||
)
|
||||
assert res['available'] is False
|
||||
assert res['albums'] == [] and res['tracks'] == []
|
||||
assert res['source'] == ''
|
||||
# The source we tried is reported even on miss.
|
||||
assert res['source'] == 'spotify'
|
||||
assert res['message']
|
||||
|
||||
|
||||
def test_resolve_client_exception_does_not_propagate():
|
||||
|
|
|
|||
|
|
@ -5728,27 +5728,26 @@ def enhanced_search():
|
|||
|
||||
@app.route('/api/enhanced-search/by-id', methods=['POST'])
|
||||
def enhanced_search_by_id():
|
||||
"""Resolve a pasted metadata link/ID to a single album or track (#775).
|
||||
"""Resolve a pasted metadata link to a single album or track (#775).
|
||||
|
||||
Source-agnostic: a provider URL (Spotify/Apple/MusicBrainz/Deezer) or a
|
||||
bare ID is looked up directly on the owning source via its get-by-id —
|
||||
no fuzzy search, no scoring. Returns the same dropdown shape the normal
|
||||
enhanced search renders, plus the resolving ``source`` so the frontend
|
||||
can route downloads/imports through the existing flow.
|
||||
A provider URL (Spotify/Apple/MusicBrainz/Deezer) is looked up directly
|
||||
on the owning source via its get-by-id — no fuzzy search, no scoring. The
|
||||
domain pins the source and the path pins album-vs-track, so it's
|
||||
unambiguous. Returns the same dropdown shape the normal enhanced search
|
||||
renders, plus the resolving ``source`` so the frontend can route
|
||||
downloads/imports through the existing flow.
|
||||
|
||||
Body: ``{"query": "<link or id>", "source": "<active source>"?}``. The
|
||||
optional ``source`` only biases the fan-out for ambiguous bare numeric
|
||||
IDs (Deezer vs iTunes).
|
||||
Body: ``{"query": "<provider link or spotify: URI>"}``. Links only — a
|
||||
bare ID is rejected with a hint, since it carries no source or type.
|
||||
"""
|
||||
data = request.get_json() or {}
|
||||
raw = (data.get('query') or '').strip()
|
||||
preferred = (data.get('source') or '').strip().lower() or None
|
||||
if not raw:
|
||||
return jsonify(_search_by_id._empty_result(''))
|
||||
|
||||
try:
|
||||
deps = _build_search_deps()
|
||||
result = _search_by_id.resolve_identifier(raw, deps, preferred_source=preferred)
|
||||
result = _search_by_id.resolve_identifier(raw, deps)
|
||||
return jsonify(result)
|
||||
except Exception as e:
|
||||
logger.error(f"Link/ID resolve error: {e}")
|
||||
|
|
|
|||
|
|
@ -2201,15 +2201,15 @@
|
|||
</div>
|
||||
</div>
|
||||
|
||||
<!-- Link / ID lookup (#775): paste a provider link or
|
||||
bare ID and resolve it directly on the owning source
|
||||
(Spotify / Apple Music / MusicBrainz / Deezer) — no
|
||||
fuzzy search. Results render in the dropdown below and
|
||||
reuse the normal download/import flow. -->
|
||||
<!-- Link lookup (#775): paste a provider link and resolve
|
||||
it directly on the owning source (Spotify / Apple Music
|
||||
/ MusicBrainz / Deezer) — no fuzzy search. Results render
|
||||
in the dropdown below and reuse the normal download/
|
||||
import flow. Links only: a bare ID is ambiguous. -->
|
||||
<div class="enh-id-lookup">
|
||||
<span class="enh-id-lookup-icon">🔗</span>
|
||||
<input type="text" id="enh-id-input" class="enh-id-input"
|
||||
placeholder="…or paste a Spotify / Apple Music / MusicBrainz / Deezer link or ID"
|
||||
placeholder="…or paste a Spotify / Apple Music / MusicBrainz / Deezer link"
|
||||
autocomplete="off" spellcheck="false">
|
||||
<button id="enh-id-btn" class="enh-id-btn" type="button">Look up</button>
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -300,10 +300,7 @@ function initializeSearchModeToggle() {
|
|||
const resp = await fetch('/api/enhanced-search/by-id', {
|
||||
method: 'POST',
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
body: JSON.stringify({
|
||||
query: raw,
|
||||
source: searchController.state.activeSource,
|
||||
}),
|
||||
body: JSON.stringify({ query: raw }),
|
||||
});
|
||||
data = await resp.json();
|
||||
} catch (err) {
|
||||
|
|
@ -329,11 +326,14 @@ function initializeSearchModeToggle() {
|
|||
}
|
||||
_renderFromState(searchController.state);
|
||||
} else {
|
||||
// No source resolved the link — show the empty state.
|
||||
// Not a link, or nothing resolved — surface the backend's hint
|
||||
// via a toast (non-destructive) and show the empty state.
|
||||
if (loadingState) loadingState.classList.add('hidden');
|
||||
if (resultsContainer) resultsContainer.classList.add('hidden');
|
||||
if (emptyState) emptyState.classList.remove('hidden');
|
||||
showDropdown();
|
||||
const msg = (data && data.message) || 'No match for that link.';
|
||||
if (typeof showToast === 'function') showToast(msg, 'warning');
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue