fix(subsonic): Sort songs by presence of lyrics for `getLyrics` #4237

kgarner7 · 2025-06-16T00:12:43Z

The current implementation of getLyrics fetches any songs matching the artist and title. However, this misses a case where there may be multiple matches for the same artist/song, and one has lyrics while the other doesn't. Resolve this by adding a custom SQL dynamic column that checks for the presence of lyrics.

The current implementation of `getLyrics` fetches any songs matching the artist and title. However, this misses a case where there may be multiple matches for the same artist/song, and one has lyrics while the other doesn't. Resolve this by adding a custom SQL dynamic column that checks for the presence of lyrics.

gemini-code-assist

Summary of Changes

Hello @kgarner7, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an issue in the Subsonic getLyrics endpoint where, if multiple songs match the same artist and title, the one with lyrics might not be returned. I've introduced a new data access method that specifically sorts matching songs to ensure the one with lyrics is returned first, improving the reliability of the getLyrics response.

Highlights

Prioritize songs with lyrics: Modified the GetLyrics Subsonic endpoint to fetch songs using a new repository method that sorts results to prioritize entries containing lyrics.
New repository method: Added a new GetAllByLyrics method to the MediaFileRepository interface and its SQL implementation. This method includes a dynamic SQL column to check for non-empty lyrics and orders results based on this column.
Test coverage: Added new test data and a dedicated test case in the persistence layer to verify that GetAllByLyrics correctly orders songs, placing those with lyrics first when multiple matches exist. Updated the Subsonic test data and mock implementation to reflect this new behavior.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

The pull request introduces a new method GetAllByLyrics to sort songs by the presence of lyrics, which is then used by the Subsonic getLyrics endpoint. The core idea is sound and addresses the issue of multiple song matches.

My main concerns are with the implementation in persistence/mediafile_repository.go:

The GetAllByLyrics method currently does not pass the options (containing filters, limits, etc.) to the underlying query builder, which means filters like artist and title will be ignored.
The SQL sorting mechanism OrderBy("lyrics desc") might not be universally robust. A more explicit ORDER BY (lyrics IS NOT NULL AND lyrics != '[]') DESC combined with secondary sort criteria from the options would be better.

The tests in persistence/mediafile_repository_test.go also need adjustment to correctly test the intended behavior, particularly regarding the application of filters and the expectation of receiving songs both with and without lyrics.

The mock implementation in server/subsonic/media_retrieval_test.go should also be updated to reflect how options (like Max) are handled by the real method.

Addressing these points will ensure the new functionality is robust and correctly integrated.

persistence/mediafile_repository.go

persistence/mediafile_repository_test.go

server/subsonic/media_retrieval_test.go

github-actions · 2025-06-16T00:29:07Z

Signed-off-by: Deluan <deluan@navidrome.org>

persistence/mediafile_repository.go

Signed-off-by: Deluan <deluan@navidrome.org>

deluan · 2025-06-16T15:43:25Z

/gemini review

gemini-code-assist

Code Review

This pull request successfully addresses the issue where getLyrics might return a song without lyrics when multiple matches exist, by prioritizing songs that have lyrics. The changes involve updating the query filter to sort by lyrics presence and updated_at, adding necessary test data, and implementing a test case to verify the new sorting logic. The mock repository is updated to support this specific sorting for testing purposes. The changes are well-contained and appear correct. No issues of medium, high, or critical severity were found during the review.

Copilot

Pull Request Overview

This PR enhances the getLyrics endpoint to prioritize songs that actually have lyrics when multiple matches exist for the same artist/title. It does so by introducing a new filter SongsByArtistTitleWithLyricsFirst, updating the SQL sort mapping to handle a dynamic “has_lyrics” column, and adding corresponding tests in both the Subsonic controller and the persistence layer.

Replace SongWithArtistTitle with SongsByArtistTitleWithLyricsFirst in GetLyrics to sort by lyrics presence first.
Extend mock repository and SQL builder to support sorting on a computed “has_lyrics” column, including tests for parentheses in sort mappings.
Augment test fixtures across media retrieval and persistence to include scenarios with and without lyrics.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
server/subsonic/media_retrieval_test.go	Updated mock `GetAll`, added lyrics‐presence test scenarios.
server/subsonic/media_retrieval.go	Switched to new filter `SongsByArtistTitleWithLyricsFirst`.
server/subsonic/filter/filters.go	Renamed/rewrote filter to sort by lyrics presence then `updated_at`.
persistence/sql_base_repository_test.go	Added tests for mapping fields with spaces (parentheses).
persistence/sql_base_repository.go	Documented parentheses requirement in sort mappings.
persistence/persistence_suite_test.go	Added test media files with lyrics and default `"[]"` handling.
persistence/mediafile_repository_test.go	Updated media count test and added repository-level lyrics sort test.

Comments suppressed due to low confidence (3)

server/subsonic/media_retrieval_test.go:322

[nitpick] The mock’s sorting logic is only verified for Order == "desc". It would be helpful to add a test for Order == "asc" to ensure ascending-order sorting also works correctly.

func (m *mockedMediaFile) GetAll(opts ...model.QueryOptions) (model.MediaFiles, error) {

server/subsonic/media_retrieval_test.go:5

Importing "cmp" will fail since there is no standard cmp package. Please use the correct import path, e.g., github.com/google/go-cmp/cmp or another supported comparison utility.

"cmp"

server/subsonic/media_retrieval_test.go:331

Using string comparison on Lyrics to detect presence may misorder items (both start with '['). Consider comparing a boolean flag (e.g., len(Lyrics) > 2) to sort by presence of lyrics, then fall back to UpdatedAt.

diff := cmp.Or(

Signed-off-by: Deluan <deluan@navidrome.org>

gemini-code-assist bot reviewed Jun 16, 2025

View reviewed changes

persistence/mediafile_repository.go Outdated Show resolved Hide resolved

persistence/mediafile_repository_test.go Outdated Show resolved Hide resolved

server/subsonic/media_retrieval_test.go Outdated Show resolved Hide resolved

kgarner7 added 2 commits June 15, 2025 17:16

add options to selectMediaFile, update test

155e37f

more robust testing of GetAllByLyrics

853a2c2

fix(subsonic): refactor GetAllByLyrics to GetAll with lyrics sorting

416f04b

Signed-off-by: Deluan <deluan@navidrome.org>

deluan reviewed Jun 16, 2025

View reviewed changes

persistence/mediafile_repository.go Outdated Show resolved Hide resolved

kgarner7 and others added 6 commits June 15, 2025 20:36

use has_lyrics, and properly support multiple sort parts

bb211f6

better handle complicated internal sorts

673403a

just use a simpler filter

b70f172

add note to setSortMappings

f08b26e

remove custom sort mapping, improve test with different updatedat

6718895

refactor tests and mock

9bf880d

Signed-off-by: Deluan <deluan@navidrome.org>

deluan force-pushed the sort-get-lyrics-by-presence branch from 258af05 to 9bf880d Compare June 16, 2025 15:40

deluan requested a review from Copilot June 16, 2025 15:43

gemini-code-assist bot reviewed Jun 16, 2025

View reviewed changes

Copilot AI reviewed Jun 16, 2025

View reviewed changes

default order when not specified is asc

19b9c44

Signed-off-by: Deluan <deluan@navidrome.org>

deluan merged commit 8d59467 into navidrome:master Jun 16, 2025
16 checks passed

kgarner7 deleted the sort-get-lyrics-by-presence branch June 17, 2025 03:22

BrewTestBot mentioned this pull request Jul 1, 2025

navidrome 0.57.0 Homebrew/homebrew-core#228738

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(subsonic): Sort songs by presence of lyrics for `getLyrics` #4237

fix(subsonic): Sort songs by presence of lyrics for `getLyrics` #4237

Uh oh!

kgarner7 commented Jun 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

deluan commented Jun 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fix(subsonic): Sort songs by presence of lyrics for getLyrics #4237

fix(subsonic): Sort songs by presence of lyrics for getLyrics #4237

Uh oh!

Conversation

kgarner7 commented Jun 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

deluan commented Jun 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

fix(subsonic): Sort songs by presence of lyrics for `getLyrics` #4237

fix(subsonic): Sort songs by presence of lyrics for `getLyrics` #4237

github-actions bot commented Jun 16, 2025 •

edited

Loading