From fcb5e1b80624ecb8b7e1d80656f799f0cb2b80f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Sat, 26 Oct 2024 14:06:34 -0400 Subject: [PATCH] fix(server): fix case-insensitive sort order and add indexes to improve performance (#3425) * refactor(server): better sort mappings * refactor(server): simplify GetIndex * fix: recreate tables and indexes using proper collation Also add tests to ensure proper collation * chore: remove unused method * fix: sort expressions * fix: lint errors * fix: cleanup --- ...5533_fix_sort_tags_collation_and_index.sql | 512 ++++++++++++++++++ model/album.go | 1 - model/mediafile.go | 4 +- model/mediafile_test.go | 1 - persistence/album_repository.go | 30 +- persistence/artist_repository.go | 56 +- persistence/artist_repository_test.go | 260 +++++---- persistence/collation_test.go | 122 +++++ persistence/helpers.go | 10 + persistence/helpers_test.go | 19 + persistence/mediafile_repository.go | 40 +- persistence/player_repository.go | 4 +- persistence/playlist_repository.go | 4 +- persistence/playlist_track_repository.go | 14 +- persistence/radio_repository.go | 3 - persistence/share_repository.go | 6 +- persistence/sql_base_repository.go | 29 +- scanner/playlist_importer_test.go | 17 +- 18 files changed, 861 insertions(+), 271 deletions(-) create mode 100644 db/migrations/20241024125533_fix_sort_tags_collation_and_index.sql create mode 100644 persistence/collation_test.go diff --git a/db/migrations/20241024125533_fix_sort_tags_collation_and_index.sql b/db/migrations/20241024125533_fix_sort_tags_collation_and_index.sql new file mode 100644 index 000000000..d2edde0ec --- /dev/null +++ b/db/migrations/20241024125533_fix_sort_tags_collation_and_index.sql @@ -0,0 +1,512 @@ +-- +goose Up +--region Artist Table +create table artist_dg_tmp +( + id varchar(255) not null + primary key, + name varchar(255) default '' not null, + album_count integer default 0 not null, + full_text varchar(255) default '', + song_count integer default 0 not null, + size integer default 0 not null, + biography varchar(255) default '' not null, + small_image_url varchar(255) default '' not null, + medium_image_url varchar(255) default '' not null, + large_image_url varchar(255) default '' not null, + similar_artists varchar(255) default '' not null, + external_url varchar(255) default '' not null, + external_info_updated_at datetime, + order_artist_name varchar collate NOCASE default '' not null, + sort_artist_name varchar collate NOCASE default '' not null, + mbz_artist_id varchar default '' not null +); + +insert into artist_dg_tmp(id, name, album_count, full_text, song_count, size, biography, small_image_url, + medium_image_url, large_image_url, similar_artists, external_url, external_info_updated_at, + order_artist_name, sort_artist_name, mbz_artist_id) +select id, + name, + album_count, + full_text, + song_count, + size, + biography, + small_image_url, + medium_image_url, + large_image_url, + similar_artists, + external_url, + external_info_updated_at, + order_artist_name, + sort_artist_name, + mbz_artist_id +from artist; + +drop table artist; + +alter table artist_dg_tmp + rename to artist; + +create index artist_full_text + on artist (full_text); + +create index artist_name + on artist (name); + +create index artist_order_artist_name + on artist (order_artist_name); + +create index artist_size + on artist (size); + +create index artist_sort_name + on artist (coalesce(nullif(sort_artist_name,''),order_artist_name) collate NOCASE); + +--endregion + +--region Album Table +create table album_dg_tmp +( + id varchar(255) not null + primary key, + name varchar(255) default '' not null, + artist_id varchar(255) default '' not null, + embed_art_path varchar(255) default '' not null, + artist varchar(255) default '' not null, + album_artist varchar(255) default '' not null, + min_year int default 0 not null, + max_year integer default 0 not null, + compilation bool default FALSE not null, + song_count integer default 0 not null, + duration real default 0 not null, + genre varchar(255) default '' not null, + created_at datetime, + updated_at datetime, + full_text varchar(255) default '', + album_artist_id varchar(255) default '', + size integer default 0 not null, + all_artist_ids varchar, + description varchar(255) default '' not null, + small_image_url varchar(255) default '' not null, + medium_image_url varchar(255) default '' not null, + large_image_url varchar(255) default '' not null, + external_url varchar(255) default '' not null, + external_info_updated_at datetime, + date varchar(255) default '' not null, + min_original_year int default 0 not null, + max_original_year int default 0 not null, + original_date varchar(255) default '' not null, + release_date varchar(255) default '' not null, + releases integer default 0 not null, + image_files varchar default '' not null, + order_album_name varchar collate NOCASE default '' not null, + order_album_artist_name varchar collate NOCASE default '' not null, + sort_album_name varchar collate NOCASE default '' not null, + sort_album_artist_name varchar collate NOCASE default '' not null, + catalog_num varchar default '' not null, + comment varchar default '' not null, + paths varchar default '' not null, + mbz_album_id varchar default '' not null, + mbz_album_artist_id varchar default '' not null, + mbz_album_type varchar default '' not null, + mbz_album_comment varchar default '' not null, + discs jsonb default '{}' not null, + library_id integer default 1 not null + references library + on delete cascade +); + +insert into album_dg_tmp(id, name, artist_id, embed_art_path, artist, album_artist, min_year, max_year, compilation, + song_count, duration, genre, created_at, updated_at, full_text, album_artist_id, size, + all_artist_ids, description, small_image_url, medium_image_url, large_image_url, external_url, + external_info_updated_at, date, min_original_year, max_original_year, original_date, + release_date, releases, image_files, order_album_name, order_album_artist_name, + sort_album_name, sort_album_artist_name, catalog_num, comment, paths, + mbz_album_id, mbz_album_artist_id, mbz_album_type, mbz_album_comment, discs, library_id) +select id, + name, + artist_id, + embed_art_path, + artist, + album_artist, + min_year, + max_year, + compilation, + song_count, + duration, + genre, + created_at, + updated_at, + full_text, + album_artist_id, + size, + all_artist_ids, + description, + small_image_url, + medium_image_url, + large_image_url, + external_url, + external_info_updated_at, + date, + min_original_year, + max_original_year, + original_date, + release_date, + releases, + image_files, + order_album_name, + order_album_artist_name, + sort_album_name, + sort_album_artist_name, + catalog_num, + comment, + paths, + mbz_album_id, + mbz_album_artist_id, + mbz_album_type, + mbz_album_comment, + discs, + library_id +from album; + +drop table album; + +alter table album_dg_tmp + rename to album; + +create index album_all_artist_ids + on album (all_artist_ids); + +create index album_alphabetical_by_artist + on album (compilation, order_album_artist_name, order_album_name); + +create index album_artist + on album (artist); + +create index album_artist_album + on album (artist); + +create index album_artist_album_id + on album (album_artist_id); + +create index album_artist_id + on album (artist_id); + +create index album_created_at + on album (created_at); + +create index album_full_text + on album (full_text); + +create index album_genre + on album (genre); + +create index album_max_year + on album (max_year); + +create index album_mbz_album_type + on album (mbz_album_type); + +create index album_min_year + on album (min_year); + +create index album_name + on album (name); + +create index album_order_album_artist_name + on album (order_album_artist_name); + +create index album_order_album_name + on album (order_album_name); + +create index album_size + on album (size); + +create index album_sort_name + on album (coalesce(nullif(sort_album_name,''),order_album_name) collate NOCASE); + +create index album_sort_album_artist_name + on album (coalesce(nullif(sort_album_artist_name,''),order_album_artist_name) collate NOCASE); + +create index album_updated_at + on album (updated_at); +--endregion + +--region Media File Table +create table media_file_dg_tmp +( + id varchar(255) not null + primary key, + path varchar(255) default '' not null, + title varchar(255) default '' not null, + album varchar(255) default '' not null, + artist varchar(255) default '' not null, + artist_id varchar(255) default '' not null, + album_artist varchar(255) default '' not null, + album_id varchar(255) default '' not null, + has_cover_art bool default FALSE not null, + track_number integer default 0 not null, + disc_number integer default 0 not null, + year integer default 0 not null, + size integer default 0 not null, + suffix varchar(255) default '' not null, + duration real default 0 not null, + bit_rate integer default 0 not null, + genre varchar(255) default '' not null, + compilation bool default FALSE not null, + created_at datetime, + updated_at datetime, + full_text varchar(255) default '', + album_artist_id varchar(255) default '', + date varchar(255) default '' not null, + original_year int default 0 not null, + original_date varchar(255) default '' not null, + release_year int default 0 not null, + release_date varchar(255) default '' not null, + order_album_name varchar collate NOCASE default '' not null, + order_album_artist_name varchar collate NOCASE default '' not null, + order_artist_name varchar collate NOCASE default '' not null, + sort_album_name varchar collate NOCASE default '' not null, + sort_artist_name varchar collate NOCASE default '' not null, + sort_album_artist_name varchar collate NOCASE default '' not null, + sort_title varchar collate NOCASE default '' not null, + disc_subtitle varchar default '' not null, + catalog_num varchar default '' not null, + comment varchar default '' not null, + order_title varchar collate NOCASE default '' not null, + mbz_recording_id varchar default '' not null, + mbz_album_id varchar default '' not null, + mbz_artist_id varchar default '' not null, + mbz_album_artist_id varchar default '' not null, + mbz_album_type varchar default '' not null, + mbz_album_comment varchar default '' not null, + mbz_release_track_id varchar default '' not null, + bpm integer default 0 not null, + channels integer default 0 not null, + rg_album_gain real default 0 not null, + rg_album_peak real default 0 not null, + rg_track_gain real default 0 not null, + rg_track_peak real default 0 not null, + lyrics jsonb default '[]' not null, + sample_rate integer default 0 not null, + library_id integer default 1 not null + references library + on delete cascade +); + +insert into media_file_dg_tmp(id, path, title, album, artist, artist_id, album_artist, album_id, has_cover_art, + track_number, disc_number, year, size, suffix, duration, bit_rate, genre, compilation, + created_at, updated_at, full_text, album_artist_id, date, original_year, original_date, + release_year, release_date, order_album_name, order_album_artist_name, order_artist_name, + sort_album_name, sort_artist_name, sort_album_artist_name, sort_title, disc_subtitle, + catalog_num, comment, order_title, mbz_recording_id, mbz_album_id, mbz_artist_id, + mbz_album_artist_id, mbz_album_type, mbz_album_comment, mbz_release_track_id, bpm, + channels, rg_album_gain, rg_album_peak, rg_track_gain, rg_track_peak, lyrics, sample_rate, + library_id) +select id, + path, + title, + album, + artist, + artist_id, + album_artist, + album_id, + has_cover_art, + track_number, + disc_number, + year, + size, + suffix, + duration, + bit_rate, + genre, + compilation, + created_at, + updated_at, + full_text, + album_artist_id, + date, + original_year, + original_date, + release_year, + release_date, + order_album_name, + order_album_artist_name, + order_artist_name, + sort_album_name, + sort_artist_name, + sort_album_artist_name, + sort_title, + disc_subtitle, + catalog_num, + comment, + order_title, + mbz_recording_id, + mbz_album_id, + mbz_artist_id, + mbz_album_artist_id, + mbz_album_type, + mbz_album_comment, + mbz_release_track_id, + bpm, + channels, + rg_album_gain, + rg_album_peak, + rg_track_gain, + rg_track_peak, + lyrics, + sample_rate, + library_id +from media_file; + +drop table media_file; + +alter table media_file_dg_tmp + rename to media_file; + +create index media_file_album_artist + on media_file (album_artist); + +create index media_file_album_id + on media_file (album_id); + +create index media_file_artist + on media_file (artist); + +create index media_file_artist_album_id + on media_file (album_artist_id); + +create index media_file_artist_id + on media_file (artist_id); + +create index media_file_bpm + on media_file (bpm); + +create index media_file_channels + on media_file (channels); + +create index media_file_created_at + on media_file (created_at); + +create index media_file_duration + on media_file (duration); + +create index media_file_full_text + on media_file (full_text); + +create index media_file_genre + on media_file (genre); + +create index media_file_mbz_track_id + on media_file (mbz_recording_id); + +create index media_file_order_album_name + on media_file (order_album_name); + +create index media_file_order_artist_name + on media_file (order_artist_name); + +create index media_file_order_title + on media_file (order_title); + +create index media_file_path + on media_file (path); + +create index media_file_path_nocase + on media_file (path collate NOCASE); + +create index media_file_sample_rate + on media_file (sample_rate); + +create index media_file_sort_title + on media_file (coalesce(nullif(sort_title,''),order_title) collate NOCASE); + +create index media_file_sort_artist_name + on media_file (coalesce(nullif(sort_artist_name,''),order_artist_name) collate NOCASE); + +create index media_file_sort_album_name + on media_file (coalesce(nullif(sort_album_name,''),order_album_name) collate NOCASE); + +create index media_file_title + on media_file (title); + +create index media_file_track_number + on media_file (disc_number, track_number); + +create index media_file_updated_at + on media_file (updated_at); + +create index media_file_year + on media_file (year); + +--endregion + +--region Radio Table +create table radio_dg_tmp +( + id varchar(255) not null + primary key, + name varchar collate NOCASE not null + unique, + stream_url varchar not null, + home_page_url varchar default '' not null, + created_at datetime, + updated_at datetime +); + +insert into radio_dg_tmp(id, name, stream_url, home_page_url, created_at, updated_at) +select id, name, stream_url, home_page_url, created_at, updated_at +from radio; + +drop table radio; + +alter table radio_dg_tmp + rename to radio; + +create index radio_name + on radio(name); +--endregion + +--region users Table +create table user_dg_tmp +( + id varchar(255) not null + primary key, + user_name varchar(255) default '' not null + unique, + name varchar(255) collate NOCASE default '' not null, + email varchar(255) default '' not null, + password varchar(255) default '' not null, + is_admin bool default FALSE not null, + last_login_at datetime, + last_access_at datetime, + created_at datetime not null, + updated_at datetime not null +); + +insert into user_dg_tmp(id, user_name, name, email, password, is_admin, last_login_at, last_access_at, created_at, + updated_at) +select id, + user_name, + name, + email, + password, + is_admin, + last_login_at, + last_access_at, + created_at, + updated_at +from user; + +drop table user; + +alter table user_dg_tmp + rename to user; + +create index user_username_password + on user(user_name collate NOCASE, password); +--endregion + +-- +goose Down +alter table album + add column sort_artist_name varchar default '' not null; diff --git a/model/album.go b/model/album.go index 05fa8b798..76bd9f4fa 100644 --- a/model/album.go +++ b/model/album.go @@ -38,7 +38,6 @@ type Album struct { Discs Discs `structs:"discs" json:"discs,omitempty"` FullText string `structs:"full_text" json:"-"` SortAlbumName string `structs:"sort_album_name" json:"sortAlbumName,omitempty"` - SortArtistName string `structs:"sort_artist_name" json:"sortArtistName,omitempty"` SortAlbumArtistName string `structs:"sort_album_artist_name" json:"sortAlbumArtistName,omitempty"` OrderAlbumName string `structs:"order_album_name" json:"orderAlbumName"` OrderAlbumArtistName string `structs:"order_album_artist_name" json:"orderAlbumArtistName"` diff --git a/model/mediafile.go b/model/mediafile.go index ed7b063e6..8abc685fd 100644 --- a/model/mediafile.go +++ b/model/mediafile.go @@ -140,7 +140,6 @@ func (mfs MediaFiles) ToAlbum() Album { a.AlbumArtist = m.AlbumArtist a.AlbumArtistID = m.AlbumArtistID a.SortAlbumName = m.SortAlbumName - a.SortArtistName = m.SortArtistName a.SortAlbumArtistName = m.SortAlbumArtistName a.OrderAlbumName = m.OrderAlbumName a.OrderAlbumArtistName = m.OrderAlbumArtistName @@ -261,11 +260,10 @@ type MediaFileRepository interface { GetAll(options ...QueryOptions) (MediaFiles, error) Search(q string, offset int, size int) (MediaFiles, error) Delete(id string) error + FindByPaths(paths []string) (MediaFiles, error) // Queries by path to support the scanner, no Annotations or Bookmarks required in the response FindAllByPath(path string) (MediaFiles, error) - FindByPath(path string) (*MediaFile, error) - FindByPaths(paths []string) (MediaFiles, error) FindPathsRecursively(basePath string) ([]string, error) DeleteByPath(path string) (int64, error) diff --git a/model/mediafile_test.go b/model/mediafile_test.go index fb8aa8a14..0aad38384 100644 --- a/model/mediafile_test.go +++ b/model/mediafile_test.go @@ -43,7 +43,6 @@ var _ = Describe("MediaFiles", func() { Expect(album.AlbumArtist).To(Equal("AlbumArtist")) Expect(album.AlbumArtistID).To(Equal("AlbumArtistID")) Expect(album.SortAlbumName).To(Equal("SortAlbumName")) - Expect(album.SortArtistName).To(Equal("SortArtistName")) Expect(album.SortAlbumArtistName).To(Equal("SortAlbumArtistName")) Expect(album.OrderAlbumName).To(Equal("OrderAlbumName")) Expect(album.OrderAlbumArtistName).To(Equal("OrderAlbumArtistName")) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 2ee94973b..cfae5c19e 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -69,27 +69,15 @@ func NewAlbumRepository(ctx context.Context, db dbx.Builder) model.AlbumReposito "has_rating": hasRatingFilter, "genre_id": eqFilter, }) - if conf.Server.PreferSortTags { - r.sortMappings = map[string]string{ - "name": "COALESCE(NULLIF(sort_album_name,''),order_album_name)", - "artist": "compilation asc, COALESCE(NULLIF(sort_album_artist_name,''),order_album_artist_name) asc, COALESCE(NULLIF(sort_album_name,''),order_album_name) asc", - "album_artist": "compilation asc, COALESCE(NULLIF(sort_album_artist_name,''),order_album_artist_name) asc, COALESCE(NULLIF(sort_album_name,''),order_album_name) asc", - "max_year": "coalesce(nullif(original_date,''), cast(max_year as text)), release_date, name, COALESCE(NULLIF(sort_album_name,''),order_album_name) asc", - "random": "random", - "recently_added": recentlyAddedSort(), - "starred_at": "starred, starred_at", - } - } else { - r.sortMappings = map[string]string{ - "name": "order_album_name asc, order_album_artist_name asc", - "artist": "compilation asc, order_album_artist_name asc, order_album_name asc", - "album_artist": "compilation asc, order_album_artist_name asc, order_album_name asc", - "max_year": "coalesce(nullif(original_date,''), cast(max_year as text)), release_date, name, order_album_name asc", - "random": "random", - "recently_added": recentlyAddedSort(), - "starred_at": "starred, starred_at", - } - } + r.setSortMappings(map[string]string{ + "name": "order_album_name, order_album_artist_name", + "artist": "compilation, order_album_artist_name, order_album_name", + "album_artist": "compilation, order_album_artist_name, order_album_name", + "max_year": "coalesce(nullif(original_date,''), cast(max_year as text)), release_date, name", + "random": "random", + "recently_added": recentlyAddedSort(), + "starred_at": "starred, starred_at", + }) return r } diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index af7c30138..c176ac7a9 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -5,7 +5,7 @@ import ( "context" "fmt" "net/url" - "sort" + "slices" "strings" . "github.com/Masterminds/squirrel" @@ -15,7 +15,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils" - "github.com/navidrome/navidrome/utils/str" + "github.com/navidrome/navidrome/utils/slice" "github.com/pocketbase/dbx" ) @@ -67,17 +67,10 @@ func NewArtistRepository(ctx context.Context, db dbx.Builder) model.ArtistReposi "starred": booleanFilter, "genre_id": eqFilter, }) - if conf.Server.PreferSortTags { - r.sortMappings = map[string]string{ - "name": "COALESCE(NULLIF(sort_artist_name,''),order_artist_name)", - "starred_at": "starred, starred_at", - } - } else { - r.sortMappings = map[string]string{ - "name": "order_artist_name", - "starred_at": "starred, starred_at", - } - } + r.setSortMappings(map[string]string{ + "name": "order_artist_name", + "starred_at": "starred, starred_at", + }) return r } @@ -143,15 +136,14 @@ func (r *artistRepository) toModels(dba []dbArtist) model.Artists { return res } -func (r *artistRepository) getIndexKey(a *model.Artist) string { - source := a.Name +func (r *artistRepository) getIndexKey(a model.Artist) string { + source := a.OrderArtistName if conf.Server.PreferSortTags { - source = cmp.Or(a.SortArtistName, a.OrderArtistName, source) + source = cmp.Or(a.SortArtistName, a.OrderArtistName) } - name := strings.ToLower(str.RemoveArticle(source)) + name := strings.ToLower(source) for k, v := range r.indexGroups { - key := strings.ToLower(k) - if strings.HasPrefix(name, key) { + if strings.HasPrefix(name, strings.ToLower(k)) { return v } } @@ -160,32 +152,16 @@ func (r *artistRepository) getIndexKey(a *model.Artist) string { // TODO Cache the index (recalculate when there are changes to the DB) func (r *artistRepository) GetIndex() (model.ArtistIndexes, error) { - sortColumn := "order_artist_name" - if conf.Server.PreferSortTags { - sortColumn = "sort_artist_name, order_artist_name" - } - all, err := r.GetAll(model.QueryOptions{Sort: sortColumn}) + artists, err := r.GetAll(model.QueryOptions{Sort: "name"}) if err != nil { return nil, err } - - fullIdx := make(map[string]*model.ArtistIndex) - for i := range all { - a := all[i] - ax := r.getIndexKey(&a) - idx, ok := fullIdx[ax] - if !ok { - idx = &model.ArtistIndex{ID: ax} - fullIdx[ax] = idx - } - idx.Artists = append(idx.Artists, a) - } var result model.ArtistIndexes - for _, idx := range fullIdx { - result = append(result, *idx) + for k, v := range slice.Group(artists, r.getIndexKey) { + result = append(result, model.ArtistIndex{ID: k, Artists: v}) } - sort.Slice(result, func(i, j int) bool { - return result[i].ID < result[j].ID + slices.SortFunc(result, func(a, b model.ArtistIndex) int { + return cmp.Compare(a.ID, b.ID) }) return result, nil } diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index 31b035cec..0de468c02 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -5,6 +5,7 @@ import ( "github.com/fatih/structs" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -46,163 +47,146 @@ var _ = Describe("ArtistRepository", func() { }) Describe("GetIndexKey", func() { + // Note: OrderArtistName should never be empty, so we don't need to test for that r := artistRepository{indexGroups: utils.ParseIndexGroups(conf.Server.IndexGroups)} - It("returns the index key when PreferSortTags is true and SortArtistName is not empty", func() { - conf.Server.PreferSortTags = true - a := model.Artist{SortArtistName: "Foo", OrderArtistName: "Bar", Name: "Qux"} - idx := GetIndexKey(&r, &a) // defines export_test.go - Expect(idx).To(Equal("F")) - - a = model.Artist{SortArtistName: "foo", OrderArtistName: "Bar", Name: "Qux"} - idx = GetIndexKey(&r, &a) - Expect(idx).To(Equal("F")) + When("PreferSortTags is false", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig) + conf.Server.PreferSortTags = false + }) + It("returns the OrderArtistName key is SortArtistName is empty", func() { + conf.Server.PreferSortTags = false + a := model.Artist{SortArtistName: "", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, a) + Expect(idx).To(Equal("B")) + }) + It("returns the OrderArtistName key even if SortArtistName is not empty", func() { + a := model.Artist{SortArtistName: "Foo", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, a) + Expect(idx).To(Equal("B")) + }) }) - - It("returns the index key when PreferSortTags is true, SortArtistName is empty and OrderArtistName is not empty", func() { - conf.Server.PreferSortTags = true - a := model.Artist{SortArtistName: "", OrderArtistName: "Bar", Name: "Qux"} - idx := GetIndexKey(&r, &a) - Expect(idx).To(Equal("B")) - - a = model.Artist{SortArtistName: "", OrderArtistName: "bar", Name: "Qux"} - idx = GetIndexKey(&r, &a) - Expect(idx).To(Equal("B")) - }) - - It("returns the index key when PreferSortTags is true, both SortArtistName, OrderArtistName are empty", func() { - conf.Server.PreferSortTags = true - a := model.Artist{SortArtistName: "", OrderArtistName: "", Name: "Qux"} - idx := GetIndexKey(&r, &a) - Expect(idx).To(Equal("Q")) - - a = model.Artist{SortArtistName: "", OrderArtistName: "", Name: "qux"} - idx = GetIndexKey(&r, &a) - Expect(idx).To(Equal("Q")) - }) - - It("returns the index key when PreferSortTags is false and SortArtistName is not empty", func() { - conf.Server.PreferSortTags = false - a := model.Artist{SortArtistName: "Foo", OrderArtistName: "Bar", Name: "Qux"} - idx := GetIndexKey(&r, &a) - Expect(idx).To(Equal("Q")) - }) - - It("returns the index key when PreferSortTags is true, SortArtistName is empty and OrderArtistName is not empty", func() { - conf.Server.PreferSortTags = false - a := model.Artist{SortArtistName: "", OrderArtistName: "Bar", Name: "Qux"} - idx := GetIndexKey(&r, &a) - Expect(idx).To(Equal("Q")) - }) - - It("returns the index key when PreferSortTags is true, both sort_artist_name, order_artist_name are empty", func() { - conf.Server.PreferSortTags = false - a := model.Artist{SortArtistName: "", OrderArtistName: "", Name: "Qux"} - idx := GetIndexKey(&r, &a) - Expect(idx).To(Equal("Q")) - - a = model.Artist{SortArtistName: "", OrderArtistName: "", Name: "qux"} - idx = GetIndexKey(&r, &a) - Expect(idx).To(Equal("Q")) + When("PreferSortTags is true", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig) + conf.Server.PreferSortTags = true + }) + It("returns the SortArtistName key if it is not empty", func() { + a := model.Artist{SortArtistName: "Foo", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, a) + Expect(idx).To(Equal("F")) + }) + It("returns the OrderArtistName key if SortArtistName is empty", func() { + a := model.Artist{SortArtistName: "", OrderArtistName: "Bar", Name: "Qux"} + idx := GetIndexKey(&r, a) + Expect(idx).To(Equal("B")) + }) }) }) Describe("GetIndex", func() { - It("returns the index when PreferSortTags is true and SortArtistName is not empty", func() { - conf.Server.PreferSortTags = true + When("PreferSortTags is true", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig) + conf.Server.PreferSortTags = true + }) + It("returns the index when SortArtistName is not empty", func() { + artistBeatles.SortArtistName = "Foo" + er := repo.Put(&artistBeatles) + Expect(er).To(BeNil()) - artistBeatles.SortArtistName = "Foo" - er := repo.Put(&artistBeatles) - Expect(er).To(BeNil()) - - idx, err := repo.GetIndex() - Expect(err).To(BeNil()) - Expect(idx).To(Equal(model.ArtistIndexes{ - { - ID: "F", - Artists: model.Artists{ - artistBeatles, + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "F", + Artists: model.Artists{ + artistBeatles, + }, }, - }, - { - ID: "K", - Artists: model.Artists{ - artistKraftwerk, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, }, - }, - })) + })) - artistBeatles.SortArtistName = "" - er = repo.Put(&artistBeatles) - Expect(er).To(BeNil()) + artistBeatles.SortArtistName = "" + er = repo.Put(&artistBeatles) + Expect(er).To(BeNil()) + }) + + It("returns the index when SortArtistName is empty", func() { + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "B", + Artists: model.Artists{ + artistBeatles, + }, + }, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, + }, + })) + }) }) - It("returns the index when PreferSortTags is true and SortArtistName is empty", func() { - conf.Server.PreferSortTags = true - idx, err := repo.GetIndex() - Expect(err).To(BeNil()) - Expect(idx).To(Equal(model.ArtistIndexes{ - { - ID: "B", - Artists: model.Artists{ - artistBeatles, - }, - }, - { - ID: "K", - Artists: model.Artists{ - artistKraftwerk, - }, - }, - })) - }) + When("PreferSortTags is false", func() { + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig) + conf.Server.PreferSortTags = false + }) + It("returns the index when SortArtistName is not empty", func() { + artistBeatles.SortArtistName = "Foo" + er := repo.Put(&artistBeatles) + Expect(er).To(BeNil()) - It("returns the index when PreferSortTags is false and SortArtistName is not empty", func() { - conf.Server.PreferSortTags = false - - artistBeatles.SortArtistName = "Foo" - er := repo.Put(&artistBeatles) - Expect(er).To(BeNil()) - - idx, err := repo.GetIndex() - Expect(err).To(BeNil()) - Expect(idx).To(Equal(model.ArtistIndexes{ - { - ID: "B", - Artists: model.Artists{ - artistBeatles, + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "B", + Artists: model.Artists{ + artistBeatles, + }, }, - }, - { - ID: "K", - Artists: model.Artists{ - artistKraftwerk, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, }, - }, - })) + })) - artistBeatles.SortArtistName = "" - er = repo.Put(&artistBeatles) - Expect(er).To(BeNil()) - }) + artistBeatles.SortArtistName = "" + er = repo.Put(&artistBeatles) + Expect(er).To(BeNil()) + }) - It("returns the index when PreferSortTags is false and SortArtistName is empty", func() { - conf.Server.PreferSortTags = false - idx, err := repo.GetIndex() - Expect(err).To(BeNil()) - Expect(idx).To(Equal(model.ArtistIndexes{ - { - ID: "B", - Artists: model.Artists{ - artistBeatles, + It("returns the index when SortArtistName is empty", func() { + idx, err := repo.GetIndex() + Expect(err).To(BeNil()) + Expect(idx).To(Equal(model.ArtistIndexes{ + { + ID: "B", + Artists: model.Artists{ + artistBeatles, + }, }, - }, - { - ID: "K", - Artists: model.Artists{ - artistKraftwerk, + { + ID: "K", + Artists: model.Artists{ + artistKraftwerk, + }, }, - }, - })) + })) + }) }) }) diff --git a/persistence/collation_test.go b/persistence/collation_test.go new file mode 100644 index 000000000..87e582670 --- /dev/null +++ b/persistence/collation_test.go @@ -0,0 +1,122 @@ +package persistence + +import ( + "database/sql" + "errors" + "fmt" + "regexp" + + "github.com/navidrome/navidrome/db" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// When creating migrations that change existing columns, it is easy to miss the original collation of a column. +// These tests enforce that the required collation of the columns and indexes in the database are kept in place. +// This is important to ensure that the database can perform fast case-insensitive searches and sorts. +var _ = Describe("Collation", func() { + conn := db.Db().ReadDB() + DescribeTable("Column collation", + func(table, column string) { + Expect(checkCollation(conn, table, column)).To(Succeed()) + }, + Entry("artist.order_artist_name", "artist", "order_artist_name"), + Entry("artist.sort_artist_name", "artist", "sort_artist_name"), + Entry("album.order_album_name", "album", "order_album_name"), + Entry("album.order_album_artist_name", "album", "order_album_artist_name"), + Entry("album.sort_album_name", "album", "sort_album_name"), + Entry("album.sort_album_artist_name", "album", "sort_album_artist_name"), + Entry("media_file.order_title", "media_file", "order_title"), + Entry("media_file.order_album_name", "media_file", "order_album_name"), + Entry("media_file.order_artist_name", "media_file", "order_artist_name"), + Entry("media_file.sort_title", "media_file", "sort_title"), + Entry("media_file.sort_album_name", "media_file", "sort_album_name"), + Entry("media_file.sort_artist_name", "media_file", "sort_artist_name"), + Entry("radio.name", "radio", "name"), + Entry("user.name", "user", "name"), + ) + + DescribeTable("Index collation", + func(table, column string) { + Expect(checkIndexUsage(conn, table, column)).To(Succeed()) + }, + Entry("artist.order_artist_name", "artist", "order_artist_name collate nocase"), + Entry("artist.sort_artist_name", "artist", "coalesce(nullif(sort_artist_name,''),order_artist_name) collate nocase"), + Entry("album.order_album_name", "album", "order_album_name collate nocase"), + Entry("album.order_album_artist_name", "album", "order_album_artist_name collate nocase"), + Entry("album.sort_album_name", "album", "coalesce(nullif(sort_album_name,''),order_album_name) collate nocase"), + Entry("album.sort_album_artist_name", "album", "coalesce(nullif(sort_album_artist_name,''),order_album_artist_name) collate nocase"), + Entry("media_file.order_title", "media_file", "order_title collate nocase"), + Entry("media_file.order_album_name", "media_file", "order_album_name collate nocase"), + Entry("media_file.order_artist_name", "media_file", "order_artist_name collate nocase"), + Entry("media_file.sort_title", "media_file", "coalesce(nullif(sort_title,''),order_title) collate nocase"), + Entry("media_file.sort_album_name", "media_file", "coalesce(nullif(sort_album_name,''),order_album_name) collate nocase"), + Entry("media_file.sort_artist_name", "media_file", "coalesce(nullif(sort_artist_name,''),order_artist_name) collate nocase"), + Entry("media_file.path", "media_file", "path collate nocase"), + Entry("radio.name", "radio", "name collate nocase"), + Entry("user.user_name", "user", "user_name collate nocase"), + ) +}) + +func checkIndexUsage(conn *sql.DB, table string, column string) error { + rows, err := conn.Query(fmt.Sprintf(` +explain query plan select * from %[1]s +where %[2]s = 'test' +order by %[2]s`, table, column)) + if err != nil { + return err + } + defer rows.Close() + + err = rows.Err() + if err != nil { + return err + } + + if rows.Next() { + var dummy int + var detail string + err = rows.Scan(&dummy, &dummy, &dummy, &detail) + if err != nil { + return nil + } + if ok, _ := regexp.MatchString("SEARCH.*USING INDEX", detail); ok { + return nil + } else { + return fmt.Errorf("INDEX for '%s' not used: %s", column, detail) + } + } + return errors.New("no rows returned") +} + +func checkCollation(conn *sql.DB, table string, column string) error { + rows, err := conn.Query(fmt.Sprintf("SELECT sql FROM sqlite_master WHERE type='table' AND tbl_name='%s'", table)) + if err != nil { + return err + } + defer rows.Close() + + err = rows.Err() + if err != nil { + return err + } + + if rows.Next() { + var res string + err = rows.Scan(&res) + if err != nil { + return err + } + re := regexp.MustCompile(fmt.Sprintf(`(?i)\b%s\b.*varchar`, column)) + if !re.MatchString(res) { + return fmt.Errorf("column '%s' not found in table '%s'", column, table) + } + re = regexp.MustCompile(fmt.Sprintf(`(?i)\b%s\b.*collate\s+NOCASE`, column)) + if re.MatchString(res) { + return nil + } + } else { + return fmt.Errorf("table '%s' not found", table) + } + return fmt.Errorf("column '%s' in table '%s' does not have NOCASE collation", column, table) +} diff --git a/persistence/helpers.go b/persistence/helpers.go index e6edf61d6..72ef0abcc 100644 --- a/persistence/helpers.go +++ b/persistence/helpers.go @@ -81,3 +81,13 @@ func (e existsCond) ToSql() (string, []interface{}, error) { } return sql, args, err } + +var sortOrderRegex = regexp.MustCompile(`order_([a-z_]+)`) + +// Convert the order_* columns to an expression using sort_* columns. Example: +// sort_album_name -> (coalesce(nullif(sort_album_name,”),order_album_name) collate nocase) +// It finds order column names anywhere in the substring +func mapSortOrder(order string) string { + order = strings.ToLower(order) + return sortOrderRegex.ReplaceAllString(order, "(coalesce(nullif(sort_$1,''),order_$1) collate nocase)") +} diff --git a/persistence/helpers_test.go b/persistence/helpers_test.go index f080d1e81..3061c7229 100644 --- a/persistence/helpers_test.go +++ b/persistence/helpers_test.go @@ -83,4 +83,23 @@ var _ = Describe("Helpers", func() { Expect(err).To(BeNil()) }) }) + + Describe("mapSortOrder", func() { + It("does not change the sort string if there are no order columns", func() { + sort := "album_name asc" + mapped := mapSortOrder(sort) + Expect(mapped).To(Equal(sort)) + }) + It("changes order columns to sort expression", func() { + sort := "ORDER_ALBUM_NAME asc" + mapped := mapSortOrder(sort) + Expect(mapped).To(Equal("(coalesce(nullif(sort_album_name,''),order_album_name) collate nocase) asc")) + }) + It("changes multiple order columns to sort expressions", func() { + sort := "compilation, order_title asc, order_album_artist_name desc, year desc" + mapped := mapSortOrder(sort) + Expect(mapped).To(Equal(`compilation, (coalesce(nullif(sort_title,''),order_title) collate nocase) asc,` + + ` (coalesce(nullif(sort_album_artist_name,''),order_album_artist_name) collate nocase) desc, year desc`)) + }) + }) }) diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 952e989bc..134b44cbc 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -10,7 +10,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/pocketbase/dbx" @@ -31,25 +30,14 @@ func NewMediaFileRepository(ctx context.Context, db dbx.Builder) *mediaFileRepos "starred": booleanFilter, "genre_id": eqFilter, }) - if conf.Server.PreferSortTags { - r.sortMappings = map[string]string{ - "title": "COALESCE(NULLIF(sort_title,''),order_title)", - "artist": "COALESCE(NULLIF(sort_artist_name,''),order_artist_name) asc, COALESCE(NULLIF(sort_album_name,''),order_album_name) asc, release_date asc, disc_number asc, track_number asc", - "album": "COALESCE(NULLIF(sort_album_name,''),order_album_name) asc, release_date asc, disc_number asc, track_number asc, COALESCE(NULLIF(sort_artist_name,''),order_artist_name) asc, COALESCE(NULLIF(sort_title,''),title) asc", - "random": "random", - "created_at": "media_file.created_at", - "starred_at": "starred, starred_at", - } - } else { - r.sortMappings = map[string]string{ - "title": "order_title", - "artist": "order_artist_name asc, order_album_name asc, release_date asc, disc_number asc, track_number asc", - "album": "order_album_name asc, release_date asc, disc_number asc, track_number asc, order_artist_name asc, title asc", - "random": "random", - "created_at": "media_file.created_at", - "starred_at": "starred, starred_at", - } - } + r.setSortMappings(map[string]string{ + "title": "order_title", + "artist": "order_artist_name, order_album_name, release_date, disc_number, track_number", + "album": "order_album_name, release_date, disc_number, track_number, order_artist_name, title", + "random": "random", + "created_at": "media_file.created_at", + "starred_at": "starred, starred_at", + }) return r } @@ -115,18 +103,6 @@ func (r *mediaFileRepository) GetAll(options ...model.QueryOptions) (model.Media return res, err } -func (r *mediaFileRepository) FindByPath(path string) (*model.MediaFile, error) { - sel := r.newSelect().Columns("*").Where(Like{"path": path}) - var res model.MediaFiles - if err := r.queryAll(sel, &res); err != nil { - return nil, err - } - if len(res) == 0 { - return nil, model.ErrNotFound - } - return &res[0], nil -} - func (r *mediaFileRepository) FindByPaths(paths []string) (model.MediaFiles, error) { sel := r.newSelect().Columns("*").Where(Eq{"path collate nocase": paths}) var res model.MediaFiles diff --git a/persistence/player_repository.go b/persistence/player_repository.go index 98c53d141..d3f13e772 100644 --- a/persistence/player_repository.go +++ b/persistence/player_repository.go @@ -21,9 +21,9 @@ func NewPlayerRepository(ctx context.Context, db dbx.Builder) model.PlayerReposi r.registerModel(&model.Player{}, map[string]filterFunc{ "name": containsFilter("player.name"), }) - r.sortMappings = map[string]string{ + r.setSortMappings(map[string]string{ "user_name": "username", //TODO rename all user_name and userName to username - } + }) return r } diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index f6eca0657..47efff5fe 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -55,9 +55,9 @@ func NewPlaylistRepository(ctx context.Context, db dbx.Builder) model.PlaylistRe "q": playlistFilter, "smart": smartPlaylistFilter, }) - r.sortMappings = map[string]string{ + r.setSortMappings(map[string]string{ "owner_name": "owner_name", - } + }) return r } diff --git a/persistence/playlist_track_repository.go b/persistence/playlist_track_repository.go index 615366cc7..c04dd0f8d 100644 --- a/persistence/playlist_track_repository.go +++ b/persistence/playlist_track_repository.go @@ -5,7 +5,6 @@ import ( . "github.com/Masterminds/squirrel" "github.com/deluan/rest" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/slice" @@ -26,18 +25,13 @@ func (r *playlistRepository) Tracks(playlistId string, refreshSmartPlaylist bool p.db = r.db p.tableName = "playlist_tracks" p.registerModel(&model.PlaylistTrack{}, nil) - p.sortMappings = map[string]string{ + p.setSortMappings(map[string]string{ "id": "playlist_tracks.id", - "artist": "order_artist_name asc", - "album": "order_album_name asc, order_album_artist_name asc", + "artist": "order_artist_name", + "album": "order_album_name, order_album_artist_name", "title": "order_title", "duration": "duration", // To make sure the field will be whitelisted - } - if conf.Server.PreferSortTags { - p.sortMappings["artist"] = "COALESCE(NULLIF(sort_artist_name,''),order_artist_name) asc" - p.sortMappings["album"] = "COALESCE(NULLIF(sort_album_name,''),order_album_name)" - p.sortMappings["title"] = "COALESCE(NULLIF(sort_title,''),title)" - } + }) pls, err := r.Get(playlistId) if err != nil { diff --git a/persistence/radio_repository.go b/persistence/radio_repository.go index 781c50241..a63c1eaf8 100644 --- a/persistence/radio_repository.go +++ b/persistence/radio_repository.go @@ -24,9 +24,6 @@ func NewRadioRepository(ctx context.Context, db dbx.Builder) model.RadioReposito r.registerModel(&model.Radio{}, map[string]filterFunc{ "name": containsFilter("name"), }) - r.sortMappings = map[string]string{ - "name": "(name collate nocase), name", - } return r } diff --git a/persistence/share_repository.go b/persistence/share_repository.go index 6b84243df..692e61403 100644 --- a/persistence/share_repository.go +++ b/persistence/share_repository.go @@ -23,10 +23,10 @@ func NewShareRepository(ctx context.Context, db dbx.Builder) model.ShareReposito r := &shareRepository{} r.ctx = ctx r.db = db - r.registerModel(&model.Share{}, map[string]filterFunc{}) - r.sortMappings = map[string]string{ + r.registerModel(&model.Share{}, nil) + r.setSortMappings(map[string]string{ "username": "username", - } + }) return r } diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index f5c33978b..9b8aefbad 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -27,17 +27,20 @@ import ( // - Call registerModel with the model instance and any possible filters. // - If the model has a different table name than the default (lowercase of the model name), it should be set manually // using the tableName field. -// - Sort mappings should be set in the sortMappings field. If the sort field is not in the map, it will be used as is. +// - Sort mappings must be set with setSortMappings method. If a sort field is not in the map, it will be used as the name of the column. // // All fields in filters and sortMappings must be in snake_case. Only sorts and filters based on real field names or // defined in the mappings will be allowed. type sqlRepository struct { - ctx context.Context - tableName string - db dbx.Builder - sortMappings map[string]string + ctx context.Context + tableName string + db dbx.Builder + + // Do not set these fields manually, they are set by the registerModel method filterMappings map[string]filterFunc isFieldWhiteListed fieldWhiteListedFunc + // Do not set this field manually, it is set by the setSortMappings method + sortMappings map[string]string } const invalidUserId = "-1" @@ -68,6 +71,22 @@ func (r *sqlRepository) registerModel(instance any, filters map[string]filterFun r.filterMappings = filters } +// setSortMappings sets the mappings for the sort fields. If the sort field is not in the map, it will be used as is. +// +// If PreferSortTags is enabled, it will map the order fields to the corresponding sort expression, +// which gives precedence to sort tags. +// Ex: order_title => (coalesce(nullif(sort_title,”),order_title) collate nocase) +// To avoid performance issues, indexes should be created for these sort expressions +func (r *sqlRepository) setSortMappings(mappings map[string]string) { + if conf.Server.PreferSortTags { + for k, v := range mappings { + v = mapSortOrder(v) + mappings[k] = v + } + } + r.sortMappings = mappings +} + func (r sqlRepository) getTableName() string { return r.tableName } diff --git a/scanner/playlist_importer_test.go b/scanner/playlist_importer_test.go index d33e5250d..8b3ae9d5d 100644 --- a/scanner/playlist_importer_test.go +++ b/scanner/playlist_importer_test.go @@ -2,6 +2,7 @@ package scanner import ( "context" + "strconv" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" @@ -70,18 +71,14 @@ type mockedMediaFile struct { model.MediaFileRepository } -func (r *mockedMediaFile) FindByPath(s string) (*model.MediaFile, error) { - return &model.MediaFile{ - ID: "123", - Path: s, - }, nil -} - func (r *mockedMediaFile) FindByPaths(paths []string) (model.MediaFiles, error) { var mfs model.MediaFiles - for _, path := range paths { - mf, _ := r.FindByPath(path) - mfs = append(mfs, *mf) + for i, path := range paths { + mf := model.MediaFile{ + ID: strconv.Itoa(i), + Path: path, + } + mfs = append(mfs, mf) } return mfs, nil }