From 839f1f9a96a93af4f26b1dfc7a747855b0e55221 Mon Sep 17 00:00:00 2001 From: m4bard <304653687+m4bard@users.noreply.github.com> Date: Tue, 14 Jul 2026 14:22:46 -0500 Subject: [PATCH] fix(naming): don't lose a series position that isn't a plain number Audnexus reports a series position as a string, and it is not always a number. An omnibus sits at "1-4" (The Father Brown Collection: Books 1-4), a bundled edition at "1-2" (The Thirty-Nine Steps, which also ships Greenmantle), a prequel at "0" (She and Allan), a novella at "1.5". Two problems followed from squeezing that string through a decimal. Culture. decimal.TryParse was called without a CultureInfo, so it used the server's culture, while FileNamingService formatted the result back with InvariantCulture. The source value always uses '.' as the decimal separator, so under a culture where '.' is the group separator a position of "1.5" parsed as 15, and "0.5" as 5; under fr-FR it did not parse at all. DownloadImportService was worse: it formatted with a bare ToString(), which would write "1,5" -- with a comma -- into a filename. Both parses are now pinned to InvariantCulture and both naming sites format invariantly. Lost positions. A position that failed to parse became null, which is indistinguishable from a book that has no series position. Naming then fell through to the track number and wrote it into the filename as if it were the series number -- so a book at position "1-4" was silently renamed using an unrelated number, with no error and nothing in the log. AudioMetadata now carries SeriesPositionRaw, the position exactly as the source gave it, and naming prefers it over the parsed decimal. The decimal is kept for callers that need to sort or compare. The existing fall back to the track number is deliberate and is preserved for books that genuinely have no series position; the bug was that a real position was being treated as an absent one. Tests cover the culture handling under en-US, de-DE and fr-FR, that a range position survives to the filename rather than being replaced by the track number, and that a book with no position still falls back as before. --- .../Common/FileNamingService.Helpers.cs | 6 +- .../Downloads/Import/DownloadImportService.cs | 14 +- listenarr.domain/Audiobooks/AudioMetadata.cs | 18 ++ listenarr.domain/Audiobooks/Audiobook.cs | 14 +- .../Audiobooks/SeriesPositionReproTests.cs | 159 ++++++++++++++++++ 5 files changed, 204 insertions(+), 7 deletions(-) create mode 100644 tests/Features/Domain/Audiobooks/SeriesPositionReproTests.cs diff --git a/listenarr.application/Common/FileNamingService.Helpers.cs b/listenarr.application/Common/FileNamingService.Helpers.cs index 327616b52..a0156d7ad 100644 --- a/listenarr.application/Common/FileNamingService.Helpers.cs +++ b/listenarr.application/Common/FileNamingService.Helpers.cs @@ -85,7 +85,11 @@ private Dictionary BuildVariables(AudioMetadata metadata) { "Publisher", string.IsNullOrWhiteSpace(metadata.Publisher) ? string.Empty : SanitizePathComponent(metadata.Publisher) }, { "Language", string.IsNullOrWhiteSpace(metadata.Language) ? string.Empty : SanitizePathComponent(metadata.Language) }, { "Asin", string.IsNullOrWhiteSpace(metadata.Asin) ? string.Empty : SanitizePathComponent(metadata.Asin) }, - { "SeriesNumber", FirstNonEmpty(metadata.SeriesPosition?.ToString(CultureInfo.InvariantCulture), metadata.TrackNumber?.ToString()) }, + // Prefer the position exactly as the source gave it. A non-numeric but real + // position (an omnibus at "1-4") does not survive the decimal parse, and + // falling through to TrackNumber here would write a track number into the + // filename as if it were the series number. + { "SeriesNumber", FirstNonEmpty(metadata.SeriesPositionRaw, metadata.SeriesPosition?.ToString(CultureInfo.InvariantCulture), metadata.TrackNumber?.ToString()) }, { "Year", FirstNonEmpty(metadata.Year?.ToString()) }, { "Quality", FirstNonEmpty(metadata.BitRate.HasValue ? metadata.BitRate + "kbps" : null, metadata.Format) }, { "DiskNumber", metadata.DiscNumber?.ToString() ?? string.Empty }, diff --git a/listenarr.application/Downloads/Import/DownloadImportService.cs b/listenarr.application/Downloads/Import/DownloadImportService.cs index d6cc98272..a45ac20db 100644 --- a/listenarr.application/Downloads/Import/DownloadImportService.cs +++ b/listenarr.application/Downloads/Import/DownloadImportService.cs @@ -15,6 +15,7 @@ * You should have received a copy of the GNU Affero General Public License * along with this program. If not, see . */ +using System.Globalization; using Listenarr.Domain.Common; using Microsoft.Extensions.Logging; @@ -230,7 +231,10 @@ public async Task> ImportDownloadFilesAsync( { "Publisher", string.IsNullOrWhiteSpace(namingMetadata.Publisher) ? string.Empty : namingMetadata.Publisher }, { "Language", string.IsNullOrWhiteSpace(namingMetadata.Language) ? string.Empty : namingMetadata.Language }, { "Asin", string.IsNullOrWhiteSpace(namingMetadata.Asin) ? string.Empty : namingMetadata.Asin }, - { "SeriesNumber", namingMetadata.SeriesPosition?.ToString() ?? effectiveChapterNumber?.ToString() ?? string.Empty }, + // As in FileNamingService: prefer the raw position, and format the + // decimal with InvariantCulture -- ToString() under the server's + // culture would put a comma into the filename. + { "SeriesNumber", FirstNonEmpty(namingMetadata.SeriesPositionRaw, namingMetadata.SeriesPosition?.ToString(CultureInfo.InvariantCulture), effectiveChapterNumber?.ToString()) }, { "Year", namingMetadata.Year?.ToString() ?? string.Empty }, { "Quality", (namingMetadata.BitRate.HasValue ? $"{namingMetadata.BitRate}kbps" : null) ?? namingMetadata.Format ?? string.Empty }, { "DiskNumber", effectiveDiskNumber?.ToString() ?? string.Empty }, @@ -368,9 +372,11 @@ private static AudioMetadata BuildNamingMetadata(Audiobook? audiobook, AudioMeta Language = FirstNonEmpty(audiobook.Language, extractedMetadata?.Language), Asin = FirstNonEmpty(audiobook.Asin, extractedMetadata?.Asin), Series = FirstNonEmpty(audiobook.Series, extractedMetadata?.Series), - SeriesPosition = !string.IsNullOrWhiteSpace(audiobook.SeriesNumber) && decimal.TryParse(audiobook.SeriesNumber, out var sp) - ? sp - : (extractedMetadata?.SeriesPosition), + SeriesPosition = !string.IsNullOrWhiteSpace(audiobook.SeriesNumber) + && decimal.TryParse(audiobook.SeriesNumber, NumberStyles.Number, CultureInfo.InvariantCulture, out var sp) + ? sp + : (extractedMetadata?.SeriesPosition), + SeriesPositionRaw = FirstNonEmpty(audiobook.SeriesNumber, extractedMetadata?.SeriesPositionRaw), Year = !string.IsNullOrWhiteSpace(audiobook.PublishYear) && int.TryParse(audiobook.PublishYear, out var year) ? year : extractedMetadata?.Year, diff --git a/listenarr.domain/Audiobooks/AudioMetadata.cs b/listenarr.domain/Audiobooks/AudioMetadata.cs index 3a8ccfc4e..826a8f02b 100644 --- a/listenarr.domain/Audiobooks/AudioMetadata.cs +++ b/listenarr.domain/Audiobooks/AudioMetadata.cs @@ -50,6 +50,22 @@ public class AudioMetadata public string? Language { get; set; } public string? Series { get; set; } public decimal? SeriesPosition { get; set; } + + /// + /// The series position exactly as the metadata source gave it. + /// + /// Audible/Audnexus report a position as a string, and it is not always a number: + /// an omnibus can sit at "1-4", a prequel at "0", a novella at "1.5". Those values + /// are meaningful, but they do not all fit in , so a + /// position that fails to parse would otherwise be indistinguishable from a book + /// that has no series position at all. + /// + /// + /// Naming prefers this over so that a real position is + /// never silently replaced by a track number. + /// + /// + public string? SeriesPositionRaw { get; set; } public byte[]? CoverArt { get; set; } public string? CoverArtUrl { get; set; } public Dictionary AdditionalData { get; set; } = []; @@ -75,6 +91,8 @@ public void Update(AudioMetadata value) if (!SeriesPosition.HasValue && value.SeriesPosition.HasValue) SeriesPosition = value.SeriesPosition; + if (string.IsNullOrWhiteSpace(SeriesPositionRaw) && !string.IsNullOrWhiteSpace(value.SeriesPositionRaw)) + SeriesPositionRaw = value.SeriesPositionRaw; if (!TrackNumber.HasValue && value.TrackNumber.HasValue) TrackNumber = value.TrackNumber; if (!DiscNumber.HasValue && value.DiscNumber.HasValue) diff --git a/listenarr.domain/Audiobooks/Audiobook.cs b/listenarr.domain/Audiobooks/Audiobook.cs index 54c3796f7..36fc93fda 100644 --- a/listenarr.domain/Audiobooks/Audiobook.cs +++ b/listenarr.domain/Audiobooks/Audiobook.cs @@ -17,6 +17,7 @@ */ using System.ComponentModel.DataAnnotations; +using System.Globalization; using Listenarr.Domain.Common; namespace Listenarr.Domain.Audiobooks @@ -104,8 +105,17 @@ public AudioMetadata CreateBasicAudioMetadata() Series = Series, // Prefer audiobook's publish year when available Year = int.TryParse(PublishYear, out var py) ? py : (int?)null, - // Series position / number - SeriesPosition = !string.IsNullOrWhiteSpace(SeriesNumber) && decimal.TryParse(SeriesNumber, out var sp) ? sp : (decimal?)null, + // Series position / number. + // Parsed with InvariantCulture: the source value always uses '.' as the + // decimal separator, so parsing under the server's culture would read a + // position of "1.5" as 15 wherever '.' is the group separator. + // SeriesPositionRaw keeps the original either way -- a position such as + // "1-4" (an omnibus) is real, but is not a decimal. + SeriesPosition = !string.IsNullOrWhiteSpace(SeriesNumber) + && decimal.TryParse(SeriesNumber, NumberStyles.Number, CultureInfo.InvariantCulture, out var sp) + ? sp + : (decimal?)null, + SeriesPositionRaw = !string.IsNullOrWhiteSpace(SeriesNumber) ? SeriesNumber.Trim() : null, // Quality string from audiobook record // Map into Bitrate/Format heuristically if useful; for now store textual quality // We'll put it into AdditionalData so FileNamingService can use Format/Bitrate/Quality diff --git a/tests/Features/Domain/Audiobooks/SeriesPositionReproTests.cs b/tests/Features/Domain/Audiobooks/SeriesPositionReproTests.cs new file mode 100644 index 000000000..efc05c573 --- /dev/null +++ b/tests/Features/Domain/Audiobooks/SeriesPositionReproTests.cs @@ -0,0 +1,159 @@ +/* + * Listenarr - Audiobook Management System + * Copyright (C) 2024-2026 Listenarr Contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published + * by the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +using System.Globalization; +using Listenarr.Application.Common; +using Microsoft.Extensions.Logging; +using Moq; +using Xunit; + +namespace Listenarr.Tests.Features.Domain.Audiobooks +{ + /// + /// Audible/Audnexus report a series position as a STRING, and it is not always a number. + /// Real, live examples from the catalogue: + /// + /// "The Father Brown Collection: Books 1-4" -> position "1-4" (one ASIN, four books) + /// "The Thirty-Nine Steps" -> position "1-2" (bundles its sequel) + /// "She and Allan" -> position "0" (prequel slot) + /// a novella between two books -> position "1.5" + /// + /// Two defects followed from squeezing that string through a decimal: + /// + /// 1. The parse used the server's culture. Where '.' is the group separator (de-DE), + /// "1.5" parses as 15; under fr-FR it does not parse at all. + /// 2. A position that does not parse became null, which is indistinguishable from a + /// book with NO series position -- so naming fell through to the track number and + /// wrote it into the filename as if it were the series number. + /// + public class SeriesPositionReproTests + { + private static Audiobook Book(string? seriesNumber) => new() + { + Title = "Test", + Series = "Test Series", + SeriesNumber = seriesNumber, + }; + + private static FileNamingService Naming() + { + var config = new Mock(); + var logger = new Mock>(); + return new FileNamingService(config.Object, logger.Object); + } + + // --------------------------------------------------------------- + // 1. Culture: the source always uses '.', so parsing must be invariant. + // --------------------------------------------------------------- + + [Theory] + [InlineData("en-US")] + [InlineData("de-DE")] // '.' is the GROUP separator here -- "1.5" once parsed as 15 + [InlineData("fr-FR")] // "1.5" once failed to parse at all + public void DecimalPosition_SurvivesAnyServerCulture(string culture) + { + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo(culture); + var metadata = Book("1.5").CreateBasicAudioMetadata(); + + Assert.Equal(1.5m, metadata.SeriesPosition); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Fact] + public void DecimalPosition_IsWrittenInvariantly_NotWithALocalDecimalComma() + { + var original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + var metadata = Book("1.5").CreateBasicAudioMetadata(); + + var name = Naming().ApplyNamingPattern("{SeriesNumber}", metadata, treatAsFilename: true); + + // Not "1,5" -- a comma in a filename is a locale leaking onto disk. + Assert.Equal("1.5", name); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + // --------------------------------------------------------------- + // 2. A real but non-numeric position must not be lost. + // --------------------------------------------------------------- + + [Theory] + [InlineData("1-4")] // The Father Brown Collection: Books 1-4 + [InlineData("1-2")] // The Thirty-Nine Steps (bundles Greenmantle) + public void RangePosition_IsPreserved_EvenThoughItIsNotADecimal(string position) + { + var metadata = Book(position).CreateBasicAudioMetadata(); + + // decimal? genuinely cannot hold "1-4", and should not try. + Assert.Null(metadata.SeriesPosition); + + // But the value is real and must survive. + Assert.Equal(position, metadata.SeriesPositionRaw); + } + + [Theory] + [InlineData("1-4")] + [InlineData("1-2")] + public void RangePosition_ReachesTheFilename_AndIsNotReplacedByTheTrackNumber(string position) + { + var metadata = Book(position).CreateBasicAudioMetadata(); + metadata.TrackNumber = 7; // the value that used to be written instead + + var name = Naming().ApplyNamingPattern("{SeriesNumber}", metadata, treatAsFilename: true); + + Assert.Equal(position, name); + Assert.NotEqual("7", name); + } + + [Fact] + public void AbsentPosition_StillFallsBackToTheTrackNumber() + { + // The fallback itself is deliberate and must be preserved: a book with no series + // position at all should still get the track number. The bug was that a REAL + // position was being treated as an absent one. + var metadata = Book(null).CreateBasicAudioMetadata(); + metadata.TrackNumber = 7; + + var name = Naming().ApplyNamingPattern("{SeriesNumber}", metadata, treatAsFilename: true); + + Assert.Equal("7", name); + } + + [Fact] + public void ZeroPosition_Survives() + { + // She and Allan sits at position "0" of the Ayesha series. + var metadata = Book("0").CreateBasicAudioMetadata(); + + Assert.Equal(0m, metadata.SeriesPosition); + Assert.Equal("0", metadata.SeriesPositionRaw); + } + } +}