Skip to content

Keep same-provider artists with different ids apart - #6324

Draft
jozefKruszynski wants to merge 4 commits into
devfrom
jozef/artist-fix
Draft

jozefKruszynski wants to merge 4 commits into
devfrom
jozef/artist-fix

Conversation

@jozefKruszynski

@jozefKruszynski jozefKruszynski commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

What does this implement/fix?

Two artists that one streaming provider instance knows under different ids were merged into a single library artist whenever their names matched case-insensitively (for example the Tidal artists "loud" and "LOUD"). The artists controller now treats such a candidate as a distinct artist unless a MusicBrainz, Discogs or TADB id says otherwise, and applies the same rule when deduplicating artist search results. Providers whose ids are not stable, such as the filesystem providers, are exempt. Artists already merged in existing libraries are not split by this change; the follow-up PR #6327 adds a repair pass for those.

Related issue (if applicable):

  • related issue: none

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • New music/player/metadata/plugin provider — new-provider
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — documentation
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Checklist

  • The code change is tested and works locally.
  • pre-commit run --all-files passes.
  • pytest passes, and tests have been added/updated under tests/ where applicable.
  • For changes to shared models, the companion PR in music-assistant/models is linked.
  • For changes affecting the UI, the companion PR in music-assistant/frontend is linked.
  • I have read and complied with the project's AI Policy for any AI-assisted contributions.
  • I have raised a PR against the documentation repository targeting the main or beta branch as appropriate.

Two artists that one streaming provider instance knows under different
ids were merged into a single library artist on a case-insensitive name
match. The artists controller now rejects such a candidate unless a
strong external id says they are the same, and applies the same rule to
search-result deduplication. Providers whose ids are not stable, like
the filesystem providers, are exempt.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 13, 2026 18:41
@musicassistant-bot

musicassistant-bot Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

✅ The title and description are good to go. Thanks!

@codspeed

codspeed Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 11 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing jozef/artist-fix (adf5dbc) with dev (4d6081a)2

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on dev (5e92422) during the generation of this report, so 4d6081a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Similar-artist deduplication incorrectly ignores the documented external-ID override.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents same-name artists with conflicting IDs from being merged.

Changes:

  • Adds provider-ID conflict detection.
  • Reuses strong artist external-ID types.
  • Adds focused matching tests.
File summaries
File Description
artists.py Adds conflict-aware matching and deduplication.
compare.py Exposes artist external-ID types.
test_artist_provider_id_conflict.py Tests provider-ID matching behavior.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread music_assistant/controllers/music/media/artists.py Outdated
@github-actions

Copy link
Copy Markdown
Contributor

🚧 Automated critical-issue gate — this PR has an unresolved [CRITICAL] finding from the automated review, so it is kept as a draft (a draft PR can't be merged) until that is resolved. Please address the [CRITICAL] thread(s), then mark it Ready for review.

Maintainers: add the override-critical label to bypass this.

The dedup kept two same-instance candidates apart even when they shared
a MusicBrainz, Discogs or TADB id, unlike the library confirm path. Both
paths now go through one helper that lets a strong external id win.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 13, 2026 19:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation matches the described behavior and covers the relevant edge cases without introducing contract changes.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 13, 2026 19:40
@jozefKruszynski
jozefKruszynski marked this pull request as ready for review September 13, 2026 19:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation matches the stated behavior and includes focused regression coverage.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 13, 2026 19:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The identity checks match the stated behavior and are covered across the affected paths.

Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@jozefKruszynski

Copy link
Copy Markdown
Contributor Author

Status: on hold, kept as draft.

Testing against a real library showed that a streaming provider can legitimately list one artist under two ids with an identical name. Tidal does this for GUNSHIP, Seeb, Keeno and PRIZM in my own library. The rule in this PR, same instance plus different ids means distinct artists, would split those into duplicate library artists. The reported loud/LOUD case is real, but names and provider ids alone cannot tell the two situations apart, and Tidal supplies no external ids for artists that could.

The automated repair in #6327 is closed for the same reason. The likely direction is manual reassignment of a provider artist id and MBID on a library artist, backed by storing the provider id on each track and album credit so the right items follow. That needs its own design before this PR can move.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants