Fix Discogs internal ID tag leaking into "View on Discogs" links - #1204
Open
RiceTeaPrince wants to merge 1 commit into
Open
Fix Discogs internal ID tag leaking into "View on Discogs" links#1204RiceTeaPrince wants to merge 1 commit into
RiceTeaPrince wants to merge 1 commit into
Conversation
… on Discogs" links Two independent leaks, both from the m/r type-tag _tag_discogs_album_id() adds (introduced in Nezreka#848): the artist-detail discography card builder never passed through the already-computed external_urls, and get_artist_full_detail read the tagged discogs_id DB column back verbatim for the frontend badge. Both fixed additively; the tag itself and _discogs_album_endpoints() routing are untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1203
Problem
The
m/rtype-tag_tag_discogs_album_id()adds to a Discogs album ID(introduced in #848 to fix the master/release collision in #847) is meant
to stay an internal routing detail for
_discogs_album_endpoints(). Itnever gets stripped back off before reaching a public-facing field, and
leaks out through two independent paths, both producing the same symptom —
clicking "View on Discogs" for an album opens a 404:
https://www.discogs.com/release/r5743831https://www.discogs.com/release/5743831(bare numeric IDredirects to Discogs' own canonical slugged URL — no need to construct
the slug ourselves)
Leak site 1 —
_build_artist_detail_release_card()(
core/metadata/discography.py), which builds cards for the artist-detaildiscography grid, drops
external_urlsentirely from its output, eventhough
Album.from_discogs_release()already computes the correct sluggedURL at parse time (
core/discogs_client.py:346-348). The card is left withonly the tagged
id.Leak site 2 —
core/discogs_worker.py's_update_album(), thebackground auto-match worker, persists the tagged ID straight into the
albums.discogs_idDB column. That column is read back verbatim (noun-tagging) by
get_artist_full_detail()(
database/music_database.py:15286,album_data = dict(album_row)) andhanded to the frontend, where
getServiceUrl()(
webui/src/routes/artist-detail/-artist-detail.enhanced-album.ts:56-59)does a plain string interpolation with no idea the tag exists. This is the
one that actually fires for an already-matched library album's "View on
Discogs" badge, not just an unowned discography-grid card.
Fix
Two independent, additive changes — neither touches the tagging scheme
itself or the DB's stored value, so
_discogs_album_endpoints()'s routingis unaffected:
core/metadata/discography.py—_build_artist_detail_release_card()now passes through
external_urls(both the typed-converter and legacyduck-typed branches) instead of dropping it.
core/discogs_client.py— new_untag_discogs_album_id()helper(inverse of the existing
_tag_discogs_album_id()), display-only.database/music_database.py—get_artist_full_detail()strips the tagoff
album_data['discogs_id']via the new helper right before the albumdict is returned, so the API response is always a clean numeric ID.
_discogs_album_endpoints()already has a documented "legacy untagged ID"fallback (release-first, then master) built specifically to self-heal
pre-#848 bad matches, so an untagged ID reaching it by some other path
still resolves correctly rather than failing outright.
Tests
tests/test_discogs_id_typing.py(+6, alongside the existing coveragefrom Discogs: fix master/release ID collision fetching the wrong album #848) —
_untag_discogs_album_id(): round-trips with the tagger,release/master stripping, empty/
None, an already-untagged legacy ID leftinert, a non-Discogs ID string left untouched if the helper is ever
misapplied, and a check that an untagged ID still resolves correctly via
_discogs_album_endpoints()'s existing legacy fallback if it's everre-fed into it.
tests/metadata/test_discography_typed_path.py(+2, +1 extendedassertion) —
_build_artist_detail_release_card()'sexternal_urlspassthrough for both the typed-converter and legacy duck-typed branches
(the latter using this issue's real reported album), plus a
no-
external_urlscase defaulting to{}rather than crashing.tests/test_artist_full_detail_discogs_id_untag.py(new file, +4) — anisolated in-memory-sqlite regression test, same pattern as
tests/test_artist_full_detail_source_id.py: release-tagged,master-tagged, missing, and legacy-untagged
discogs_idvalues all comeback correctly from
get_artist_full_detail().Verification
Both fixes' logic (not just the diagnosis) checked against a real
Discogs API response for the originally-reported release
(
GET /releases/5743831, Naoya Matsuoka — Watermelon Dandies):urifield, run through theexternal_urlsfix,produces exactly
https://www.discogs.com/release/5743831-Naoya-Matsuoka-Watermelon-Dandies.exact reported broken ID (
r5743831); run through_untag_discogs_album_id(), correctly strips to5743831._discogs_album_endpoints('5743831')(untagged) still resolves to['/releases/5743831', '/masters/5743831']— release-first, matching theexisting self-heal fallback — confirming the untag can't break routing
even if re-fed into it.
Ran this repo's actual CI gates locally (Python 3.11.15 via
uv,requirements-dev.txtpinned versions,webui/deps vianpm cisincetests/test_vanilla_globals_resolve.pyneeds a real pinnedoxlint):ruff check .— cleancompileall api core database services scripts web_server.py wsgi.py beatport_unified_scraper.py— cleanpytest— 15159 passed, 2 skipped, 7 deselected, 0 failed (15147 baseline + this PR's 12 new tests)Not independently confirmed in a real browser from this environment —
curlagainst all three URL variants (tagged, bare numeric, full slug) hitDiscogs' Cloudflare managed-challenge page identically regardless of which
one is correct, so it's not a usable signal. The one real-browser
confirmation is the reporter's own manual test (removing the leading
rwas sufficient to make the link resolve), consistent with the API/code-level
checks above.