Skip to content

Commit e5c3cc0

Browse files
fix(sort): handle empty album_artist in artist sort order
COALESCE(album_artist, artist) treats empty string "" as non-null, so tracks with empty album_artist tags sort by "" instead of falling through to artist. Use NULLIF to convert empty strings to NULL in the SQL, and normalize empty/whitespace-only tag strings to None during metadata extraction to prevent bad data entering the database. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent de44f0d commit e5c3cc0

3 files changed

Lines changed: 118 additions & 40 deletions

File tree

crates/mt-tauri/src/db/models.rs

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -203,15 +203,13 @@ pub struct PaginatedResult<T> {
203203
}
204204

205205
/// Sort order for queries
206-
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
207-
#[derive(Default)]
206+
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
208207
pub enum SortOrder {
209208
Asc,
210209
#[default]
211210
Desc,
212211
}
213212

214-
215213
impl SortOrder {
216214
pub fn as_sql(&self) -> &'static str {
217215
match self {
@@ -222,8 +220,7 @@ impl SortOrder {
222220
}
223221

224222
/// Valid sort columns for library queries
225-
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
226-
#[derive(Default)]
223+
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
227224
pub enum LibrarySortColumn {
228225
Title,
229226
Artist,
@@ -240,9 +237,8 @@ pub enum LibrarySortColumn {
240237
TrackNumber,
241238
}
242239

243-
244240
/// Raw column expression for artist sort (no COLLATE).
245-
const ARTIST_SORT_EXPR: &str = "COALESCE(album_artist, artist)";
241+
const ARTIST_SORT_EXPR: &str = "COALESCE(NULLIF(album_artist, ''), artist)";
246242

247243
impl LibrarySortColumn {
248244
/// Raw column expression without COLLATE or wrapping
@@ -279,7 +275,9 @@ impl LibrarySortColumn {
279275
pub fn as_sql(&self) -> &'static str {
280276
match self {
281277
LibrarySortColumn::Title => "title COLLATE NOCASE",
282-
LibrarySortColumn::Artist => "COALESCE(album_artist, artist) COLLATE NOCASE",
278+
LibrarySortColumn::Artist => {
279+
"COALESCE(NULLIF(album_artist, ''), artist) COLLATE NOCASE"
280+
}
283281
LibrarySortColumn::Album => "album COLLATE NOCASE",
284282
LibrarySortColumn::AddedDate => "added_date",
285283
LibrarySortColumn::PlayCount => "play_count",
@@ -321,11 +319,17 @@ impl LibrarySortColumn {
321319
/// Uses CAST(... AS INTEGER) because track_number and disc_number are TEXT columns.
322320
pub fn secondary_sort_sql(&self) -> &'static str {
323321
match self {
324-
LibrarySortColumn::Artist => ", album COLLATE NOCASE ASC, CAST(disc_number AS INTEGER) ASC, CAST(track_number AS INTEGER) ASC",
325-
LibrarySortColumn::Album => ", COALESCE(album_artist, artist) COLLATE NOCASE ASC, CAST(disc_number AS INTEGER) ASC, CAST(track_number AS INTEGER) ASC",
322+
LibrarySortColumn::Artist => {
323+
", album COLLATE NOCASE ASC, CAST(disc_number AS INTEGER) ASC, CAST(track_number AS INTEGER) ASC"
324+
}
325+
LibrarySortColumn::Album => {
326+
", COALESCE(NULLIF(album_artist, ''), artist) COLLATE NOCASE ASC, CAST(disc_number AS INTEGER) ASC, CAST(track_number AS INTEGER) ASC"
327+
}
326328
LibrarySortColumn::DiscNumber => ", CAST(track_number AS INTEGER) ASC",
327329
LibrarySortColumn::TrackNumber => ", CAST(disc_number AS INTEGER) ASC",
328-
_ => ", COALESCE(album_artist, artist) COLLATE NOCASE ASC, album COLLATE NOCASE ASC, CAST(disc_number AS INTEGER) ASC, CAST(track_number AS INTEGER) ASC",
330+
_ => {
331+
", COALESCE(NULLIF(album_artist, ''), artist) COLLATE NOCASE ASC, album COLLATE NOCASE ASC, CAST(disc_number AS INTEGER) ASC, CAST(track_number AS INTEGER) ASC"
332+
}
329333
}
330334
}
331335

@@ -391,6 +395,34 @@ mod tests {
391395
assert_eq!(deserialized.title, Some("Test Song".to_string()));
392396
}
393397

398+
#[test]
399+
fn test_artist_sort_expr_uses_nullif() {
400+
// Empty album_artist should fall through to artist, not sort as ""
401+
let expr = ARTIST_SORT_EXPR;
402+
assert!(
403+
expr.contains("NULLIF"),
404+
"ARTIST_SORT_EXPR must use NULLIF to handle empty album_artist: {expr}"
405+
);
406+
assert!(
407+
expr.contains("NULLIF(album_artist, '')"),
408+
"ARTIST_SORT_EXPR must convert empty album_artist to NULL: {expr}"
409+
);
410+
}
411+
412+
#[test]
413+
fn test_artist_order_by_with_ignore_words() {
414+
let col = LibrarySortColumn::Artist;
415+
let order_by = col.as_order_by(Some("the, a, an"));
416+
assert!(
417+
order_by.contains("NULLIF(album_artist, '')"),
418+
"Artist ORDER BY must handle empty album_artist: {order_by}"
419+
);
420+
assert!(
421+
order_by.contains("strip_sort_prefix"),
422+
"Artist ORDER BY with ignore_words must use strip_sort_prefix: {order_by}"
423+
);
424+
}
425+
394426
#[test]
395427
fn test_sort_column_from_str() {
396428
use std::str::FromStr;

crates/mt-tauri/src/metadata.rs

Lines changed: 59 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,16 @@ use rayon::prelude::*;
66
use serde::{Deserialize, Serialize};
77
use std::path::Path;
88

9+
/// Convert empty or whitespace-only tag strings to None.
10+
fn non_empty(s: &str) -> Option<String> {
11+
let trimmed = s.trim();
12+
if trimmed.is_empty() {
13+
None
14+
} else {
15+
Some(trimmed.to_string())
16+
}
17+
}
18+
919
#[derive(Debug, Serialize, Deserialize)]
1020
pub struct TrackMetadata {
1121
pub path: String,
@@ -57,7 +67,9 @@ pub fn get_track_metadata(path: String) -> Result<TrackMetadata, String> {
5767
};
5868

5969
if let Some(tagged_file) = tagged_file {
60-
let tag = tagged_file.primary_tag().or_else(|| tagged_file.first_tag());
70+
let tag = tagged_file
71+
.primary_tag()
72+
.or_else(|| tagged_file.first_tag());
6173
let properties = tagged_file.properties();
6274

6375
let format = match tagged_file.file_type() {
@@ -76,23 +88,33 @@ pub fn get_track_metadata(path: String) -> Result<TrackMetadata, String> {
7688
_ => "Unknown",
7789
};
7890

79-
let (title, artist, album, album_artist, track_number, track_total, disc_number, disc_total, year, genre) =
80-
if let Some(tag) = tag {
81-
(
82-
tag.title().map(|s| s.to_string()),
83-
tag.artist().map(|s| s.to_string()),
84-
tag.album().map(|s| s.to_string()),
85-
tag.get_string(&ItemKey::AlbumArtist).map(|s| s.to_string()),
86-
tag.track(),
87-
tag.track_total(),
88-
tag.disk(),
89-
tag.disk_total(),
90-
tag.year(),
91-
tag.genre().map(|s| s.to_string()),
92-
)
93-
} else {
94-
(None, None, None, None, None, None, None, None, None, None)
95-
};
91+
let (
92+
title,
93+
artist,
94+
album,
95+
album_artist,
96+
track_number,
97+
track_total,
98+
disc_number,
99+
disc_total,
100+
year,
101+
genre,
102+
) = if let Some(tag) = tag {
103+
(
104+
tag.title().and_then(|s| non_empty(&s)),
105+
tag.artist().and_then(|s| non_empty(&s)),
106+
tag.album().and_then(|s| non_empty(&s)),
107+
tag.get_string(&ItemKey::AlbumArtist).and_then(non_empty),
108+
tag.track(),
109+
tag.track_total(),
110+
tag.disk(),
111+
tag.disk_total(),
112+
tag.year(),
113+
tag.genre().and_then(|s| non_empty(&s)),
114+
)
115+
} else {
116+
(None, None, None, None, None, None, None, None, None, None)
117+
};
96118

97119
Ok(TrackMetadata {
98120
path,
@@ -115,7 +137,10 @@ pub fn get_track_metadata(path: String) -> Result<TrackMetadata, String> {
115137
} else {
116138
// File exists but tags are unreadable — return empty metadata so the
117139
// editor can still open and the user can write new tags.
118-
tracing::warn!("Could not parse tags for {:?}; returning empty metadata", path);
140+
tracing::warn!(
141+
"Could not parse tags for {:?}; returning empty metadata",
142+
path
143+
);
119144
Ok(TrackMetadata {
120145
path,
121146
title: None,
@@ -467,7 +492,20 @@ mod tests {
467492

468493
#[test]
469494
fn test_supported_formats() {
470-
let formats = ["AAC", "AIFF", "APE", "FLAC", "MP3", "M4A", "MPC", "Opus", "Ogg Vorbis", "Speex", "WAV", "WavPack"];
495+
let formats = [
496+
"AAC",
497+
"AIFF",
498+
"APE",
499+
"FLAC",
500+
"MP3",
501+
"M4A",
502+
"MPC",
503+
"Opus",
504+
"Ogg Vorbis",
505+
"Speex",
506+
"WAV",
507+
"WavPack",
508+
];
471509

472510
for format in formats {
473511
let metadata = TrackMetadata {
@@ -651,9 +689,7 @@ mod tests {
651689

652690
#[test]
653691
fn test_batch_metadata_missing_file_returns_error() {
654-
let result = get_tracks_metadata_batch(vec![
655-
"/nonexistent/file1.mp3".to_string(),
656-
]);
692+
let result = get_tracks_metadata_batch(vec!["/nonexistent/file1.mp3".to_string()]);
657693
assert!(result.is_err());
658694
}
659695

crates/mt-tauri/src/scanner/metadata.rs

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,22 @@ use lofty::prelude::*;
88
use lofty::probe::Probe;
99
use rayon::prelude::*;
1010
use std::path::Path;
11-
use std::sync::atomic::{AtomicUsize, Ordering};
1211
use std::sync::Arc;
12+
use std::sync::atomic::{AtomicUsize, Ordering};
1313

1414
use crate::scanner::fingerprint::FileFingerprint;
1515
use crate::scanner::{ExtractedMetadata, ScanResult};
1616

17+
/// Convert empty or whitespace-only tag strings to None.
18+
fn non_empty(s: &str) -> Option<String> {
19+
let trimmed = s.trim();
20+
if trimmed.is_empty() {
21+
None
22+
} else {
23+
Some(trimmed.to_string())
24+
}
25+
}
26+
1727
/// Extract metadata from a single audio file
1828
pub fn extract_metadata(filepath: &str) -> ScanResult<ExtractedMetadata> {
1929
let path = Path::new(filepath);
@@ -78,10 +88,10 @@ pub fn extract_metadata(filepath: &str) -> ScanResult<ExtractedMetadata> {
7888
.primary_tag()
7989
.or_else(|| tagged_file.first_tag())
8090
{
81-
metadata.title = tag.title().map(|s| s.to_string());
82-
metadata.artist = tag.artist().map(|s| s.to_string());
83-
metadata.album = tag.album().map(|s| s.to_string());
84-
metadata.album_artist = tag.get_string(&ItemKey::AlbumArtist).map(|s| s.to_string());
91+
metadata.title = tag.title().and_then(|s| non_empty(&s));
92+
metadata.artist = tag.artist().and_then(|s| non_empty(&s));
93+
metadata.album = tag.album().and_then(|s| non_empty(&s));
94+
metadata.album_artist = tag.get_string(&ItemKey::AlbumArtist).and_then(non_empty);
8595

8696
// Track number can be in format "1" or "1/10"
8797
metadata.track_number = tag.track().map(|n| n.to_string());
@@ -93,7 +103,7 @@ pub fn extract_metadata(filepath: &str) -> ScanResult<ExtractedMetadata> {
93103
// Year/date
94104
metadata.date = tag.year().map(|y| y.to_string());
95105

96-
metadata.genre = tag.genre().map(|s| s.to_string());
106+
metadata.genre = tag.genre().and_then(|s| non_empty(&s));
97107
}
98108

99109
// Use filename as title if no title found

0 commit comments

Comments
 (0)