Add comprehensive sort field infrastructure - #5498
Conversation
|
✅ The title and description are good to go. Thanks! |
There was a problem hiding this comment.
Pull request overview
Adds a new backend sorting infrastructure for library listings by introducing typed sort fields/directions, legacy compatibility parsing, and media-type-specific SQL ordering overrides (notably ARTIST_NAME for tracks/albums).
Changes:
- Replaces string-based
SORT_KEYShandling withSortField/SortDirectionparsing via_parse_order_by()and SQL generation via_get_sort_sql(). - Adds a new API endpoint
music/<mediatype>/get_sort_optionsto expose supported sort options to clients. - Implements media-type-specific
ARTIST_NAMEordering and conditional artist joins inTracksControllerandAlbumsController.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| music_assistant/controllers/music/media/base.py | Introduces the new sort parsing/SQL infrastructure and registers the new get_sort_options API endpoint. |
| music_assistant/controllers/music/media/tracks.py | Adds track-specific ARTIST_NAME ordering and joins artists when needed for sorting. |
| music_assistant/controllers/music/media/albums.py | Adds album-specific ARTIST_NAME ordering and joins artists when needed for sorting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
music_assistant/controllers/music/media/base.py:422
- [CRITICAL] get_sort_options() calls get_sort_options_for_media_type and references SortOptionInfo, but neither symbol is imported anywhere in this module, so calling this endpoint will raise NameError at runtime.
return get_sort_options_for_media_type(self.media_type)
music_assistant/controllers/music/media/tracks.py:290
- [CRITICAL] Adding an unconditional track_artists/artists JOIN for ARTIST_NAME sorting can lead to the same tables being joined a second time later in this method (the secondary artist-search query path appends another JOIN), which will break the generated SQL when both paths are hit (e.g. search term + ARTIST_NAME sort).
if order_by:
parsed = self._parse_order_by(order_by)
if parsed and parsed[0] == SortField.ARTIST_NAME:
extra_join_parts.append(
"JOIN track_artists ON track_artists.track_id = tracks.item_id "
music_assistant/controllers/music/media/base.py:1753
- [PROBLEM] _parse_order_by adds a new public API surface (field:direction + default direction rules) but there are no tests covering the new formats/edge cases (e.g. "name:desc", invalid field, invalid direction, random fields with direction). This logic is central to library listing and regressions here will be hard to spot without automated coverage.
def _parse_order_by(
self, order_by: str | None
) -> tuple[SortField, SortDirection | None] | None:
"""
Parse order_by string into SortField and SortDirection.
music_assistant/controllers/music/media/tracks.py:902
- [PROBLEM] Sorting by ARTIST_NAME orders by artists.search_name while the final query adds GROUP BY tracks.item_id whenever joins are present; for tracks with multiple artists this makes the ORDER BY value effectively arbitrary/non-deterministic. Using an aggregate for the artist sort key (or a correlated subquery) makes ordering stable.
if field == SortField.ARTIST_NAME:
if direction == SortDirection.DESC:
return "artists.search_name DESC, tracks.search_name ASC"
return "artists.search_name ASC, tracks.search_name ASC"
return super()._get_sort_sql(field, direction)
music_assistant/controllers/music/media/albums.py:733
- [PROBLEM] Sorting by ARTIST_NAME orders by artists.search_name while the final query adds GROUP BY albums.item_id whenever joins are present; for albums with multiple artists this makes the ORDER BY value effectively arbitrary/non-deterministic. Using an aggregate for the artist sort key (or a correlated subquery) makes ordering stable.
if field == SortField.ARTIST_NAME:
if direction == SortDirection.DESC:
return "artists.search_name DESC, year DESC"
return "artists.search_name ASC, year DESC"
return super()._get_sort_sql(field, direction)
|
In general this approach is good and what I had in mind. The remarks I have is that the models now leak some server logic, let's not do that and I proposed another way to implement these new fields where they are are fully typed while keeping backwards compatibility. |
ded8f68 to
c281090
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
music_assistant/controllers/music/sorting.py:178
- [PROBLEM]
MEDIA_TYPE_SORT_FIELDSdoes not includeMediaType.PODCASTorMediaType.GENRE, but there are controllers for both (media/podcasts.py,media/genres.py) that now accept the new sorting parameters and exposeget_sort_options; as-is,get_sort_options_for_media_typewill return an empty list for those media types.
MediaType.AUDIOBOOK: [
SortField.NAME,
SortField.SORT_NAME,
SortField.TIMESTAMP_ADDED,
SortField.TIMESTAMP_MODIFIED,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
music_assistant/controllers/music/sorting.py:190
- [PROBLEM]
get_sort_options_for_media_typereturns an empty list for media types not present inMEDIA_TYPE_SORT_FIELDS(notably PODCAST and GENRE), so the newget_sort_optionsAPI will expose no sort UI options for those endpoints despite them supportingorder_by/sort_field; add explicit entries for these media types (limited to fields their tables actually support).
MediaType.PLAYLIST: [
SortField.NAME,
SortField.SORT_NAME,
SortField.TIMESTAMP_ADDED,
SortField.TIMESTAMP_MODIFIED,
SortField.LAST_PLAYED,
SortField.PLAY_COUNT,
SortField.RANDOM,
SortField.RANDOM_PLAY_COUNT,
],
music_assistant/controllers/music/media/base.py:528
- [PROBLEM] When
sort_fieldis provided butsort_directionis omitted, this hardcodes an:ascdirection, which can contradict the per-field defaults exposed viaget_sort_options(e.g., TIMESTAMP_ADDED defaults to DESC) and silently produce unexpected ordering; derive the default direction from the sort option metadata (or leave direction unset) instead of forcing ASC (and apply the same logic in controllers that duplicate this block).
# Resolve sort parameters: prefer typed parameters over legacy order_by string
if sort_field is not None:
# Use new typed parameters
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
music_assistant/controllers/music/sorting.py:187
- [CRITICAL]
MEDIA_TYPE_SORT_FIELDSomits MediaType.PODCAST and MediaType.GENRE, somusic/podcasts/get_sort_optionsandmusic/genres/get_sort_optionswill return an empty list even though those controllers support sorting (and now acceptsort_field/sort_direction). Add sort-field lists for these media types so clients can build correct sorting UI.
MediaType.PLAYLIST: [
SortField.NAME,
SortField.SORT_NAME,
SortField.TIMESTAMP_ADDED,
SortField.TIMESTAMP_MODIFIED,
music_assistant/controllers/music/media/base.py:1813
- [CRITICAL]
_parse_order_bycurrently accepts anySortFieldvalue (including via the newfield:directionformat) without validating whether it’s supported for the current media type, which can surface as SQL errors (e.g., ordering tracks byyearor artists byduration). Since this PR adds a per-media-type sort-options registry, consider rejecting unsupported fields early (return None + warn) based onget_sort_options_for_media_type(self.media_type).
field_str, direction_str = order_by.split(":", 1)
try:
field = SortField(field_str)
direction = SortDirection(direction_str.lower())
return (field, direction)
music_assistant/controllers/music/media/base.py:1799
- [PROBLEM] The new sorting support (typed
sort_field/sort_directionparams and thefield:directionorder_byformat) isn’t covered by tests: the existing suite only exercises legacy keys likename_desc/sort_name. Adding a few assertions fororder_by="name:desc"andsort_field=SortField.NAMEwould protect the new parsing logic and backward compatibility.
# Parse new format (e.g., "name:desc" or "name:asc")
if ":" in order_by:
field_str, direction_str = order_by.split(":", 1)
try:
field = SortField(field_str)
direction = SortDirection(direction_str.lower())
return (field, direction)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (9)
music_assistant/controllers/music/sorting.py:186
- [PROBLEM]
MEDIA_TYPE_SORT_FIELDSdoesn’t includeMediaType.PODCASTorMediaType.GENRE, somusic/<type>/get_sort_optionswill return an empty list for those controllers and clients can’t discover supported sort fields/directions for podcasts/genres.
MediaType.PLAYLIST: [
SortField.NAME,
SortField.SORT_NAME,
SortField.TIMESTAMP_ADDED,
SortField.TIMESTAMP_MODIFIED,
SortField.LAST_PLAYED,
SortField.PLAY_COUNT,
SortField.RANDOM,
SortField.RANDOM_PLAY_COUNT,
],
music_assistant/controllers/music/media/tracks.py:276
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; this is inconsistent with the per-field defaults returned byget_sort_options(e.g., TIMESTAMP_ADDED defaults to DESC). Consider centralizing sort-parameter resolution (e.g., a base helper) so all controllers apply the same default-direction logic.
# Resolve sort parameters: prefer typed parameters over legacy order_by string
if sort_field is not None:
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/base.py:528
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; that contradicts the per-field defaults advertised byget_sort_options(e.g., TIMESTAMP_ADDED defaults to DESC) and can produce unexpected ordering.
# Resolve sort parameters: prefer typed parameters over legacy order_by string
if sort_field is not None:
# Use new typed parameters
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/albums.py:173
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; this is inconsistent with the per-field defaults returned byget_sort_options(e.g., TIMESTAMP_ADDED defaults to DESC). Consider centralizing sort-parameter resolution so albums/tracks/etc all share the same behavior.
# Resolve sort parameters: prefer typed parameters over legacy order_by string
if sort_field is not None:
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/artists.py:185
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; this is inconsistent with the per-field defaults returned byget_sort_options(e.g., LAST_PLAYED defaults to DESC).
if sort_field is not None:
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/audiobooks.py:244
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; this is inconsistent with the per-field defaults returned byget_sort_options(e.g., TIMESTAMP_ADDED defaults to DESC).
if sort_field is not None:
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/podcasts.py:117
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; this is inconsistent with the per-field defaults returned byget_sort_options.
if sort_field is not None:
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/genres.py:348
- [PROBLEM] When callers use the new typed
sort_fieldwithoutsort_direction, this forces:ascfor every field; this is inconsistent with the per-field defaults returned byget_sort_options(and would likely be surfaced in the UI as a confusing default sort direction).
if sort_field is not None:
final_order_by = (
f"{sort_field.value}:{sort_direction.value if sort_direction else 'asc'}"
)
music_assistant/controllers/music/media/base.py:1803
- [PROBLEM] New sort parsing behavior (
field:direction, special handling for RANDOM/*, legacy fallback) isn’t covered by tests; existing tests appear to only exercise legacyorder_byvalues (e.g.name_desc). Adding focused unit/integration coverage for_parse_order_byand ORDER BY generation will prevent regressions during the models/frontend rollout.
def _parse_order_by(
self, order_by: str | None
) -> tuple[SortField, SortDirection | None] | None:
"""
Parse order_by string into SortField and SortDirection.
Supports both new format (field:direction) and legacy format (field_desc).
Returns None if order_by is None or invalid.
"""
if not order_by:
return None
# Check legacy format first (e.g., "name_desc")
if order_by in LEGACY_SORT_KEYS:
return LEGACY_SORT_KEYS[order_by]
# Parse new format (e.g., "name:desc" or "name:asc")
if ":" in order_by:
field_str, direction_str = order_by.split(":", 1)
try:
field = SortField(field_str)
direction = SortDirection(direction_str.lower())
return (field, direction)
except ValueError:
self.logger.warning("Invalid sort field or direction: %s", order_by)
return None
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
music_assistant/controllers/music/media/base.py:1824
- [PROBLEM]
_resolve_sort_parametersdocstring says it always returns anorder_bystring infield:directionformat, but the function can return legacy values likesort_name(when usingorder_by/default). This mismatch can mislead future callers/maintainers about what formats_parse_order_bymust accept.
"""
Resolve sort parameters to final order_by string.
Applies proper defaults: if sort_field is given without sort_direction,
uses the field's default direction from SORT_FIELD_DEFINITIONS.
:param sort_field: Optional SortField enum value.
:param sort_direction: Optional SortDirection enum value.
:param order_by: Legacy string-based order_by parameter.
:param default: Default order_by string if none specified.
:return: Resolved order_by string in 'field:direction' format.
"""
music_assistant/controllers/music/media/base.py:1804
- [PROBLEM] New sorting behavior (
field:directionparsing and typedsort_field/sort_direction) is introduced in_parse_order_by/_get_sort_sql, but there are no tests verifying the new format maps to the expected ORDER BY SQL (including media-specific ARTIST_NAME handling). The repo already has query-builder regression tests (e.g.tests/controllers/music/test_library_listing_queries.py), so adding a few cases there would prevent silent regressions.
def _parse_order_by(
self, order_by: str | None
) -> tuple[SortField, SortDirection | None] | None:
"""
Parse order_by string into SortField and SortDirection.
Supports both new format (field:direction) and legacy format (field_desc).
Returns None if order_by is None or invalid.
"""
if not order_by:
return None
# Check legacy format first (e.g., "name_desc")
if order_by in LEGACY_SORT_KEYS:
return LEGACY_SORT_KEYS[order_by]
# Parse new format (e.g., "name:desc" or "name:asc")
if ":" in order_by:
field_str, direction_str = order_by.split(":", 1)
try:
field = SortField(field_str)
direction = SortDirection(direction_str.lower())
return (field, direction)
except ValueError:
self.logger.warning("Invalid sort field or direction: %s", order_by)
return None
# Try parsing as field without direction (default ASC)
try:
field = SortField(order_by)
# Special fields like RANDOM don't support direction
if field in (SortField.RANDOM, SortField.RANDOM_PLAY_COUNT):
return (field, None)
return (field, SortDirection.ASC)
except ValueError:
self.logger.warning("Invalid sort field: %s", order_by)
return None
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
music_assistant/controllers/music/media/base.py:2275
- [CRITICAL]
_adapt_query_for_collectionsorders the outer derived query using_get_sort_sql, which qualifies columns as{self.db_table}.<col>; the derived query has no{self.db_table}alias, so this can produce invalid SQL (e.g.,ORDER BY albums.search_name). Build the ORDER BY using unqualified derived-column names instead.
if sql_sort := self._get_sort_sql(field, direction):
sql_query += f" ORDER BY {sql_sort}"
music_assistant/controllers/music/media/base.py:1808
- [PROBLEM]
_get_sort_sqlreturns"RANDOM(), play_count"forRANDOM_PLAY_COUNTwithout qualifyingplay_count; when caller-provided JOINs include another table withplay_count(e.g., tracks joins artists), SQLite can error with an ambiguous column name. Qualifyplay_countwith the controller’s base table for this special case.
# Special fields like RANDOM don't use direction
if field in (SortField.RANDOM, SortField.RANDOM_PLAY_COUNT):
return sql_field
music_assistant/controllers/music/media/base.py:494
- [PROBLEM]
final_order_byis computed and passed intoget_library_items_by_query, but the localized-search fallback path (used for Genre/Playlist searches) still forwards the legacyorder_byinstead offinal_order_by, so requests usingsort_field/sort_directioncan return fallback results in a different sort order.
offset=offset,
order_by=final_order_by,
provider_filter=self._ensure_provider_filter(provider),
genre_ids=genre,
played_only=played_only,
Per Marcel's review feedback: - Add sort_field and sort_direction typed parameters to all library_items() - Deprecate order_by string parameter (kept for backward compatibility) - Move sort metadata from models to server (sorting.py) - Per-type SQL overrides in _get_sort_sql() for ARTIST_NAME - Covers all media types: tracks, albums, artists, podcasts, audiobooks, genres - Radio and playlists use base.py implementation directly Related: music-assistant/models#356
- Use final_order_by for ARTIST_NAME JOIN detection (tracks.py) - Add to_dict() method to SortOptionInfo for JSON serialization
Normalize new field:direction format to legacy field_desc format before validation against supported_order_keys to prevent fallback when using typed sort parameters.
Replace string-based validation with SortField enum set: - supported_order_keys (strings) → supported_sort_fields (SortField set) - Direct use of _parse_order_by() and _get_sort_sql() - Unified parsing for both legacy and new formats
…ections - Add MediaType.PODCAST and MediaType.GENRE to MEDIA_TYPE_SORT_FIELDS - Add get_default_direction() helper to return per-field defaults - Add _resolve_sort_parameters() base method for centralized resolution - Replace duplicated logic in all controllers with helper method - Ensures typed sort_field without sort_direction uses field's default (e.g., TIMESTAMP_ADDED defaults to DESC, not forced ASC)
…reorder library_items parameters, clean up docstrings
Fix Ruff PLC0415 error by moving the import from inside the function to the module-level imports.
fac4f4e to
1f2b473
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
music_assistant/controllers/music/media/base.py:2311
- [CRITICAL]
_adapt_query_for_collectionsbuilds an ORDER BY via_get_sort_sql(), which qualifies columns with the base table name (e.g.albums.search_sort_name), but this query orders a derived UNION/CTE result set where those table names are not in scope, causing SQLite errors likeno such column: albums.search_sort_namewhen collections collapsing is used.
if sql_sort := self._get_sort_sql(field, direction):
sql_query += f" ORDER BY {sql_sort}"
music_assistant/controllers/music/media/tracks.py:907
- [PROBLEM]
library_itemsadds a JOIN totrack_artists/artistsfor ARTIST_NAME sorting and the base query then appliesGROUP BY tracks.item_id; ordering byartists.search_namein this grouped query is undefined for multi-artist tracks (SQLite will pick an arbitrary row), leading to unstable/non-deterministic sorting.
if field == SortField.ARTIST_NAME:
if direction == SortDirection.DESC:
return "artists.search_name DESC, tracks.search_name ASC"
return "artists.search_name ASC, tracks.search_name ASC"
music_assistant/controllers/music/media/albums.py:738
- [PROBLEM]
library_itemsadds a JOIN toalbum_artists/artistsfor ARTIST_NAME sorting and the base query then appliesGROUP BY albums.item_id; ordering byartists.search_namein this grouped query is undefined for multi-artist albums (SQLite will pick an arbitrary row), leading to unstable/non-deterministic sorting.
if field == SortField.ARTIST_NAME:
if direction == SortDirection.DESC:
return "artists.search_name DESC, year DESC"
return "artists.search_name ASC, year DESC"
When collapse_collections=True, the query is wrapped in a CTE where column names are not table-qualified. Strip table qualifiers from sort SQL to fix 'no such column: audiobooks.search_name' errors.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (2)
music_assistant/controllers/music/media/base.py:1836
- [PROBLEM]
_get_sort_sqlreturns the rawBASE_SORT_FIELD_SQLforSortField.RANDOM_PLAY_COUNTwithout qualifyingplay_count, which becomes an SQLite "ambiguous column name: play_count" error when the query adds JOINs (e.g., track/album listings joined toartists, which also has aplay_countcolumn).
# Special fields like RANDOM don't use direction
if field in (SortField.RANDOM, SortField.RANDOM_PLAY_COUNT):
return sql_field
music_assistant/controllers/music/media/base.py:164
- [PROBLEM] This PR adds a new API command (
music/<type>/get_sort_options), butAPI_SCHEMA_VERSIONis not bumped, so clients cannot reliably feature-detect this endpoint viaschema_version. Please bumpAPI_SCHEMA_VERSIONinmusic_assistant/constants.pyas required when adding API commands.
self.mass.register_api_command(
f"music/{api_base}/get_sort_options",
self.get_sort_options,
required_scope=Scope.LIBRARY_READ,
)
What does this implement/fix?
Implements comprehensive backend infrastructure for improved library sorting, including media-type specific sort logic and backward compatibility.
Depends on: music-assistant/models#356 (must be merged first)
Related discussions:
Changes
Base Controller (
base.py)BASE_SORT_FIELD_SQL: Maps SortField enum to SQL expressions for common fields (ARTIST_NAME intentionally omitted - implemented per MediaType in subclasses)LEGACY_SORT_KEYS: Backward compatibility mapping for old"field_desc"formatget_sort_options(): New API endpoint returninglist[SortOptionInfo]for the MediaType_parse_order_by(): Parses both new"field:direction"and legacy"field_desc"formats_get_sort_sql(): Translates SortField + SortDirection to SQL ORDER BY clause (can be overridden)_build_final_query(): Uses new parsing logicMedia-Type Specific Controllers
TracksController (
tracks.py)_get_sort_sql()override: For ARTIST_NAME returns"artists.search_name ASC/DESC, tracks.search_name ASC"(secondary sort on track name)library_items()modified: Detects ARTIST_NAME sorting and adds artist JOINAlbumsController (
albums.py)_get_sort_sql()override: For ARTIST_NAME returns"artists.search_name ASC/DESC, year DESC"(secondary sort on year)library_items()modified: Detects ARTIST_NAME sorting and adds artist JOINFormat
"field:direction"(e.g.,"name:asc","timestamp_added:desc")"field_desc"(e.g.,"name_desc","timestamp_added")Types of changes
new-featureenhancementChecklist
pre-commit run --all-filespasses. (Currently fails on missing models imports - will pass after rebase on models#356)pytestpasses, and tests have been added/updated undertests/where applicable. (No new tests needed - refactors existing sort logic)music-assistant/modelsis linked.music-assistant/frontendis linked. (Frontend PR to follow)