fix(naming): don't lose a series position that isn't a plain number - #763
fix(naming): don't lose a series position that isn't a plain number#763m4bard wants to merge 1 commit into
Conversation
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.
|
Rebased this onto #717 locally. Half of it needs attention sooner than the rebase does. On unmodified #717 head, under the invariant culture a container runs with by default, a book whose series position is "1-4" gets the track number written into its filename: No locale involved. The other half is the culture-dependent parse, which only shows up with Both come back in the new file:
SeriesPosition = !string.IsNullOrWhiteSpace(audiobook.SeriesNumber)
&& decimal.TryParse(audiobook.SeriesNumber, out var seriesPosition)
? seriesPosition
: extractedMetadata?.SeriesPosition,
The port comes to five sites against four on canary. Splitting
The test needed adapting too. I ran the full suite on 379bb9d with and without the port and diffed the failing tests. Identical set either way, so the port adds none. I am not claiming a green suite on your branch. It is one commit on top of 379bb9d if you want to cherry-pick it rather than wait for the rebase: |
1 participant
Summary
Fixes #764.
Audnexus reports a series position as a string, and it is not always a number. Squeezing it through a
decimallost it in two ways, and the loss reached the filename.Real positions from the catalogue (public domain, verifiable against
api.audnex.us):positionB0F84DFZ66"1-4"— one ASIN, four booksB002V1PLZK"1-2"— the product also ships GreenmantleB00CQ5WAXW"0"— a prequel slot"1.5"The parse used the server's culture.
decimal.TryParsewas called with noCultureInfo, whileFileNamingServiceformatted the result back withInvariantCulture. The source always uses.as the decimal separator, so where.is the group separator the number changed:A position that didn't parse was silently discarded. It became
null— indistinguishable from a book with no position — so naming fell through to the track number and wrote that into the filename instead. No error, nothing in the log.Changes
Added
AudioMetadata.SeriesPositionRaw— the series position exactly as the metadata source gave it.SeriesPosition(decimal?) is kept for callers that sort or compare, but it cannot hold"1-4"and no longer pretends to.SeriesPositionReproTests— 10 cases covering culture handling and non-numeric positions.Changed
Audiobook.CreateBasicAudioMetadataandDownloadImportServicenow parse the position withInvariantCulture.FileNamingService.Helpers,DownloadImportService) preferSeriesPositionRaw, then the invariant-formatted decimal, then the track number.DownloadImportServicepreviously formatted with a bare.ToString(), which would write1,5— with a comma — into a filename.Fixed
Testing
SeriesPositionReproTests(10 new cases):en-US,de-DEandfr-FR"1-4"and"1-2"are preserved and reach the filename rather than being replaced by the track number"0"survivesdotnet test: 1199 passing, 0 failing (1189 existing + 10 new). No existing test was changed.Notes
The fall back to the track number is deliberate and is preserved — a book with genuinely no series position still gets it. The bug was that a real position was being treated as an absent one.
Apologies for opening this before the issue; #764 has the write-up. I've read
CONTRIBUTING.mdproperly now.Out of scope but worth flagging while it's in view: a book can hold two series memberships at once. Audnexus gives She and Allan (
B00CQ5WAXW)seriesPrimary= Ayesha #0 andseriesSecondary= Allan Quatermain #7.MetadataConvertersreads both, butAudiobookSeriesMembershipis never constructed outside tests, so neither is persisted. Happy to open that separately if useful.