diff --git a/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs new file mode 100644 index 00000000..2752032b --- /dev/null +++ b/src/Chaptarr.Core.Test/Books/BookServiceAuthorDeletedFixture.cs @@ -0,0 +1,142 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using NLog; +using NUnit.Framework; +using NzbDrone.Common.Messaging; +using NzbDrone.Core.Books; +using NzbDrone.Core.Books.Events; +using NzbDrone.Core.Messaging.Events; + +namespace Chaptarr.Core.Test.Books +{ + // BookService.Handle(AuthorDeletedEvent) used to always publish BookDeletedEvent with + // deleteFiles hardcoded to false, regardless of what the author-level delete actually + // requested - so MediaFileService never deleted BookFile rows for an author's books, only + // unlinked them, leaving them behind as "unmapped files" even when the physical files really + // were deleted (see MediaFileDeletionServiceFixture's dual-path test for the other half of + // this bug). + [TestFixture] + public class BookServiceAuthorDeletedFixture + { + private sealed class RecordingEventAggregator : IEventAggregator + { + public List Events { get; } = new(); + + public void PublishEvent(TEvent @event) + where TEvent : class, IEvent + { + Events.Add(@event); + } + } + + private class ThrowingProxy : DispatchProxy where T : class + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + throw new NotImplementedException($"Test proxy does not implement {typeof(T).Name}.{targetMethod?.Name}"); + } + } + + private class EmptyLinksSeriesBookLinkRepositoryProxy : DispatchProxy + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(ISeriesBookLinkRepository.GetLinksByBook), StringComparison.Ordinal)) + { + return new List(); + } + + throw new NotImplementedException($"Test proxy does not implement ISeriesBookLinkRepository.{targetMethod?.Name}"); + } + } + + private class RecordingBookRepositoryProxy : DispatchProxy + { + public List BooksByAuthor { get; set; } = new(); + public List DeletedBooks { get; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IBookRepository.GetBooksByAuthorId), StringComparison.Ordinal)) + { + return BooksByAuthor; + } + + if (string.Equals(targetMethod?.Name, "DeleteMany", StringComparison.Ordinal) && + args?.Length == 1 && + args[0] is IEnumerable books) + { + DeletedBooks.AddRange(books); + return null; + } + + throw new NotImplementedException($"Test proxy does not implement IBookRepository.{targetMethod?.Name}"); + } + } + + [Test] + public void should_pass_the_authors_delete_files_flag_through_to_each_books_delete_event() + { + var eventAggregator = new RecordingEventAggregator(); + var bookRepository = DispatchProxy.Create(); + var bookRepoProxy = (RecordingBookRepositoryProxy)(object)bookRepository; + + var author = new Author { Id = 7, Name = "Jim Butcher" }; + var book = new Book { Id = 42, AuthorId = author.Id, Author = author, Title = "Storm Front" }; + bookRepoProxy.BooksByAuthor = new List { book }; + + var service = new BookService( + bookRepository, + null, // editionService + eventAggregator, + null, // authorService + null, // mediaFileService + null, // rootFolderService + DispatchProxy.Create(), + null, // multiCopySeriesService + LogManager.GetCurrentClassLogger()); + + service.Handle(new AuthorDeletedEvent(author, deleteFiles: true, addImportListExclusion: false)); + + var published = eventAggregator.Events.OfType().Single(); + Assert.That(published.DeleteFiles, Is.True); + Assert.That(bookRepoProxy.DeletedBooks.Select(b => b.Id), Does.Contain(book.Id)); + + // MediaFileDeletionService's own AuthorDeletedEvent handler already recursively deletes + // the author's whole folder(s), and NotificationService already sends one OnAuthorDelete + // notification for the whole author - a per-book disk delete or notification here would + // just duplicate both. + Assert.That(published.PartOfAuthorDelete, Is.True); + } + + [Test] + public void should_not_delete_files_when_the_author_delete_did_not_request_it() + { + var eventAggregator = new RecordingEventAggregator(); + var bookRepository = DispatchProxy.Create(); + var bookRepoProxy = (RecordingBookRepositoryProxy)(object)bookRepository; + + var author = new Author { Id = 7, Name = "Jim Butcher" }; + var book = new Book { Id = 42, AuthorId = author.Id, Author = author, Title = "Storm Front" }; + bookRepoProxy.BooksByAuthor = new List { book }; + + var service = new BookService( + bookRepository, + null, // editionService + eventAggregator, + null, // authorService + null, // mediaFileService + null, // rootFolderService + DispatchProxy.Create(), + null, // multiCopySeriesService + LogManager.GetCurrentClassLogger()); + + service.Handle(new AuthorDeletedEvent(author, deleteFiles: false, addImportListExclusion: false)); + + var published = eventAggregator.Events.OfType().Single(); + Assert.That(published.DeleteFiles, Is.False); + } + } +} diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs index 87dd41f5..8bd54c7c 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileDeletionServiceFixture.cs @@ -90,6 +90,69 @@ public RootFolder GetBestRootFolder(string path, List allRootFolders public string GetBestRootFolderPath(string path, List allRootFolders) => throw new NotImplementedException(); } + private class FolderExistsOnlyDiskProviderProxy : DispatchProxy + { + public HashSet ExistingFolders { get; } = new(PathEqualityComparer.Instance); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IDiskProvider.FolderExists), StringComparison.Ordinal) && + args?.Length == 1 && + args[0] is string folderPath) + { + return ExistingFolders.Contains(folderPath); + } + + throw new NotImplementedException($"Test proxy does not implement IDiskProvider.{targetMethod?.Name}"); + } + } + + private class GetFilesByAuthorOnlyMediaFileServiceProxy : DispatchProxy + { + public List Files { get; set; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IMediaFileService.GetFilesByAuthor), StringComparison.Ordinal)) + { + return Files; + } + + throw new NotImplementedException($"Test proxy does not implement IMediaFileService.{targetMethod?.Name}"); + } + } + + private class RecordingCalibreProxy : DispatchProxy + { + public List<(List Books, CalibreSettings Settings)> DeleteBooksCalls { get; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(ICalibreProxy.DeleteBooks), StringComparison.Ordinal)) + { + DeleteBooksCalls.Add(((List)args[0], (CalibreSettings)args[1])); + return null; + } + + throw new NotImplementedException($"Test proxy does not implement ICalibreProxy.{targetMethod?.Name}"); + } + } + + private class AllAuthorPathsOnlyAuthorServiceProxy : DispatchProxy + { + public List> Paths { get; } = new(); + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(IAuthorService.AllAuthorMediaPaths), StringComparison.Ordinal)) + { + return Paths; + } + + throw new NotImplementedException($"Test proxy does not implement IAuthorService.{targetMethod?.Name}"); + } + } + private class ThrowingProxy : DispatchProxy where T : class { protected override object Invoke(MethodInfo targetMethod, object[] args) @@ -385,6 +448,61 @@ public void should_delete_every_file_and_replica_and_remove_the_folder_on_whole_ Assert.That(diskProxy.DeletedFolders, Does.Contain(author.EbookPath)); } + [Test] + public void should_do_no_disk_work_on_book_delete_when_an_author_delete_already_covers_it() + { + // A book delete published as part of a larger author delete (PartOfAuthorDelete) relies + // on MediaFileDeletionService's own AuthorDeletedEvent handler to recursively remove the + // whole author folder. Doing per-file work here too would race that and duplicate + // recycle-bin entries for the same files. + var author = new Author + { + Id = 1, + Name = "Jim Butcher", + Path = "/ebooks/Jim Butcher", + EbookPath = "/ebooks/Jim Butcher" + }; + + var bookFolder = "/ebooks/Jim Butcher/Captains Fury"; + + var diskProvider = DispatchProxy.Create>(); + var recycleBinProvider = new RecordingRecycleBinProvider(); + + var service = new MediaFileDeletionService( + diskProvider, + recycleBinProvider, + DispatchProxy.Create>(), + DispatchProxy.Create>(), + DispatchProxy.Create>(), + new RecordingEventAggregator(), + new StubRootFolderService(new RootFolder { Id = 1, Path = "/ebooks" }), + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + var book = new Book + { + Id = 5792, + AuthorId = author.Id, + Author = author, + Title = "Captain's Fury", + MediaType = BookMediaType.Ebook, + BookFiles = new List + { + new() + { + Id = 1, + Path = bookFolder + "/Captains Fury.epub", + Author = author + } + } + }; + + Assert.DoesNotThrow(() => + service.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, partOfAuthorDelete: true))); + + Assert.That(recycleBinProvider.DeletedFiles, Is.Empty); + } + [Test] public void should_clean_the_book_folder_from_its_parent_so_it_can_actually_be_removed() { @@ -531,6 +649,122 @@ public void should_not_throw_and_should_refuse_deleting_parent_of_root_folder_on Assert.That(eventAggregator.Events.OfType(), Is.Not.Empty); } + [Test] + public void should_delete_both_audiobook_and_ebook_folders_on_author_delete_when_they_differ() + { + // An author's legacy Path only ever pointed at one of AudiobookPath/EbookPath. Deleting + // just Path silently left the other format's entire folder - and every file in it - + // behind on disk, with no error, despite "delete files" having been requested. + var recycleBinProvider = new RecordingRecycleBinProvider(); + var eventAggregator = new RecordingEventAggregator(); + var diskProvider = DispatchProxy.Create(); + var diskProxy = (FolderExistsOnlyDiskProviderProxy)(object)diskProvider; + + var author = new Author + { + Id = 1, + Name = "Jim Butcher", + Path = "/audiobooks/Jim Butcher", + AudiobookPath = "/audiobooks/Jim Butcher", + EbookPath = "/ebooks/Jim Butcher" + }; + + diskProxy.ExistingFolders.Add(author.AudiobookPath); + diskProxy.ExistingFolders.Add(author.EbookPath); + + var rootFolderService = new StubRootFolderService( + new RootFolder { Path = "/audiobooks", FolderType = FolderType.Audiobook }, + new RootFolder { Path = "/ebooks", FolderType = FolderType.Ebook }); + + var service = new MediaFileDeletionService( + diskProvider, + recycleBinProvider, + DispatchProxy.Create>(), + DispatchProxy.Create(), + DispatchProxy.Create>(), + eventAggregator, + rootFolderService, + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + service.HandleAsync(new AuthorDeletedEvent(author, deleteFiles: true, addImportListExclusion: false)); + + Assert.That(recycleBinProvider.DeletedFolders, Does.Contain(author.AudiobookPath)); + Assert.That(recycleBinProvider.DeletedFolders, Does.Contain(author.EbookPath)); + Assert.That(eventAggregator.Events.OfType(), Is.Not.Empty); + } + + [Test] + public void should_route_each_path_through_calibre_or_recycle_bin_independently_when_only_one_format_is_calibre_managed() + { + // Calibre status must be decided per path, not once for the whole author from Path + // alone: a non-Calibre AudiobookPath must never be recycle-binned as if it were part of + // an EbookPath that happens to be Calibre-managed (or vice versa) - either direction + // would either corrupt Calibre's own metadata.db or leave a format's folder untouched. + var recycleBinProvider = new RecordingRecycleBinProvider(); + var eventAggregator = new RecordingEventAggregator(); + var diskProvider = DispatchProxy.Create(); + var diskProxy = (FolderExistsOnlyDiskProviderProxy)(object)diskProvider; + var calibreProxy = DispatchProxy.Create(); + var calibreRecorder = (RecordingCalibreProxy)(object)calibreProxy; + + var author = new Author + { + Id = 1, + Name = "Jim Butcher", + Path = "/audiobooks/Jim Butcher", + AudiobookPath = "/audiobooks/Jim Butcher", + EbookPath = "/calibre-ebooks/Jim Butcher" + }; + + diskProxy.ExistingFolders.Add(author.AudiobookPath); + diskProxy.ExistingFolders.Add(author.EbookPath); + + var calibreSettings = new CalibreSettings(); + var calibreRootFolder = new RootFolder + { + Path = "/calibre-ebooks", + FolderType = FolderType.Ebook, + IsCalibreLibrary = true, + CalibreSettings = calibreSettings + }; + + var rootFolderService = new StubRootFolderService( + new RootFolder { Path = "/audiobooks", FolderType = FolderType.Audiobook }, + calibreRootFolder); + + var ebookFile = new BookFile { Id = 1, Path = author.EbookPath + "/Storm Front.epub" }; + var audiobookFile = new BookFile { Id = 2, Path = author.AudiobookPath + "/Storm Front.m4b" }; + + var mediaFileService = DispatchProxy.Create(); + ((GetFilesByAuthorOnlyMediaFileServiceProxy)(object)mediaFileService).Files = new List { ebookFile, audiobookFile }; + + var service = new MediaFileDeletionService( + diskProvider, + recycleBinProvider, + mediaFileService, + DispatchProxy.Create(), + DispatchProxy.Create>(), + eventAggregator, + rootFolderService, + calibreProxy, + LogManager.GetCurrentClassLogger()); + + var authorDeletedEvent = new AuthorDeletedEvent(author, deleteFiles: true, addImportListExclusion: false); + service.Handle(authorDeletedEvent); + service.HandleAsync(authorDeletedEvent); + + // The plain audiobook folder goes through the recycle bin, and only that folder. + Assert.That(recycleBinProvider.DeletedFolders, Does.Contain(author.AudiobookPath)); + Assert.That(recycleBinProvider.DeletedFolders, Does.Not.Contain(author.EbookPath)); + + // The Calibre-managed ebook folder goes through _calibre.DeleteBooks instead, with only + // the file(s) actually under that path, using that path's own CalibreSettings. + var calibreCall = calibreRecorder.DeleteBooksCalls.Single(); + Assert.That(calibreCall.Settings, Is.SameAs(calibreSettings)); + Assert.That(calibreCall.Books.Select(b => b.Id), Is.EquivalentTo(new[] { ebookFile.Id })); + } + [Test] public void should_delete_managed_ebook_replica_files_on_bookfile_delete_event_even_on_upgrade() { diff --git a/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs b/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs index 71c993d2..2ba2492b 100644 --- a/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs +++ b/src/Chaptarr.Core.Test/MediaFiles/MediaFileServiceDeleteFixture.cs @@ -70,6 +70,8 @@ public int PurgeUnderPath(string pathPrefix) private class MediaFileRepositoryProxy : DispatchProxy { public List DeletedMany { get; } = new List(); + public List DeletedIds { get; } = new List(); + public int DeleteFilesByBookCalls { get; private set; } public List ReplacementAdditions { get; } = new List(); public List ReplacementRemovals { get; } = new List(); public BookFile DeletedSingle { get; private set; } @@ -87,6 +89,12 @@ protected override object Invoke(MethodInfo targetMethod, object[] args) return null; case nameof(IMediaFileRepository.DeleteMany): + if (args[0] is IEnumerable deletedIds) + { + DeletedIds.AddRange(deletedIds); + return null; + } + DeletedMany.Clear(); DeletedMany.AddRange((IEnumerable)args[0]); return null; @@ -108,8 +116,11 @@ protected override object Invoke(MethodInfo targetMethod, object[] args) return null; - case nameof(IMediaFileRepository.UnlinkFilesByBook): case nameof(IMediaFileRepository.DeleteFilesByBook): + DeleteFilesByBookCalls++; + return null; + + case nameof(IMediaFileRepository.UnlinkFilesByBook): return null; case nameof(IMediaFileRepository.GetFilesByEdition): @@ -323,5 +334,75 @@ public void book_delete_should_purge_ingest_queue_for_left_behind_book_files() Assert.That(ingestQueue.PurgedPrefixes, Is.EqualTo(new[] { "/books/books/Test Author/Test Book/Test Book.m4b" })); } + [Test] + public void book_delete_with_delete_files_should_delete_snapshot_rows_that_editions_already_unlinked() + { + var repo = DispatchProxy.Create(); + var repoProxy = (MediaFileRepositoryProxy)(object)repo; + repoProxy.GetByIdsHandler = ids => ids.Select(id => new BookFile { Id = id, EditionId = 0 }); + var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); + + // EditionDeletedEvent has already unlinked these (EditionId = 0) by the time the async handler runs. + var book = new Book + { + Id = 10, + Title = "Test Book", + BookFiles = new List + { + new BookFile { Id = 1, Path = "/books/books/Test Author/Test Book/a.mp3", EditionId = 0 }, + new BookFile { Id = 2, Path = "/books/books/Test Author/Test Book/b.mp3", EditionId = 0 } + } + }; + + sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false)); + + Assert.That(repoProxy.DeletedIds, Is.EquivalentTo(new[] { 1, 2 }), "rows must be deleted by id, not left behind as unmapped"); + Assert.That(repoProxy.DeleteFilesByBookCalls, Is.EqualTo(1), "the by-book delete stays as a backstop"); + } + + [Test] + public void book_delete_with_delete_files_should_not_delete_a_row_relinked_to_another_books_edition() + { + var repo = DispatchProxy.Create(); + var repoProxy = (MediaFileRepositoryProxy)(object)repo; + repoProxy.GetByIdsHandler = ids => ids.Select(id => new BookFile { Id = id, EditionId = id == 2 ? 999 : 0 }); + var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); + + var book = new Book + { + Id = 10, + Title = "Test Book", + Editions = new List { new Edition { Id = 100, BookId = 10 } }, + BookFiles = new List + { + new BookFile { Id = 1, Path = "/books/books/Test Author/Test Book/a.mp3", EditionId = 0 }, + new BookFile { Id = 2, Path = "/books/books/Test Author/Test Book/b.mp3", EditionId = 0 } + } + }; + + sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false)); + + Assert.That(repoProxy.DeletedIds, Is.EqualTo(new[] { 1 }), "row 2 now belongs to edition 999 (another book) and must survive"); + } + + [Test] + public void book_delete_without_delete_files_should_not_delete_any_rows() + { + var repo = DispatchProxy.Create(); + var repoProxy = (MediaFileRepositoryProxy)(object)repo; + var sut = new MediaFileService(repo, new RecordingEventAggregator(), new RecordingIngestQueueRepository(), LogManager.GetLogger("test")); + + var book = new Book + { + Id = 10, + Title = "Test Book", + BookFiles = new List { new BookFile { Id = 1, Path = "/books/books/Test Author/Test Book/a.mp3", EditionId = 0 } } + }; + + sut.HandleAsync(new BookDeletedEvent(book, deleteFiles: false, addImportListExclusion: false)); + + Assert.That(repoProxy.DeletedIds, Is.Empty, "keeping files means keeping their (unmapped) rows"); + Assert.That(repoProxy.DeleteFilesByBookCalls, Is.EqualTo(0), "the keep-files path must not delete by book either"); + } } } diff --git a/src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs b/src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs new file mode 100644 index 00000000..84fb6b69 --- /dev/null +++ b/src/Chaptarr.Core.Test/Notifications/NotificationServiceBookDeletedFixture.cs @@ -0,0 +1,79 @@ +using System; +using System.Collections.Generic; +using System.Reflection; +using NLog; +using NUnit.Framework; +using NzbDrone.Core.Books; +using NzbDrone.Core.Books.Events; +using NzbDrone.Core.Notifications; + +namespace Chaptarr.Core.Test.Notifications +{ + // A book delete published as part of a larger author delete already gets a single + // OnAuthorDelete notification for the whole author. Sending OnBookDelete on top of that for + // every one of its books is the "one notification per episode when the whole series was + // deleted" noise Sonarr/Radarr deliberately don't send. + [TestFixture] + public class NotificationServiceBookDeletedFixture + { + private class ThrowingProxy : DispatchProxy where T : class + { + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + throw new NotImplementedException($"Test proxy does not implement {typeof(T).Name}.{targetMethod?.Name}"); + } + } + + private class EmptyOnBookDeleteNotificationFactoryProxy : DispatchProxy + { + public int OnBookDeleteEnabledCalls { get; private set; } + + protected override object Invoke(MethodInfo targetMethod, object[] args) + { + if (string.Equals(targetMethod?.Name, nameof(INotificationFactory.OnBookDeleteEnabled), StringComparison.Ordinal)) + { + OnBookDeleteEnabledCalls++; + return new List(); + } + + throw new NotImplementedException($"Test proxy does not implement INotificationFactory.{targetMethod?.Name}"); + } + } + + [Test] + public void should_not_look_up_notifications_at_all_when_part_of_an_author_delete() + { + // A throwing INotificationFactory proves OnBookDeleteEnabled is never even called - + // not just that no notification happens to fire. + var service = new NotificationService( + DispatchProxy.Create>(), + DispatchProxy.Create>(), + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + var book = new Book { Id = 1, Author = new Author { Id = 1, Name = "Jim Butcher" } }; + + Assert.DoesNotThrow(() => + service.Handle(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, partOfAuthorDelete: true))); + } + + [Test] + public void should_still_look_up_notifications_for_a_standalone_book_delete() + { + var factory = DispatchProxy.Create(); + var factoryRecorder = (EmptyOnBookDeleteNotificationFactoryProxy)(object)factory; + + var service = new NotificationService( + factory, + DispatchProxy.Create>(), + DispatchProxy.Create>(), + LogManager.GetCurrentClassLogger()); + + var book = new Book { Id = 1, Author = new Author { Id = 1, Name = "Jim Butcher" } }; + + service.Handle(new BookDeletedEvent(book, deleteFiles: true, addImportListExclusion: false, partOfAuthorDelete: false)); + + Assert.That(factoryRecorder.OnBookDeleteEnabledCalls, Is.EqualTo(1)); + } + } +} diff --git a/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs b/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs index 66cd5af5..9b3dc047 100644 --- a/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs +++ b/src/NzbDrone.Core/Books/Events/BookDeletedEvent.cs @@ -12,7 +12,19 @@ public class BookDeletedEvent : IEvent public bool ApplyToBothFormats { get; private set; } public IReadOnlyList DeletedBooks { get; private set; } - public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion, bool applyToBothFormats = false, IEnumerable deletedBooks = null) + // Set when this book is being deleted as part of a larger author delete, whose own + // AuthorDeletedEvent handling already covers what a per-book handler would otherwise do + // again for every one of the author's books: MediaFileDeletionService recursively removes + // the author's whole folder(s) (so per-book/per-file disk cleanup here is redundant at best + // and racy - duplicate recycle-bin entries, deleting files out from under each other - at + // worst), and NotificationService already sends a single OnAuthorDelete notification (so a + // per-book OnBookDelete on top of that is exactly the "one notification per episode when a + // whole series is deleted" noise Sonarr/Radarr deliberately avoid). DeleteFiles still + // independently controls whether MediaFileService deletes vs. unlinks the BookFile DB rows, + // since that's unrelated to who removes physical files or which notification fires. + public bool PartOfAuthorDelete { get; private set; } + + public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion, bool applyToBothFormats = false, IEnumerable deletedBooks = null, bool partOfAuthorDelete = false) { Book = book; DeleteFiles = deleteFiles; @@ -21,6 +33,7 @@ public BookDeletedEvent(Book book, bool deleteFiles, bool addImportListExclusion DeletedBooks = (deletedBooks ?? Enumerable.Repeat(book, 1)) .Where(item => item != null) .ToList(); + PartOfAuthorDelete = partOfAuthorDelete; } } } diff --git a/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs b/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs index 1d9d2613..8c71358b 100644 --- a/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs +++ b/src/NzbDrone.Core/Books/Repositories/AuthorRepository.cs @@ -3,6 +3,7 @@ using System.Linq; using Dapper; using NLog; +using NzbDrone.Common.Disk; using NzbDrone.Common.Extensions; using NzbDrone.Core.Datastore; using NzbDrone.Core.Messaging.Events; @@ -20,6 +21,11 @@ public interface IAuthorRepository : IBasicRepository Author FindByGoogleBooksId(string googleBooksAuthorId); List GetAuthorIdsByMetadataProfileId(int metadataProfileId); Dictionary AllAuthorPaths(); + // The default exists only for lightweight test doubles. Every production implementation must override it. + List> AllAuthorMediaPaths() + { + throw new NotSupportedException(); + } Dictionary> AllAuthorTags(); void UpdateLastSelectedMediaType(int authorId, string mediaType); @@ -110,6 +116,32 @@ public Dictionary AllAuthorPaths() } } + private sealed class AuthorPathRow + { + public int Id { get; set; } + public string Path { get; set; } + public string AudiobookPath { get; set; } + public string EbookPath { get; set; } + } + + // Unlike AllAuthorPaths (legacy single-path field, used by validators/health checks that + // predate dual-format authors), this flattens every author down to all of the paths they + // actually use - Path, AudiobookPath and EbookPath - one row per non-empty path, so a + // consumer can safely check a candidate path against every path any other author owns. + public List> AllAuthorMediaPaths() + { + using (var conn = _database.OpenConnection()) + { + var strSql = "SELECT \"Id\", \"Path\", \"AudiobookPath\", \"EbookPath\" FROM \"Authors\""; + return conn.Query(strSql) + .SelectMany(row => new[] { row.Path, row.AudiobookPath, row.EbookPath } + .Where(p => !string.IsNullOrWhiteSpace(p)) + .Distinct(StringComparer.FromComparison(DiskProviderBase.PathStringComparison)) + .Select(p => new KeyValuePair(row.Id, p))) + .ToList(); + } + } + public Dictionary> AllAuthorTags() { using (var conn = _database.OpenConnection()) diff --git a/src/NzbDrone.Core/Books/Services/AuthorService.cs b/src/NzbDrone.Core/Books/Services/AuthorService.cs index 82443e91..4de8fa11 100644 --- a/src/NzbDrone.Core/Books/Services/AuthorService.cs +++ b/src/NzbDrone.Core/Books/Services/AuthorService.cs @@ -57,6 +57,11 @@ Author UpdateAuthorProgressiveSettings(Author author, int? audiobookQualityProfi } List UpdateAuthors(List authors, bool useExistingRelativeFolder); Dictionary AllAuthorPaths(); + // The default exists only for lightweight test doubles. Every production implementation must override it. + List> AllAuthorMediaPaths() + { + throw new NotSupportedException(); + } bool AuthorPathExists(string folder); void RemoveAddOptions(Author author); void SetMediaTypeMonitoring(int authorId, string mediaType, bool monitored); @@ -594,6 +599,11 @@ public Dictionary AllAuthorPaths() return _authorRepository.AllAuthorPaths(); } + public List> AllAuthorMediaPaths() + { + return _authorRepository.AllAuthorMediaPaths(); + } + public List AllForTag(int tagId) { return GetAllAuthors().Where(s => s.Tags.Contains(tagId)) diff --git a/src/NzbDrone.Core/Books/Services/BookService.cs b/src/NzbDrone.Core/Books/Services/BookService.cs index 39f75f8e..8710ed25 100644 --- a/src/NzbDrone.Core/Books/Services/BookService.cs +++ b/src/NzbDrone.Core/Books/Services/BookService.cs @@ -70,6 +70,15 @@ void UpdateManyWithLifecycle(List books) void ReassignAuthor(List books, int authorId) { throw new NotImplementedException(); } void RefreshProviderAliases(Book book) { } void DeleteMany(List books); + // The default exists only for lightweight test doubles. Kept as an overload (not an added + // parameter on the line above) so the many hand-written IBookService test stubs that only + // implement the 1-arg form keep compiling unchanged. Throws rather than silently forwarding + // to the 1-arg overload (which would drop deleteFiles) - every production implementation + // must override this directly, same as AllAuthorMediaPaths() elsewhere in this change. + void DeleteMany(List books, bool deleteFiles) + { + throw new NotSupportedException(); + } void SetAddOptions(IEnumerable books); List GetAuthorBooksWithFiles(Author author); List GetBooksForDisplay(int? authorId = null, string mediaType = null); @@ -2007,6 +2016,16 @@ private static List GetPersistedBooksForAuthorReassignment(IEnumerable books) + { + DeleteMany(books, false); + } + + public void DeleteMany(List books, bool deleteFiles) + { + DeleteMany(books, deleteFiles, partOfAuthorDelete: false); + } + + private void DeleteMany(List books, bool deleteFiles, bool partOfAuthorDelete) { var booksToDelete = (books ?? new List()) .Where(book => book != null) @@ -2018,7 +2037,7 @@ public void DeleteMany(List books) foreach (var book in booksToDelete) { - _eventAggregator.PublishEvent(new BookDeletedEvent(book, false, false)); + _eventAggregator.PublishEvent(new BookDeletedEvent(book, deleteFiles, false, partOfAuthorDelete: partOfAuthorDelete)); _providerAliasService?.DeleteAliases("Book", book.Id); } @@ -2200,7 +2219,16 @@ public void Handle(AuthorDeletedEvent message) { var books = GetBooksByAuthorId(message.Author.Id); - DeleteMany(books); + // Previously hardcoded to false here regardless of what the author-level delete actually + // requested, so MediaFileService always unlinked (never deleted) each book's BookFile rows - + // leaving them behind as "unmapped files" even when the physical files really were deleted. + // partOfAuthorDelete: true because MediaFileDeletionService's own AuthorDeletedEvent handler + // already recursively deletes the author's whole folder(s) when DeleteFiles is set (a + // per-book disk delete here would just race that and duplicate recycle-bin entries), and + // NotificationService already sends one OnAuthorDelete notification for the whole author - + // a per-book OnBookDelete on top of that is the "notification per episode when the whole + // series was deleted" noise Sonarr/Radarr deliberately don't send. + DeleteMany(books, message.DeleteFiles, partOfAuthorDelete: true); } public void Execute(BulkSyncFormatMonitoringCommand message) diff --git a/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs b/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs index 9ba599c8..fedb00c4 100644 --- a/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs +++ b/src/NzbDrone.Core/Extras/Files/ExtraFileService.cs @@ -119,7 +119,10 @@ public void Handle(BookDeletedEvent message) var authorId = book.AuthorId; - if (message.DeleteFiles) + // PartOfAuthorDelete means this book is being deleted as part of a larger author delete, + // whose own handler already recursively removes the author's whole folder(s) - recycling + // extras individually here too would just race that and duplicate recycle-bin entries. + if (message.DeleteFiles && !message.PartOfAuthorDelete) { var author = book.Author ?? GetAuthorOrNull(authorId); diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs index 35adedf4..b38ee6ad 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileDeletionService.cs @@ -158,6 +158,48 @@ private void DeleteFile(BookFile bookFile, string subfolder = "", RootFolder roo } } + // Shared by both handlers below so a Calibre-routed path gets exactly the same refusal + // checks as a plain recycle-bin path - it used to skip them entirely, which only mattered + // for the single legacy Path but now applies to up to three paths per author. + // allAuthorsCache is fetched lazily, once per method call, only if some path actually needs + // it - an author whose only path(s) are already refused as unsafe should never touch it. + // Uses AllAuthorMediaPaths (Path + AudiobookPath + EbookPath), not the legacy single-path + // AllAuthorPaths - otherwise a dual-format author's AudiobookPath could collide with another + // author's separately-configured EbookPath and never be caught. + private bool ShouldRefuseToDeletePath(string path, Author author, ref List> allAuthorsCache) + { + if (IsPathUnsafeToDelete(path)) + { + _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", + path, author.Name); + return true; + } + + allAuthorsCache ??= _authorService.AllAuthorMediaPaths(); + + foreach (var s in allAuthorsCache) + { + if (s.Key == author.Id) + { + continue; + } + + if (path.IsParentPath(s.Value)) + { + _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", path); + return true; + } + + if (path.PathEquals(s.Value)) + { + _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", path); + return true; + } + } + + return false; + } + [EventHandleOrder(EventHandleOrder.First)] public void Handle(AuthorDeletedEvent message) { @@ -165,14 +207,52 @@ public void Handle(AuthorDeletedEvent message) { var author = message.Author; - var rootFolder = _rootFolderService.GetBestRootFolder(message.Author.Path); - var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; + List allFiles = null; + List> allAuthors = null; - if (isCalibre) + // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) + // in addition to the legacy single Path field, and each one can independently be a + // Calibre library or not. Deriving Calibre status from just one path and applying it + // to the rest is wrong in both directions, so each path here is checked individually. + // ExtraFilePathHelper.GetAuthorBasePaths is the existing helper for exactly this + // {Path, AudiobookPath, EbookPath} dedup - reused here rather than reimplemented. + foreach (var path in ExtraFilePathHelper.GetAuthorBasePaths(author)) { - // use authorId for the query - var books = _mediaFileService.GetFilesByAuthor(author.Id); - _calibre.DeleteBooks(books, rootFolder.CalibreSettings); + var rootFolder = _rootFolderService.GetBestRootFolder(path); + var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; + + if (!isCalibre) + { + continue; + } + + if (ShouldRefuseToDeletePath(path, author, ref allAuthors)) + { + continue; + } + + allFiles ??= _mediaFileService.GetFilesByAuthor(author.Id); + + var booksUnderPath = allFiles + .Where(file => file?.Path != null && path.IsParentPath(file.Path)) + .ToList(); + + if (!booksUnderPath.Any()) + { + continue; + } + + try + { + _calibre.DeleteBooks(booksUnderPath, rootFolder.CalibreSettings); + } + catch (Exception ex) + { + // Don't let one Calibre-managed path's failure (server down, timeout, locked + // metadata.db) stop another distinct Calibre path on this same author from + // being attempted - same isolation as the recycle-bin loop in HandleAsync. + _logger.Error(ex, "Failed to delete Calibre books at '{0}' for author '{1}'.", path, author.Name); + } } } } @@ -183,48 +263,52 @@ public void HandleAsync(AuthorDeletedEvent message) { var author = message.Author; - var rootFolder = _rootFolderService.GetBestRootFolder(message.Author.Path); - var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; + // An author can have separate audiobook/ebook root folders (AudiobookPath, EbookPath) + // in addition to the legacy single Path field. Only ever deleting Path left the other + // format's entire folder - and every file in it - untouched on disk while the DB + // treated the author as fully deleted, leaving those files' BookFile rows to surface + // as "unmapped" even though they were never actually removed. + List> allAuthors = null; - if (!isCalibre) + foreach (var path in ExtraFilePathHelper.GetAuthorBasePaths(author)) { - if (IsPathUnsafeToDelete(author.Path)) + var rootFolder = _rootFolderService.GetBestRootFolder(path); + var isCalibre = rootFolder?.IsCalibreLibrary == true && rootFolder.CalibreSettings != null; + + if (isCalibre) { - _logger.Error("Refusing to delete '{0}' for author '{1}' because it matches or contains a configured root folder. This indicates the author path was misconfigured and deleting would risk data loss.", - author.Path, author.Name); - _eventAggregator.PublishEvent(new DeleteCompletedEvent()); - return; + // Calibre-managed paths are cleaned up via _calibre.DeleteBook(s) in the sync + // Handle() above, not a raw recycle-bin folder delete. + continue; } - var allAuthors = _authorService.AllAuthorPaths(); - - foreach (var s in allAuthors) + if (ShouldRefuseToDeletePath(path, author, ref allAuthors)) { - if (s.Key == author.Id) - { - continue; - } - - if (author.Path.IsParentPath(s.Value)) - { - _logger.Error("Author path: '{0}' is a parent of another author, not deleting files.", author.Path); - return; - } + continue; + } - if (author.Path.PathEquals(s.Value)) + try + { + if (_diskProvider.FolderExists(path)) { - _logger.Error("Author path: '{0}' is the same as another author, not deleting files.", author.Path); - return; + _recycleBinProvider.DeleteFolder(path); } } - - if (_diskProvider.FolderExists(message.Author.Path)) + catch (Exception ex) { - _recycleBinProvider.DeleteFolder(message.Author.Path); + // Don't let one path's failure (permissions, a momentarily-unavailable NFS + // mount, ...) abort the rest of this author's paths or skip the + // DeleteCompletedEvent below - a partially-deleted author still needs its + // Plex refresh queue flushed. + _logger.Error(ex, "Failed to delete '{0}' for author '{1}'.", path, author.Name); } - - _eventAggregator.PublishEvent(new DeleteCompletedEvent()); } + + // Always published once per author now, regardless of which (if any) path above was + // refused/Calibre-routed - the previous single-path version only published this in + // some cases, which could leave Plex's pending-refresh queue never flushed. + // ProcessQueue() is a no-op against an empty queue, so this is safe unconditionally. + _eventAggregator.PublishEvent(new DeleteCompletedEvent()); } } @@ -262,7 +346,7 @@ private bool IsPathUnsafeToDelete(string path) public void HandleAsync(BookDeletedEvent message) { - if (!message.DeleteFiles) + if (!message.DeleteFiles || message.PartOfAuthorDelete) { return; } diff --git a/src/NzbDrone.Core/MediaFiles/MediaFileService.cs b/src/NzbDrone.Core/MediaFiles/MediaFileService.cs index 5a03cff4..9c14d76f 100644 --- a/src/NzbDrone.Core/MediaFiles/MediaFileService.cs +++ b/src/NzbDrone.Core/MediaFiles/MediaFileService.cs @@ -503,6 +503,38 @@ public void HandleAsync(BookDeletedEvent message) if (message.DeleteFiles) { + // A book's editions are deleted synchronously while BookDeletedEvent is being published + // (EditionService), and Handle(EditionDeletedEvent) unlinks their files (EditionId = 0) + // before this async handler runs - so a lookup by book/edition no longer finds them and + // DeleteFilesByBook alone leaves the rows behind as "unmapped" files even though the user + // asked to delete files. Delete the rows captured in the event's snapshot by id, and keep + // the by-book delete for any row linked after the snapshot was taken. + var snapshotIds = (bookFiles ?? new List()) + .Where(file => file != null && file.Id > 0) + .Select(file => file.Id) + .Distinct() + .ToList(); + + if (snapshotIds.Any()) + { + // Re-read the rows and only delete ones that are still unlinked or still belong to this + // book's own editions: an id in the snapshot that was re-linked to another book's edition + // in the meantime (concurrent import/replace) must never be deleted here. + var ownEditionIds = (message.Book.Editions ?? new List()) + .Select(edition => edition.Id) + .ToHashSet(); + + var deletableIds = (_mediaFileRepository.Get(snapshotIds) ?? Enumerable.Empty()) + .Where(file => file.EditionId == 0 || ownEditionIds.Contains(file.EditionId)) + .Select(file => file.Id) + .ToList(); + + if (deletableIds.Any()) + { + _mediaFileRepository.DeleteMany(deletableIds); + } + } + _mediaFileRepository.DeleteFilesByBook(message.Book.Id); } else diff --git a/src/NzbDrone.Core/Notifications/NotificationService.cs b/src/NzbDrone.Core/Notifications/NotificationService.cs index 4616921c..8ba913f0 100644 --- a/src/NzbDrone.Core/Notifications/NotificationService.cs +++ b/src/NzbDrone.Core/Notifications/NotificationService.cs @@ -339,6 +339,15 @@ public void Handle(AuthorDeletedEvent message) public void Handle(BookDeletedEvent message) { + // A book delete published as part of a larger author delete already gets a single + // OnAuthorDelete notification for the whole author - sending OnBookDelete on top of that + // for every one of its books is the "one notification per episode when a whole series is + // deleted" noise Sonarr/Radarr deliberately don't send. + if (message.PartOfAuthorDelete) + { + return; + } + var deleteMessage = new BookDeleteMessage(message.Book, message.DeleteFiles); foreach (var notification in _notificationFactory.OnBookDeleteEnabled())