Import album extras #4370

tty418 · 2023-12-15T21:24:01Z

Database Migration

YES - Constraint removed from ExtraFiles table to enable for album-wide extras to be inserted.

Motivation

Music collections often include additional extra files for each album such as metadata (.nfo's, EAC .log files, .txt files etc), album art images, as well as .cue sheets. Currently, these files get lost during import - both during manual import in-place, as well as from the download client.

My goal was for this feature to work seamlessly, not break existing flows, and be on by default. The lack of album extras 'surprises' quite a few people, and often the revelation comes a bit late (e.g. after processing half my existing music collection),

Description

This PR adds handling for extra files which are specific to a whole album and not a single track. The extra files themselves will not be renamed and will be kept as-is during import.

These 'album extras' will be handled only if renaming is enabled. Another requirement is that an album directory is present in the track naming template. If these two conditions are not satisfied, importing the album extras will not be a safe operation due to conflicts with extras from other albums.

Implementation details (relevant for the PR)

The current extra file handling is geared towards per-track extras. Adding support for album extras is impossible without having to touch a ton of working code and redefine interfaces. The new feature has a separate implementation as it follows completely different logic. It is easier to follow and won't break anything.

The list of relevant file extensions is temporarily hardcoded, as I needed to get the PR out of the door. This feature needs separate configuration, as the existing ExtraFileExtensions and ImportExtraFiles config values are targeted at per-track extras. Maybe they can be reused, this will save me some changes on the frontend side.
Currently the list of file extensions which will be handled includes: .cue, .jpg, .nfo, .log, .txt. The plan is to make this configurable in a follow-up PR . Or use the existing setting (up for discussion).

Implementation todo's (draft PR):

Test migration against PostgreS
Check file naming configuration for the presence of an album dir
Retest with downloads and manual imports after refactoring
Test rename (move)
Avoid comparing source/destination paths?

Todos

Tests
Translation Keys (./src/NzbDrone.Core/Localization/Core/en.json)
Wiki Updates

Issues Fixed or Closed by this PR

Fixes Lidarr not moving extra files #484

tty418 · 2023-12-20T22:58:19Z

src/NzbDrone.Core/Extras/Files/AlbumExtraFileManager.cs

+            var transferMode = _configService.CopyUsingHardlinks ? TransferMode.HardLinkOrCopy : TransferMode.Copy;
+
+            // TODO: check for readonly / downloaditem etc, adjust transferMode
+            if (extraFilePath != newFileName)


I would like to avoid this check, but if source and destination are the same, TransferFile() will throw an exception.

Changed this comparison to work on two DirectoryInfo instances, not perfect but it covers more cases :)

src/NzbDrone.Core/Datastore/Migration/075_relax_not_null_constraints_extra_files.cs

tty418 · 2024-02-01T12:26:21Z

@mynameisbogdan (or anyone reviewing): as a temporary measure I'm considering adding an option to configure the list of supported extensions for the album extras. The mid-term plan is, as before, to add configuration through the UI in a follow-up PR. But until then I could enable users or people willing to test the feature specify which extensions should be handled.

The easiest thing to do would be to read an environment variable, and then override the hardcoded list:
if (Environment.GetEnvironmentVariable("LIDARR_EXPERIMENTAL_EXTRA_FILE_EXTENSIONS") != null) { ... }
Or use the configuration provider and inject IOptions<LidarrExperimentalFoo> - cleaner, but more involved and probably unnecessary for a temporary setting.

This would resolve the issue mentioned here: #484 (comment)

src/NzbDrone.Core/Datastore/Migration/076_relax_not_null_constraints_extra_files.cs

Qstick · 2024-02-04T21:26:53Z

src/NzbDrone.Core/Extras/ExtraService.cs

+            var filtered = albumFiles.Where(x => !importedTracks.Select(t => t.Item.Path).Contains(x));
+
+            // TODO: grab extensions from config?
+            var extensions = new List<string> { ".cue", ".jpg", ".png", ".nfo", ".log", ".txt" };


_configService.ExtraFileExtensions

or a new setting if we think it's needed (existing for Artist and new for Album extensions)

I have two questions regarding this suggestion:

The default value currently is srt, which is probably coming from the TV Arrs.
-> Shall we define a list of suitable extensions, e.g. the ones I did my tests with and hardcoded above?

The setting is currently off by default.
-> I was hoping that this feature can be instead on / opt out.

Especially wrt the second point - the impact of downloading dozens of albums and finding out afterwards that none of the usual extra files (nfos, covers, cue's) are there is huge. I literally had to stop my noise file acquisition for weeks until I figured out what's happening and came up with this PR. I'm still slowly going through my spreadsheet and rebuilding artist by artist.

With that said, I assumed that this should be a different setting, which does not seem to make sense. Putting this under the umbrella of "Extra Files" is fine.

Yes, let's change the default to something that makes sense. SRT doesn't (I'm actually surprised it's gone this long)

I'm not opposed to on by default if we think that makes sense for music.

Alright, I made the changes. The defaults are as following:

Import extra files on by default;

Extra file extensions: log, cue, nfo, jpg, jpeg, png

Regarding sensible defaults for the extensions:

Must-haves are .log and .cue: highly valued in at least one particular music community :)

Nice to haves: .nfo (release info)

Maybe: .pdf and .txt (release-specific, some include such extra files or docs)

Whatever: cover files (so your jpg's, png's etc), this is recoverable information

I could add all of these, but I didn't want to make the list too long. Let me know what you think.

src/NzbDrone.Core/Extras/ExtraService.cs

src/NzbDrone.Core/Extras/Files/AlbumExtraFileManager.cs

src/NzbDrone.Core/Datastore/Migration/076_relax_not_null_constraints_extra_files.cs

Qstick · 2024-02-06T00:12:25Z

src/NzbDrone.Core/Datastore/Migration/078_relax_not_null_constraints_extra_files.cs

+    {
+        protected override void MainDbUpgrade()
+        {
+            Alter.Table("ExtraFiles").AlterColumn("TrackFileId").AsInt64().Nullable();


Is the change from int32 to int64 intentional?

Not really, I did it on autopilot. Reverted to Int32
Underlying column types are, according to the SQLite documentation, int64 already.

tty418 · 2024-02-28T13:26:03Z

It turned out that extra file imports were handled only in the simplest case. Multi-CD releases and basically anything different than a single directory for all files resulted in files not being found, or being moved to the incorrect target directory on import.

All problems should be solved now. At the price of quite a bit of path wrangling and extra route matching code. I tried to keep things simple and readable, with the occasional comment here and there to explain why something is done in a particular way.

The tests, both setup and the way I organized them, also needed lots of adjustments. I am happy with the end result, but faking and mocking a file system with recursive file search hurts. But without the tests I would've never made it.

The unit tests cover all supported directory structure variants:

tracks at root, extras at any dir under root (search recursively)
tracks in a single subdir, extras only under subdir (otherwise its unsafe)
tracks in multiple subdirs (CD1, CD2 etc), extras at root or any subdir (recursively)

@mynameisbogdan @Qstick
TL DR:

large parts of the PR were rewritten
importing downloads and manually / in place works:
- High confidence that it works well on Windows / while debugging with VS
- Cross-platform tests caught a few issues early on
- As soon as a Docker Image is ready I'll quickly test on linux

And sorry for the massive amount of if statements + for-loops. Searching for common parent dir and extra handling for files under track dirs is necessary to keep audio files and extras together after import/rename. :(

mynameisbogdan · 2024-03-04T13:49:54Z

FYI just did a rebase in #4526

tty418 · 2024-03-07T09:40:07Z

FYI just did a rebase in #4526

I rebased develop and fixed the issue reported by Sonar. I also used the opportunity to extract some of the file matching code:
https://github.com/Lidarr/Lidarr/pull/4370/files/59b984f621f72a5516c541a61bb76740ebdacee2..9ae4b1fb16e7e81aa57c18d4fa2b1dd44f1d6bbe

Let me know if the code is hard to follow somewhere, or if you find issues in the PR :)

mynameisbogdan · 2024-03-07T09:42:16Z

can you squash 36 commits into a single one as well?

tty418 · 2024-03-07T19:54:10Z

can you squash 36 commits into a single one as well?

Squashed 👍

tty418 changed the title ~~Feature/import album extras~~ Import album extras Dec 15, 2023

github-actions bot added the Area: Db-migration Warning: DB Migration | Related to DB Migration label Dec 15, 2023

tty418 marked this pull request as ready for review December 20, 2023 22:56

tty418 commented Dec 20, 2023

View reviewed changes

mynameisbogdan reviewed Dec 24, 2023

View reviewed changes

src/NzbDrone.Core/Datastore/Migration/075_relax_not_null_constraints_extra_files.cs Outdated Show resolved Hide resolved

tty418 force-pushed the feature/import_album_extras branch from dcc0f3e to 94447a5 Compare December 29, 2023 21:03

tty418 force-pushed the feature/import_album_extras branch from 94447a5 to e29e479 Compare January 27, 2024 23:19

Qstick reviewed Feb 4, 2024

View reviewed changes

src/NzbDrone.Core/Extras/ExtraService.cs Outdated Show resolved Hide resolved

src/NzbDrone.Core/Extras/Files/AlbumExtraFileManager.cs Outdated Show resolved Hide resolved

mynameisbogdan requested changes Feb 5, 2024

View reviewed changes

src/NzbDrone.Core/Datastore/Migration/076_relax_not_null_constraints_extra_files.cs Outdated Show resolved Hide resolved

Qstick reviewed Feb 6, 2024

View reviewed changes

tty418 force-pushed the feature/import_album_extras branch from 077f22c to 483d85b Compare February 6, 2024 21:58

tty418 force-pushed the feature/import_album_extras branch from 483d85b to 4a607c4 Compare February 23, 2024 08:49

tty418 force-pushed the feature/import_album_extras branch from 4390567 to b9e0709 Compare March 7, 2024 09:33

tty418 force-pushed the feature/import_album_extras branch from 9ae4b1f to c230d6e Compare March 7, 2024 19:53

tty418 force-pushed the feature/import_album_extras branch from c230d6e to d62734a Compare March 7, 2024 20:01

Import album extras for manual imports and downloads

f344e60

tty418 force-pushed the feature/import_album_extras branch from d62734a to f344e60 Compare April 18, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import album extras #4370

Import album extras #4370

tty418 commented Dec 15, 2023 •

edited

tty418 Dec 20, 2023

tty418 Jan 27, 2024

tty418 commented Feb 1, 2024

Qstick Feb 4, 2024

tty418 Feb 5, 2024

Qstick Feb 6, 2024

tty418 Feb 6, 2024

Qstick Feb 6, 2024

tty418 Feb 6, 2024

tty418 commented Feb 28, 2024 •

edited

mynameisbogdan commented Mar 4, 2024

tty418 commented Mar 7, 2024

mynameisbogdan commented Mar 7, 2024 •

edited

tty418 commented Mar 7, 2024

Import album extras #4370

Are you sure you want to change the base?

Import album extras #4370

Conversation

tty418 commented Dec 15, 2023 • edited

Database Migration

Motivation

Description

Implementation details (relevant for the PR)

Implementation todo's (draft PR):

Todos

Issues Fixed or Closed by this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tty418 commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tty418 commented Feb 28, 2024 • edited

mynameisbogdan commented Mar 4, 2024

tty418 commented Mar 7, 2024

mynameisbogdan commented Mar 7, 2024 • edited

tty418 commented Mar 7, 2024

tty418 commented Dec 15, 2023 •

edited

tty418 commented Feb 28, 2024 •

edited

mynameisbogdan commented Mar 7, 2024 •

edited