Skip to content

Fix: approved imports silently delete higher-quality files of the same book #92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebclark wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix: approved imports silently delete higher-quality files of the same book #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 124 additions & 1 deletion src/Chaptarr.Core.Test/MediaFiles/UpgradeMediaFileServiceFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
using NzbDrone.Core.Books.Calibre;
using NzbDrone.Core.MediaFiles;
using NzbDrone.Core.Parser.Model;
using NzbDrone.Core.Profiles.Qualities;
using NzbDrone.Core.Qualities;
using NzbDrone.Core.RootFolders;

namespace Chaptarr.Core.Test.MediaFiles
Expand Down Expand Up @@ -146,7 +148,8 @@ public void should_not_delete_a_loose_path_match_while_replacing_its_stale_row()
DispatchProxy.Create<IDiskProvider, DiskProviderProxy>(),
DispatchProxy.Create<IRootFolderService, RootFolderServiceProxy>(),
DispatchProxy.Create<ICalibreProxy, ThrowingProxy<ICalibreProxy>>(),
LogManager.GetCurrentClassLogger());
LogManager.GetCurrentClassLogger(),
null);

Assert.DoesNotThrow(() => subject.UpgradeBookFile(replacement, localBook));

Expand All @@ -157,5 +160,125 @@ public void should_not_delete_a_loose_path_match_while_replacing_its_stale_row()
Assert.That(mover.Moved, Is.True);
});
}

private class QualityProfileServiceProxy : DispatchProxy
{
public static QualityProfile Profile { get; set; }

protected override object Invoke(MethodInfo targetMethod, object[] args)
{
if (targetMethod?.Name == "Get")
{
return Profile;
}

throw new NotImplementedException($"Test proxy does not implement IQualityProfileService.{targetMethod?.Name}");
}
}

private static QualityProfile AudiobookProfile()
{
return new QualityProfile
{
Id = 7,
Name = "Audiobook",
UpgradeAllowed = true,
Cutoff = Quality.M4B.Id,
Items = new List<QualityProfileQualityItem>
{
new QualityProfileQualityItem { Quality = Quality.MP3, Allowed = true },
new QualityProfileQualityItem { Quality = Quality.M4B, Allowed = true }
}
};
}

private static (UpgradeMediaFileService subject, RecordingRecycleBinProvider bin, MediaFileServiceProxy media, StubBookFileMover mover) BuildGuardedSubject()
{
QualityProfileServiceProxy.Profile = AudiobookProfile();
var recycleBin = new RecordingRecycleBinProvider();
var mediaFileService = DispatchProxy.Create<IMediaFileService, MediaFileServiceProxy>();
var mediaProxy = (MediaFileServiceProxy)(object)mediaFileService;
var mover = new StubBookFileMover();
var subject = new UpgradeMediaFileService(
recycleBin,
mediaFileService,
DispatchProxy.Create<IMetadataTagService, NoOpProxy<IMetadataTagService>>(),
mover,
DispatchProxy.Create<IDiskProvider, DiskProviderProxy>(),
DispatchProxy.Create<IRootFolderService, RootFolderServiceProxy>(),
DispatchProxy.Create<ICalibreProxy, ThrowingProxy<ICalibreProxy>>(),
LogManager.GetCurrentClassLogger(),
DispatchProxy.Create<NzbDrone.Core.Profiles.Qualities.IQualityProfileService, QualityProfileServiceProxy>());
return (subject, recycleBin, mediaProxy, mover);
}

private static LocalBook BuildLocalBook(BookFile existing, string incomingPath)
{
var author = new Author { Id = 1, Path = "/books/Author", AudiobookQualityProfileId = 7 };
var book = new Book
{
Id = 2,
Author = author,
MediaType = BookMediaType.Audiobook,
BookFiles = new List<BookFile> { existing }
};
return new LocalBook { Author = author, Book = book, Path = incomingPath };
}

[Test]
public void should_refuse_to_delete_existing_file_that_outranks_the_incoming_file()
{
var existingM4b = new BookFile
{
Id = 1,
Path = "/books/Author/Book (2003)/Book.m4b",
Quality = new QualityModel(Quality.M4B)
};
var incomingMp3 = new BookFile
{
Id = 2,
Path = "/downloads/Book.mp3",
Quality = new QualityModel(Quality.MP3)
};
var localBook = BuildLocalBook(existingM4b, incomingMp3.Path);
var (subject, bin, media, mover) = BuildGuardedSubject();

Assert.Throws<InvalidOperationException>(() => subject.UpgradeBookFile(incomingMp3, localBook));

Assert.Multiple(() =>
{
Assert.That(bin.DeletedFiles, Is.Empty);
Assert.That(media.Deleted, Is.Empty);
Assert.That(mover.Moved, Is.False);
});
}

[Test]
public void should_still_replace_existing_file_when_incoming_file_is_an_upgrade()
{
var existingMp3 = new BookFile
{
Id = 1,
Path = "/books/Author/Book (2003)/Book.mp3",
Quality = new QualityModel(Quality.MP3)
};
var incomingM4b = new BookFile
{
Id = 2,
Path = "/downloads/Book.m4b",
Quality = new QualityModel(Quality.M4B)
};
var localBook = BuildLocalBook(existingMp3, incomingM4b.Path);
var (subject, bin, media, mover) = BuildGuardedSubject();

Assert.DoesNotThrow(() => subject.UpgradeBookFile(incomingM4b, localBook));

Assert.Multiple(() =>
{
Assert.That(media.Deleted, Is.EqualTo(new[] { existingMp3 }));
Assert.That(mover.Moved, Is.True);
});
}

}
}
29 changes: 28 additions & 1 deletion src/NzbDrone.Core/MediaFiles/UpgradeMediaFileService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
using NzbDrone.Core.Books.Calibre;
using NzbDrone.Core.MediaFiles.BookImport;
using NzbDrone.Core.Parser.Model;
using NzbDrone.Core.Profiles.Qualities;
using NzbDrone.Core.Qualities;
using NzbDrone.Core.RootFolders;

namespace NzbDrone.Core.MediaFiles
Expand All @@ -26,6 +28,7 @@ public class UpgradeMediaFileService : IUpgradeMediaFiles
private readonly IRootFolderService _rootFolderService;
private readonly ICalibreProxy _calibre;
private readonly Logger _logger;
private readonly IQualityProfileService _qualityProfileService;

public UpgradeMediaFileService(IRecycleBinProvider recycleBinProvider,
IMediaFileService mediaFileService,
Expand All @@ -34,7 +37,8 @@ public UpgradeMediaFileService(IRecycleBinProvider recycleBinProvider,
IDiskProvider diskProvider,
IRootFolderService rootFolderService,
ICalibreProxy calibre,
Logger logger)
Logger logger,
IQualityProfileService qualityProfileService)
{
_recycleBinProvider = recycleBinProvider;
_mediaFileService = mediaFileService;
Expand All @@ -44,6 +48,7 @@ public UpgradeMediaFileService(IRecycleBinProvider recycleBinProvider,
_rootFolderService = rootFolderService;
_calibre = calibre;
_logger = logger;
_qualityProfileService = qualityProfileService;
}

public BookFileMoveResult UpgradeBookFile(BookFile bookFile, LocalBook localBook, bool copyOnly = false)
Expand All @@ -53,6 +58,28 @@ public BookFileMoveResult UpgradeBookFile(BookFile bookFile, LocalBook localBook
// Ensure BookFiles collection is loaded
var existingFiles = localBook.Book.BookFiles ?? new List<BookFile>();

// Last-line quality guard: an approved import (e.g. an edition
// switch) must never destroy files that outrank the incoming one
// in the author's profile. Decision specs compare within an
// edition; this is the only place that sees the actual deletion.
var guardProfileId = localBook.Book?.MediaType == Books.BookMediaType.Ebook
? localBook.Author?.EbookQualityProfileId
: localBook.Author?.AudiobookQualityProfileId;
if (guardProfileId > 0 && bookFile.Quality != null && _qualityProfileService != null)
{
var guardProfile = _qualityProfileService.Get(guardProfileId.Value);
if (guardProfile != null)
{
var comparer = new QualityModelComparer(guardProfile);
var betterExisting = existingFiles.FirstOrDefault(f => f.Quality != null && comparer.Compare(f.Quality, bookFile.Quality) > 0);
if (betterExisting != null)
{
throw new System.InvalidOperationException(
$"Refusing to replace '{betterExisting.Path}' ({betterExisting.Quality}) with lower-ranked '{localBook.Path}' ({bookFile.Quality})");
}
}
}

// Handle cases where author path might not be set (e.g., for downloads)
string rootFolderPath;
if (localBook.Author != null && !string.IsNullOrWhiteSpace(localBook.Author.Path))
Expand Down
Loading