From 0b7a42ee3b6fe9eb64fb937891a089f946bbcab7 Mon Sep 17 00:00:00 2001 From: ta264 Date: Tue, 9 Jul 2019 21:14:05 +0100 Subject: [PATCH] New: Refactor metadata update --- .../IdentificationServiceFixture.cs | 3 +- .../MusicTests/RefreshAlbumServiceFixture.cs | 218 ++++++---- .../MusicTests/RefreshArtistServiceFixture.cs | 206 +++++++-- .../NzbDrone.Core.Test.csproj | 1 - src/NzbDrone.Core/History/HistoryService.cs | 6 + .../SkyHook/Resource/ArtistResource.cs | 7 +- .../MetadataSource/SkyHook/SkyHookProxy.cs | 8 +- src/NzbDrone.Core/Music/AddAlbumService.cs | 160 ------- src/NzbDrone.Core/Music/AddArtistService.cs | 15 + src/NzbDrone.Core/Music/Album.cs | 71 +++- .../Music/ArtistMetadataRepository.cs | 53 ++- src/NzbDrone.Core/Music/ArtistService.cs | 11 +- .../Music/RefreshAlbumReleaseService.cs | 138 ++++++ .../Music/RefreshAlbumService.cs | 402 +++++++++++------- .../Music/RefreshArtistService.cs | 259 ++++++----- .../Music/RefreshEntityServiceBase.cs | 277 ++++++++++++ .../Music/RefreshTrackService.cs | 125 ++---- src/NzbDrone.Core/Music/Track.cs | 2 +- src/NzbDrone.Core/NzbDrone.Core.csproj | 3 +- 19 files changed, 1294 insertions(+), 671 deletions(-) delete mode 100644 src/NzbDrone.Core/Music/AddAlbumService.cs create mode 100644 src/NzbDrone.Core/Music/RefreshAlbumReleaseService.cs create mode 100644 src/NzbDrone.Core/Music/RefreshEntityServiceBase.cs diff --git a/src/NzbDrone.Core.Test/MediaFiles/TrackImport/Identification/IdentificationServiceFixture.cs b/src/NzbDrone.Core.Test/MediaFiles/TrackImport/Identification/IdentificationServiceFixture.cs index 4b17f6541..2698946ae 100644 --- a/src/NzbDrone.Core.Test/MediaFiles/TrackImport/Identification/IdentificationServiceFixture.cs +++ b/src/NzbDrone.Core.Test/MediaFiles/TrackImport/Identification/IdentificationServiceFixture.cs @@ -61,7 +61,8 @@ namespace NzbDrone.Core.Test.MediaFiles.TrackImport.Identification _addArtistService = Mocker.Resolve(); Mocker.SetConstant(Mocker.Resolve()); - Mocker.SetConstant(Mocker.Resolve()); + Mocker.SetConstant(Mocker.Resolve()); + Mocker.SetConstant(Mocker.Resolve()); _refreshArtistService = Mocker.Resolve(); Mocker.GetMock().Setup(x => x.Validate(It.IsAny())).Returns(new ValidationResult()); diff --git a/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs b/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs index 6161c4d5b..00a0d97ab 100644 --- a/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs +++ b/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs @@ -13,6 +13,8 @@ using NzbDrone.Core.Music.Commands; using NzbDrone.Test.Common; using FluentAssertions; using NzbDrone.Common.Serializer; +using NzbDrone.Core.MediaFiles; +using NzbDrone.Core.History; namespace NzbDrone.Core.Test.MusicTests { @@ -40,6 +42,7 @@ namespace NzbDrone.Core.Test.MusicTests _releases = new List { release }; var album1 = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) .With(s => s.Id = 1234) .With(s => s.ForeignAlbumId = "1") .With(s => s.AlbumReleases = _releases) @@ -63,6 +66,10 @@ namespace NzbDrone.Core.Test.MusicTests .Setup(s => s.FindById(It.IsAny>())) .Returns(new List()); + Mocker.GetMock() + .Setup(s => s.UpsertMany(It.IsAny >())) + .Returns(true); + Mocker.GetMock() .Setup(s => s.GetAlbumInfo(It.IsAny())) .Callback(() => { throw new AlbumNotFoundException(album1.ForeignAlbumId); }); @@ -70,6 +77,18 @@ namespace NzbDrone.Core.Test.MusicTests Mocker.GetMock() .Setup(s => s.ShouldRefresh(It.IsAny())) .Returns(true); + + Mocker.GetMock() + .Setup(x => x.GetFilesByAlbum(It.IsAny())) + .Returns(new List()); + + Mocker.GetMock() + .Setup(x => x.GetFilesByRelease(It.IsAny())) + .Returns(new List()); + + Mocker.GetMock() + .Setup(x => x.GetByAlbum(It.IsAny(), It.IsAny())) + .Returns(new List()); } private void GivenNewAlbumInfo(Album album) @@ -80,27 +99,69 @@ namespace NzbDrone.Core.Test.MusicTests } [Test] - public void should_log_error_if_musicbrainz_id_not_found() + public void should_update_if_musicbrainz_id_changed_and_no_clash() { - Subject.RefreshAlbumInfo(_albums, false, false); + var newAlbumInfo = _albums.First().JsonClone(); + newAlbumInfo.ArtistMetadata = _albums.First().ArtistMetadata.Value.JsonClone(); + newAlbumInfo.ForeignAlbumId = _albums.First().ForeignAlbumId + 1; + newAlbumInfo.AlbumReleases = _releases; - Mocker.GetMock() - .Verify(v => v.UpdateMany(It.IsAny>()), Times.Never()); + GivenNewAlbumInfo(newAlbumInfo); + + Subject.RefreshAlbumInfo(_albums, null, false, false); - ExceptionVerification.ExpectedErrors(1); + Mocker.GetMock() + .Verify(v => v.UpdateMany(It.Is>(s => s.First().ForeignAlbumId == newAlbumInfo.ForeignAlbumId))); } [Test] - public void should_update_if_musicbrainz_id_changed() + public void should_merge_if_musicbrainz_id_changed_and_new_already_exists() { - var newAlbumInfo = _albums.FirstOrDefault().JsonClone(); + var existing = _albums.First(); + + var clash = existing.JsonClone(); + clash.Id = 100; + clash.ArtistMetadata = existing.ArtistMetadata.Value.JsonClone(); + clash.ForeignAlbumId = clash.ForeignAlbumId + 1; + + clash.AlbumReleases = Builder.CreateListOfSize(10) + .All().With(x => x.AlbumId = clash.Id) + .BuildList(); + + Mocker.GetMock() + .Setup(x => x.FindById(clash.ForeignAlbumId)) + .Returns(clash); + + Mocker.GetMock() + .Setup(x => x.GetReleasesByAlbum(_albums.First().Id)) + .Returns(_releases); + + Mocker.GetMock() + .Setup(x => x.GetReleasesByAlbum(clash.Id)) + .Returns(new List()); + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(It.IsAny(), It.IsAny>())) + .Returns(_releases); + + var newAlbumInfo = existing.JsonClone(); + newAlbumInfo.ArtistMetadata = existing.ArtistMetadata.Value.JsonClone(); newAlbumInfo.ForeignAlbumId = _albums.First().ForeignAlbumId + 1; newAlbumInfo.AlbumReleases = _releases; GivenNewAlbumInfo(newAlbumInfo); - Subject.RefreshAlbumInfo(_albums, false, false); + Subject.RefreshAlbumInfo(_albums, null, false, false); + + // check releases moved to clashing album + Mocker.GetMock() + .Verify(v => v.UpdateMany(It.Is>(x => x.All(y => y.AlbumId == clash.Id) && x.Count == _releases.Count))); + + // check old album is deleted + Mocker.GetMock() + .Verify(v => v.DeleteMany(It.Is>(x => x.First().ForeignAlbumId == existing.ForeignAlbumId))); + // check that clash gets updated Mocker.GetMock() .Verify(v => v.UpdateMany(It.Is>(s => s.First().ForeignAlbumId == newAlbumInfo.ForeignAlbumId))); @@ -115,11 +176,23 @@ namespace NzbDrone.Core.Test.MusicTests GivenNewAlbumInfo(album); - Subject.RefreshAlbumInfo(album, false); + Subject.RefreshAlbumInfo(album, null, false); Mocker.GetMock() - .Verify(x => x.DeleteMany(It.Is>(y => y.Count == 1 && y.First().ForeignAlbumId == album.ForeignAlbumId)), + .Verify(x => x.DeleteAlbum(album.Id, true), Times.Once()); + + ExceptionVerification.ExpectedWarns(1); + } + + [Test] + public void two_equivalent_albums_should_be_equal() + { + var album = Builder.CreateNew().Build(); + var album2 = Builder.CreateNew().Build(); + + ReferenceEquals(album, album2).Should().BeFalse(); + album.Equals(album2).Should().BeTrue(); } [Test] @@ -160,9 +233,13 @@ namespace NzbDrone.Core.Test.MusicTests [Test] public void should_not_add_duplicate_releases() { - var newAlbum = Builder.CreateNew().Build(); + var newAlbum = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); // this is required because RefreshAlbumInfo will edit the album passed in - var albumCopy = Builder.CreateNew().Build(); + var albumCopy = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); var releases = Builder.CreateListOfSize(10) .All() @@ -191,21 +268,13 @@ namespace NzbDrone.Core.Test.MusicTests .Setup(x => x.GetAlbumInfo(It.IsAny())) .Returns(Tuple.Create("dummy string", albumCopy, new List())); - Subject.RefreshAlbumInfo(newAlbum, false); + Subject.RefreshAlbumInfo(newAlbum, null, false); - newAlbum.AlbumReleases.Value.Should().HaveCount(7); - - Mocker.GetMock() - .Verify(x => x.DeleteMany(It.Is>(l => l.Count == 0)), Times.Once()); - - Mocker.GetMock() - .Verify(x => x.UpdateMany(It.Is>(l => l.Count == 1 && l.Select(r => r.ForeignReleaseId).Distinct().Count() == 1)), Times.Once()); - - Mocker.GetMock() - .Verify(x => x.InsertMany(It.Is>(l => l.Count == 6 && - l.Select(r => r.ForeignReleaseId).Distinct().Count() == l.Count && - !l.Select(r => r.ForeignReleaseId).Contains("DuplicateId2"))), - Times.Once()); + Mocker.GetMock() + .Verify(x => x.RefreshEntityInfo(It.Is>(l => l.Count == 7 && l.Count(y => y.Monitored) == 1), + It.IsAny>(), + It.IsAny(), + It.IsAny())); } [TestCase(true, true, 1)] @@ -214,9 +283,13 @@ namespace NzbDrone.Core.Test.MusicTests [TestCase(false, false, 0)] public void should_only_leave_one_release_monitored(bool skyhookMonitored, bool existingMonitored, int expectedUpdates) { - var newAlbum = Builder.CreateNew().Build(); + var newAlbum = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); // this is required because RefreshAlbumInfo will edit the album passed in - var albumCopy = Builder.CreateNew().Build(); + var albumCopy = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); var releases = Builder.CreateListOfSize(10) .All() @@ -248,31 +321,26 @@ namespace NzbDrone.Core.Test.MusicTests .Setup(x => x.GetAlbumInfo(It.IsAny())) .Returns(Tuple.Create("dummy string", albumCopy, new List())); - Subject.RefreshAlbumInfo(newAlbum, false); - - newAlbum.AlbumReleases.Value.Should().HaveCount(10); - newAlbum.AlbumReleases.Value.Where(x => x.Monitored).Should().HaveCount(1); - - Mocker.GetMock() - .Verify(x => x.DeleteMany(It.Is>(l => l.Count == 0)), Times.Once()); + Subject.RefreshAlbumInfo(newAlbum, null, false); - Mocker.GetMock() - .Verify(x => x.UpdateMany(It.Is>(l => l.Count == expectedUpdates && l.Select(r => r.ForeignReleaseId).Distinct().Count() == expectedUpdates)), Times.Once()); + Mocker.GetMock() + .Verify(x => x.RefreshEntityInfo(It.Is>(l => l.Count == 10 && l.Count(y => y.Monitored) == 1), + It.IsAny>(), + It.IsAny(), + It.IsAny())); - Mocker.GetMock() - .Verify(x => x.InsertMany(It.Is>(l => l.Count == 8 && - l.Select(r => r.ForeignReleaseId).Distinct().Count() == l.Count && - !l.Select(r => r.ForeignReleaseId).Contains("ExistingId1") && - !l.Select(r => r.ForeignReleaseId).Contains("ExistingId2"))), - Times.Once()); } [Test] public void refreshing_album_should_not_change_monitored_release_if_monitored_release_not_deleted() { - var newAlbum = Builder.CreateNew().Build(); + var newAlbum = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); // this is required because RefreshAlbumInfo will edit the album passed in - var albumCopy = Builder.CreateNew().Build(); + var albumCopy = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); // only ExistingId1 is monitored from dummy skyhook var releases = Builder.CreateListOfSize(10) @@ -308,32 +376,28 @@ namespace NzbDrone.Core.Test.MusicTests .Setup(x => x.GetAlbumInfo(It.IsAny())) .Returns(Tuple.Create("dummy string", albumCopy, new List())); - Subject.RefreshAlbumInfo(newAlbum, false); - - newAlbum.AlbumReleases.Value.Should().HaveCount(10); - newAlbum.AlbumReleases.Value.Where(x => x.Monitored).Should().HaveCount(1); - newAlbum.AlbumReleases.Value.Single(x => x.Monitored).ForeignReleaseId.Should().Be("ExistingId2"); + Subject.RefreshAlbumInfo(newAlbum, null, false); - Mocker.GetMock() - .Verify(x => x.DeleteMany(It.Is>(l => l.Count == 0)), Times.Once()); - - Mocker.GetMock() - .Verify(x => x.UpdateMany(It.Is>(l => l.Count == 0)), Times.Once()); - - Mocker.GetMock() - .Verify(x => x.InsertMany(It.Is>(l => l.Count == 8 && - l.Select(r => r.ForeignReleaseId).Distinct().Count() == l.Count && - !l.Select(r => r.ForeignReleaseId).Contains("ExistingId1") && - !l.Select(r => r.ForeignReleaseId).Contains("ExistingId2"))), - Times.Once()); + Mocker.GetMock() + .Verify(x => x.RefreshEntityInfo(It.Is>( + l => l.Count == 10 && + l.Count(y => y.Monitored) == 1 && + l.Single(y => y.Monitored).ForeignReleaseId == "ExistingId2"), + It.IsAny>(), + It.IsAny(), + It.IsAny())); } [Test] public void refreshing_album_should_change_monitored_release_if_monitored_release_deleted() { - var newAlbum = Builder.CreateNew().Build(); + var newAlbum = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); // this is required because RefreshAlbumInfo will edit the album passed in - var albumCopy = Builder.CreateNew().Build(); + var albumCopy = Builder.CreateNew() + .With(x => x.ArtistMetadata = Builder.CreateNew().Build()) + .Build(); // Only existingId1 monitored in skyhook. ExistingId2 is missing var releases = Builder.CreateListOfSize(10) @@ -369,24 +433,16 @@ namespace NzbDrone.Core.Test.MusicTests .Setup(x => x.GetAlbumInfo(It.IsAny())) .Returns(Tuple.Create("dummy string", albumCopy, new List())); - Subject.RefreshAlbumInfo(newAlbum, false); - - newAlbum.AlbumReleases.Value.Should().HaveCount(10); - newAlbum.AlbumReleases.Value.Where(x => x.Monitored).Should().HaveCount(1); - newAlbum.AlbumReleases.Value.Single(x => x.Monitored).ForeignReleaseId.Should().NotBe("ExistingId2"); - - Mocker.GetMock() - .Verify(x => x.DeleteMany(It.Is>(l => l.Single().ForeignReleaseId == "ExistingId2")), Times.Once()); - - Mocker.GetMock() - .Verify(x => x.UpdateMany(It.Is>(l => l.Count == 0)), Times.Once()); + Subject.RefreshAlbumInfo(newAlbum, null, false); - Mocker.GetMock() - .Verify(x => x.InsertMany(It.Is>(l => l.Count == 9 && - l.Select(r => r.ForeignReleaseId).Distinct().Count() == l.Count && - !l.Select(r => r.ForeignReleaseId).Contains("ExistingId1") && - !l.Select(r => r.ForeignReleaseId).Contains("ExistingId2"))), - Times.Once()); + Mocker.GetMock() + .Verify(x => x.RefreshEntityInfo(It.Is>( + l => l.Count == 11 && + l.Count(y => y.Monitored) == 1 && + l.Single(y => y.Monitored).ForeignReleaseId != "ExistingId2"), + It.IsAny>(), + It.IsAny(), + It.IsAny())); } } } diff --git a/src/NzbDrone.Core.Test/MusicTests/RefreshArtistServiceFixture.cs b/src/NzbDrone.Core.Test/MusicTests/RefreshArtistServiceFixture.cs index f97d49a94..dc55a6ea3 100644 --- a/src/NzbDrone.Core.Test/MusicTests/RefreshArtistServiceFixture.cs +++ b/src/NzbDrone.Core.Test/MusicTests/RefreshArtistServiceFixture.cs @@ -11,6 +11,9 @@ using NzbDrone.Core.Test.Framework; using NzbDrone.Core.Music; using NzbDrone.Core.Music.Commands; using NzbDrone.Test.Common; +using NzbDrone.Core.MediaFiles; +using NzbDrone.Core.History; +using NzbDrone.Core.Music.Events; namespace NzbDrone.Core.Test.MusicTests { @@ -41,17 +44,24 @@ namespace NzbDrone.Core.Test.MusicTests .With(a => a.Metadata = metadata) .Build(); - Mocker.GetMock() + Mocker.GetMock(MockBehavior.Strict) .Setup(s => s.GetArtist(_artist.Id)) .Returns(_artist); - Mocker.GetMock() - .Setup(s => s.GetAlbumsForRefresh(It.IsAny(), It.IsAny>())) - .Returns(new List()); + Mocker.GetMock(MockBehavior.Strict) + .Setup(s => s.InsertMany(It.IsAny>())); Mocker.GetMock() .Setup(s => s.GetArtistInfo(It.IsAny(), It.IsAny())) .Callback(() => { throw new ArtistNotFoundException(_artist.ForeignArtistId); }); + + Mocker.GetMock() + .Setup(x => x.GetFilesByArtist(It.IsAny())) + .Returns(new List()); + + Mocker.GetMock() + .Setup(x => x.GetByArtist(It.IsAny(), It.IsAny())) + .Returns(new List()); } private void GivenNewArtistInfo(Artist artist) @@ -60,81 +70,207 @@ namespace NzbDrone.Core.Test.MusicTests .Setup(s => s.GetArtistInfo(_artist.ForeignArtistId, _artist.MetadataProfileId)) .Returns(artist); } + + private void GivenArtistFiles() + { + Mocker.GetMock() + .Setup(x => x.GetFilesByArtist(It.IsAny())) + .Returns(Builder.CreateListOfSize(1).BuildList()); + } + + private void GivenAlbumsForRefresh() + { + Mocker.GetMock(MockBehavior.Strict) + .Setup(s => s.GetAlbumsForRefresh(It.IsAny(), It.IsAny>())) + .Returns(new List()); + } + + private void AllowArtistUpdate() + { + Mocker.GetMock(MockBehavior.Strict) + .Setup(x => x.UpdateArtist(It.IsAny())) + .Returns((Artist a) => a); + } [Test] - public void should_log_error_if_musicbrainz_id_not_found() + public void should_not_publish_artist_updated_event_if_metadata_not_updated() { - Subject.Execute(new RefreshArtistCommand(_artist.Id)); + var newArtistInfo = _artist.JsonClone(); + newArtistInfo.Metadata = _artist.Metadata.Value.JsonClone(); + newArtistInfo.Albums = _albums; - Mocker.GetMock() - .Verify(v => v.UpdateArtist(It.IsAny()), Times.Never()); + GivenNewArtistInfo(newArtistInfo); + GivenAlbumsForRefresh(); + AllowArtistUpdate(); - ExceptionVerification.ExpectedErrors(1); + Subject.Execute(new RefreshArtistCommand(_artist.Id)); + + VerifyEventNotPublished(); } [Test] - public void should_update_if_musicbrainz_id_changed() + public void should_publish_artist_updated_event_if_metadata_updated() { var newArtistInfo = _artist.JsonClone(); newArtistInfo.Metadata = _artist.Metadata.Value.JsonClone(); + newArtistInfo.Metadata.Value.Images = new List { + new MediaCover.MediaCover(MediaCover.MediaCoverTypes.Logo, "dummy") + }; newArtistInfo.Albums = _albums; - newArtistInfo.ForeignArtistId = _artist.ForeignArtistId + 1; GivenNewArtistInfo(newArtistInfo); + GivenAlbumsForRefresh(); + AllowArtistUpdate(); + + Subject.Execute(new RefreshArtistCommand(_artist.Id)); + + VerifyEventPublished(); + } + + [Test] + public void should_log_error_and_delete_if_musicbrainz_id_not_found_and_artist_has_no_files() + { + Mocker.GetMock() + .Setup(x => x.DeleteArtist(It.IsAny(), It.IsAny(), It.IsAny())); Subject.Execute(new RefreshArtistCommand(_artist.Id)); Mocker.GetMock() - .Verify(v => v.UpdateArtist(It.Is(s => s.ForeignArtistId == newArtistInfo.ForeignArtistId))); + .Verify(v => v.UpdateArtist(It.IsAny()), Times.Never()); + + Mocker.GetMock() + .Verify(v => v.DeleteArtist(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); + ExceptionVerification.ExpectedErrors(1); ExceptionVerification.ExpectedWarns(1); } + + [Test] + public void should_log_error_but_not_delete_if_musicbrainz_id_not_found_and_artist_has_files() + { + GivenArtistFiles(); + GivenAlbumsForRefresh(); + + Subject.Execute(new RefreshArtistCommand(_artist.Id)); + + Mocker.GetMock() + .Verify(v => v.UpdateArtist(It.IsAny()), Times.Never()); + + Mocker.GetMock() + .Verify(v => v.DeleteArtist(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); + + ExceptionVerification.ExpectedErrors(2); + } [Test] - [Ignore("This test needs to be re-written as we no longer store albums in artist table or object")] - public void should_not_throw_if_duplicate_album_is_in_existing_info() + public void should_update_if_musicbrainz_id_changed_and_no_clash() { var newArtistInfo = _artist.JsonClone(); - newArtistInfo.Albums.Value.Add(Builder.CreateNew() - .With(s => s.ForeignAlbumId = "2") - .Build()); + newArtistInfo.Metadata = _artist.Metadata.Value.JsonClone(); + newArtistInfo.Albums = _albums; + newArtistInfo.ForeignArtistId = _artist.ForeignArtistId + 1; + newArtistInfo.Metadata.Value.Id = 100; + + GivenNewArtistInfo(newArtistInfo); - _artist.Albums.Value.Add(Builder.CreateNew() - .With(s => s.ForeignAlbumId = "2") - .Build()); + var seq = new MockSequence(); - _artist.Albums.Value.Add(Builder.CreateNew() - .With(s => s.ForeignAlbumId = "2") - .Build()); + Mocker.GetMock(MockBehavior.Strict) + .Setup(x => x.FindById(newArtistInfo.ForeignArtistId)) + .Returns(default(Artist)); - GivenNewArtistInfo(newArtistInfo); + // Make sure that the artist is updated before we refresh the albums + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.UpdateArtist(It.IsAny())) + .Returns((Artist a) => a); + + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.GetAlbumsForRefresh(It.IsAny(), It.IsAny>())) + .Returns(new List()); + + // Update called twice for a move/merge + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.UpdateArtist(It.IsAny())) + .Returns((Artist a) => a); Subject.Execute(new RefreshArtistCommand(_artist.Id)); Mocker.GetMock() - .Verify(v => v.UpdateArtist(It.Is(s => s.Albums.Value.Count == 2))); + .Verify(v => v.UpdateArtist(It.Is(s => s.ArtistMetadataId == 100 && s.ForeignArtistId == newArtistInfo.ForeignArtistId)), + Times.Exactly(2)); } [Test] - [Ignore("This test needs to be re-written as we no longer store albums in artist table or object")] - public void should_filter_duplicate_albums() + public void should_merge_if_musicbrainz_id_changed_and_new_id_already_exists() { - var newArtistInfo = _artist.JsonClone(); - newArtistInfo.Albums.Value.Add(Builder.CreateNew() - .With(s => s.ForeignAlbumId = "2") - .Build()); + var existing = _artist; + + var clash = _artist.JsonClone(); + clash.Id = 100; + clash.Metadata = existing.Metadata.Value.JsonClone(); + clash.Metadata.Value.Id = 101; + clash.Metadata.Value.ForeignArtistId = clash.Metadata.Value.ForeignArtistId + 1; - newArtistInfo.Albums.Value.Add(Builder.CreateNew() - .With(s => s.ForeignAlbumId = "2") - .Build()); + Mocker.GetMock(MockBehavior.Strict) + .Setup(x => x.FindById(clash.Metadata.Value.ForeignArtistId)) + .Returns(clash); + + var newArtistInfo = clash.JsonClone(); + newArtistInfo.Metadata = clash.Metadata.Value.JsonClone(); + newArtistInfo.Albums = _albums.JsonClone(); + newArtistInfo.Albums.Value.ForEach(x => x.Id = 0); GivenNewArtistInfo(newArtistInfo); + var seq = new MockSequence(); + + // Make sure that the artist is updated before we refresh the albums + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.GetAlbumsByArtist(existing.Id)) + .Returns(_albums); + + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.UpdateMany(It.IsAny>())); + + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.DeleteArtist(existing.Id, It.IsAny(), false)); + + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.UpdateArtist(It.Is(a => a.Id == clash.Id))) + .Returns((Artist a) => a); + + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.GetAlbumsForRefresh(clash.ArtistMetadataId, It.IsAny>())) + .Returns(_albums); + + // Update called twice for a move/merge + Mocker.GetMock(MockBehavior.Strict) + .InSequence(seq) + .Setup(x => x.UpdateArtist(It.IsAny())) + .Returns((Artist a) => a); + Subject.Execute(new RefreshArtistCommand(_artist.Id)); + // the retained artist gets updated + Mocker.GetMock() + .Verify(v => v.UpdateArtist(It.Is(s => s.Id == clash.Id)), Times.Exactly(2)); + + // the old one gets removed Mocker.GetMock() - .Verify(v => v.UpdateArtist(It.Is(s => s.Albums.Value.Count == 2))); + .Verify(v => v.DeleteArtist(existing.Id, false, false)); + + Mocker.GetMock() + .Verify(v => v.UpdateMany(It.Is>(x => x.Count == _albums.Count))); + ExceptionVerification.ExpectedWarns(1); } } } diff --git a/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj b/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj index 183c46f2f..554fa39d5 100644 --- a/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj +++ b/src/NzbDrone.Core.Test/NzbDrone.Core.Test.csproj @@ -275,7 +275,6 @@ - diff --git a/src/NzbDrone.Core/History/HistoryService.cs b/src/NzbDrone.Core/History/HistoryService.cs index b2bba0e01..1e0c280d9 100644 --- a/src/NzbDrone.Core/History/HistoryService.cs +++ b/src/NzbDrone.Core/History/HistoryService.cs @@ -28,6 +28,7 @@ namespace NzbDrone.Core.History List Find(string downloadId, HistoryEventType eventType); List FindByDownloadId(string downloadId); List Since(DateTime date, HistoryEventType? eventType); + void UpdateMany(IList items); } public class HistoryService : IHistoryService, @@ -381,5 +382,10 @@ namespace NzbDrone.Core.History { return _historyRepository.Since(date, eventType); } + + public void UpdateMany(IList items) + { + _historyRepository.UpdateMany(items); + } } } diff --git a/src/NzbDrone.Core/MetadataSource/SkyHook/Resource/ArtistResource.cs b/src/NzbDrone.Core/MetadataSource/SkyHook/Resource/ArtistResource.cs index 52a0dc852..170617391 100644 --- a/src/NzbDrone.Core/MetadataSource/SkyHook/Resource/ArtistResource.cs +++ b/src/NzbDrone.Core/MetadataSource/SkyHook/Resource/ArtistResource.cs @@ -1,14 +1,13 @@ -using System; using System.Collections.Generic; -using System.Linq; -using System.Text; namespace NzbDrone.Core.MetadataSource.SkyHook.Resource { public class ArtistResource { - public ArtistResource() { + public ArtistResource() + { Albums = new List(); + Genres = new List(); } public List Genres { get; set; } diff --git a/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs b/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs index 362c361e7..7d3badd8b 100644 --- a/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs +++ b/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs @@ -82,10 +82,7 @@ namespace NzbDrone.Core.MetadataSource.SkyHook artist.SortName = Parser.Parser.NormalizeTitle(artist.Metadata.Value.Name); artist.Albums = FilterAlbums(httpResponse.Resource.Albums, metadataProfileId) - .Select(x => new Album { - ForeignAlbumId = x.Id - }) - .ToList(); + .Select(x => MapAlbum(x, null)).ToList(); return artist; } @@ -136,6 +133,7 @@ namespace NzbDrone.Core.MetadataSource.SkyHook var artists = httpResponse.Resource.Artists.Select(MapArtistMetadata).ToList(); var artistDict = artists.ToDictionary(x => x.ForeignArtistId, x => x); var album = MapAlbum(httpResponse.Resource, artistDict); + album.ArtistMetadata = artistDict[httpResponse.Resource.ArtistId]; return new Tuple>(httpResponse.Resource.ArtistId, album, artists); } @@ -181,8 +179,6 @@ namespace NzbDrone.Core.MetadataSource.SkyHook .SetSegment("route", "search") .AddQueryParam("type", "artist") .AddQueryParam("query", title.ToLower().Trim()) - //.AddQueryParam("images","false") // Should pass these on import search to avoid looking to fanart and wiki - //.AddQueryParam("overview","false") .Build(); diff --git a/src/NzbDrone.Core/Music/AddAlbumService.cs b/src/NzbDrone.Core/Music/AddAlbumService.cs deleted file mode 100644 index c2cf6d417..000000000 --- a/src/NzbDrone.Core/Music/AddAlbumService.cs +++ /dev/null @@ -1,160 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using FluentValidation; -using FluentValidation.Results; -using NLog; -using NzbDrone.Common.EnsureThat; -using NzbDrone.Common.Extensions; -using NzbDrone.Common.Instrumentation.Extensions; -using NzbDrone.Core.Exceptions; -using NzbDrone.Core.MetadataSource; - -namespace NzbDrone.Core.Music -{ - public interface IAddAlbumService - { - Album AddAlbum(Album newAlbum); - List AddAlbums(List newAlbums); - } - - public class AddAlbumService : IAddAlbumService - { - private readonly IAlbumService _albumService; - private readonly IReleaseService _releaseService; - private readonly IProvideAlbumInfo _albumInfo; - private readonly IArtistMetadataRepository _artistMetadataRepository; - private readonly IRefreshTrackService _refreshTrackService; - private readonly Logger _logger; - - public AddAlbumService(IAlbumService albumService, - IReleaseService releaseService, - IProvideAlbumInfo albumInfo, - IArtistMetadataRepository artistMetadataRepository, - IRefreshTrackService refreshTrackService, - Logger logger) - { - _albumService = albumService; - _releaseService = releaseService; - _albumInfo = albumInfo; - _artistMetadataRepository = artistMetadataRepository; - _refreshTrackService = refreshTrackService; - _logger = logger; - } - - public List AddAlbumReleases(Album album) - { - var remoteReleases = album.AlbumReleases.Value.DistinctBy(m => m.ForeignReleaseId).ToList(); - var existingReleases = _releaseService.GetReleasesForRefresh(album.Id, remoteReleases.Select(x => x.ForeignReleaseId)); - var newReleaseList = new List(); - var updateReleaseList = new List(); - - foreach (var release in remoteReleases) - { - release.AlbumId = album.Id; - release.Album = album; - var releaseToRefresh = existingReleases.SingleOrDefault(r => r.ForeignReleaseId == release.ForeignReleaseId); - - if (releaseToRefresh != null) - { - existingReleases.Remove(releaseToRefresh); - - // copy across the db keys and check for equality - release.Id = releaseToRefresh.Id; - release.AlbumId = releaseToRefresh.AlbumId; - - updateReleaseList.Add(release); - } - else - { - newReleaseList.Add(release); - } - } - - // Ensure only one release is monitored - remoteReleases.ForEach(x => x.Monitored = false); - remoteReleases.OrderByDescending(x => x.TrackCount).First().Monitored = true; - Ensure.That(remoteReleases.Count(x => x.Monitored) == 1).IsTrue(); - - // Since this is a new album, we can't be deleting any existing releases - _releaseService.UpdateMany(updateReleaseList); - _releaseService.InsertMany(newReleaseList); - - return remoteReleases; - } - - private Album AddAlbum(Tuple> skyHookData) - { - var newAlbum = skyHookData.Item2; - - if (newAlbum.AlbumReleases.Value.Count == 0) - { - _logger.Debug($"Skipping album with no valid releases {newAlbum}"); - return null; - } - - _logger.ProgressInfo("Adding Album {0}", newAlbum.Title); - - _artistMetadataRepository.UpsertMany(skyHookData.Item3); - newAlbum.ArtistMetadata = _artistMetadataRepository.FindById(skyHookData.Item1); - newAlbum.ArtistMetadataId = newAlbum.ArtistMetadata.Value.Id; - - _albumService.AddAlbum(newAlbum); - AddAlbumReleases(newAlbum); - - _refreshTrackService.RefreshTrackInfo(newAlbum, false); - - return newAlbum; - } - - public Album AddAlbum(Album newAlbum) - { - Ensure.That(newAlbum, () => newAlbum).IsNotNull(); - - var tuple = AddSkyhookData(newAlbum); - - return AddAlbum(tuple); - } - - public List AddAlbums(List newAlbums) - { - var added = DateTime.UtcNow; - var albumsToAdd = new List(); - - foreach (var newAlbum in newAlbums) - { - var tuple = AddSkyhookData(newAlbum); - tuple.Item2.Added = added; - tuple.Item2.LastInfoSync = added; - - albumsToAdd.Add(AddAlbum(tuple)); - } - - return albumsToAdd; - } - - private Tuple> AddSkyhookData(Album newAlbum) - { - Tuple> tuple; - - try - { - tuple = _albumInfo.GetAlbumInfo(newAlbum.ForeignAlbumId); - } - catch (AlbumNotFoundException) - { - _logger.Error("LidarrId {1} was not found, it may have been removed from Lidarr.", newAlbum.ForeignAlbumId); - - throw new ValidationException(new List - { - new ValidationFailure("MusicBrainzId", "An album with this ID was not found", newAlbum.ForeignAlbumId) - }); - } - - tuple.Item2.Monitored = newAlbum.Monitored; - tuple.Item2.ProfileId = newAlbum.ProfileId; - - return tuple; - } - } -} diff --git a/src/NzbDrone.Core/Music/AddArtistService.cs b/src/NzbDrone.Core/Music/AddArtistService.cs index 3206efeb4..c2b1fc745 100644 --- a/src/NzbDrone.Core/Music/AddArtistService.cs +++ b/src/NzbDrone.Core/Music/AddArtistService.cs @@ -23,18 +23,21 @@ namespace NzbDrone.Core.Music public class AddArtistService : IAddArtistService { private readonly IArtistService _artistService; + private readonly IArtistMetadataRepository _artistMetadataRepository; private readonly IProvideArtistInfo _artistInfo; private readonly IBuildFileNames _fileNameBuilder; private readonly IAddArtistValidator _addArtistValidator; private readonly Logger _logger; public AddArtistService(IArtistService artistService, + IArtistMetadataRepository artistMetadataRepository, IProvideArtistInfo artistInfo, IBuildFileNames fileNameBuilder, IAddArtistValidator addArtistValidator, Logger logger) { _artistService = artistService; + _artistMetadataRepository = artistMetadataRepository; _artistInfo = artistInfo; _fileNameBuilder = fileNameBuilder; _addArtistValidator = addArtistValidator; @@ -49,6 +52,12 @@ namespace NzbDrone.Core.Music newArtist = SetPropertiesAndValidate(newArtist); _logger.Info("Adding Artist {0} Path: [{1}]", newArtist, newArtist.Path); + + // add metadata + _artistMetadataRepository.Upsert(newArtist.Metadata.Value); + newArtist.ArtistMetadataId = newArtist.Metadata.Value.Id; + + // add the artist itself _artistService.AddArtist(newArtist); return newArtist; @@ -77,6 +86,12 @@ namespace NzbDrone.Core.Music } + // add metadata + _artistMetadataRepository.UpsertMany(artistsToAdd); + + _logger.Debug("metadata id 1 {0}", string.Join(", ", artistsToAdd.Select(x => x.Metadata.Value.Id))); + _logger.Debug("metadata id 2 {0}", string.Join(", ", artistsToAdd.Select(x => x.ArtistMetadataId))); + return _artistService.AddArtists(artistsToAdd); } diff --git a/src/NzbDrone.Core/Music/Album.cs b/src/NzbDrone.Core/Music/Album.cs index 2ecc69564..6eec0f258 100644 --- a/src/NzbDrone.Core/Music/Album.cs +++ b/src/NzbDrone.Core/Music/Album.cs @@ -3,10 +3,12 @@ using NzbDrone.Core.Datastore; using System; using System.Collections.Generic; using Marr.Data; +using System.Linq; +using NzbDrone.Common.Serializer; namespace NzbDrone.Core.Music { - public class Album : ModelBase + public class Album : ModelBase, IEquatable { public Album() { @@ -66,5 +68,72 @@ namespace NzbDrone.Core.Music Monitored = otherAlbum.Monitored; AnyReleaseOk = otherAlbum.AnyReleaseOk; } + + public bool Equals(Album other) + { + if (other == null) + { + return false; + } + + if (Id == other.Id && + ForeignAlbumId == other.ForeignAlbumId && + (OldForeignAlbumIds?.SequenceEqual(other.OldForeignAlbumIds) ?? true) && + Title == other.Title && + Overview == other.Overview && + Disambiguation == other.Disambiguation && + ReleaseDate == other.ReleaseDate && + Images?.ToJson() == other.Images?.ToJson() && + Links?.ToJson() == other.Links?.ToJson() && + (Genres?.SequenceEqual(other.Genres) ?? true) && + AlbumType == other.AlbumType && + (SecondaryTypes?.SequenceEqual(other.SecondaryTypes) ?? true) && + Ratings?.ToJson() == other.Ratings?.ToJson()) + { + return true; + } + + return false; + } + + public override bool Equals(object obj) + { + if (obj == null) + { + return false; + } + + var other = obj as Album; + if (other == null) + { + return false; + } + else + { + return Equals(other); + } + } + + public override int GetHashCode() + { + unchecked + { + int hash = 17; + hash = hash * 23 + Id; + hash = hash * 23 + ForeignAlbumId.GetHashCode(); + hash = hash * 23 + OldForeignAlbumIds?.GetHashCode() ?? 0; + hash = hash * 23 + Title?.GetHashCode() ?? 0; + hash = hash * 23 + Overview?.GetHashCode() ?? 0; + hash = hash * 23 + Disambiguation?.GetHashCode() ?? 0; + hash = hash * 23 + ReleaseDate?.GetHashCode() ?? 0; + hash = hash * 23 + Images?.GetHashCode() ?? 0; + hash = hash * 23 + Links?.GetHashCode() ?? 0; + hash = hash * 23 + Genres?.GetHashCode() ?? 0; + hash = hash * 23 + AlbumType?.GetHashCode() ?? 0; + hash = hash * 23 + SecondaryTypes?.GetHashCode() ?? 0; + hash = hash * 23 + Ratings?.GetHashCode() ?? 0; + return hash; + } + } } } diff --git a/src/NzbDrone.Core/Music/ArtistMetadataRepository.cs b/src/NzbDrone.Core/Music/ArtistMetadataRepository.cs index ca5f6e804..c999019d0 100644 --- a/src/NzbDrone.Core/Music/ArtistMetadataRepository.cs +++ b/src/NzbDrone.Core/Music/ArtistMetadataRepository.cs @@ -2,7 +2,6 @@ using System.Collections.Generic; using NzbDrone.Core.Datastore; using NzbDrone.Core.Messaging.Events; -using Marr.Data; using NLog; namespace NzbDrone.Core.Music @@ -16,8 +15,8 @@ namespace NzbDrone.Core.Music void UpdateMany(List artists); ArtistMetadata FindById(string foreignArtistId); List FindById(List foreignIds); - void UpsertMany(List artists); - void UpsertMany(List artists); + bool UpsertMany(List artists); + bool UpsertMany(List artists); } public class ArtistMetadataRepository : BasicRepository, IArtistMetadataRepository @@ -40,10 +39,8 @@ namespace NzbDrone.Core.Music public List InsertMany(List artists) { InsertMany(artists.Select(x => x.Metadata.Value).ToList()); - foreach (var artist in artists) - { - artist.ArtistMetadataId = artist.Metadata.Value.Id; - } + artists.ForEach(x => x.ArtistMetadataId = x.Metadata.Value.Id); + return artists; } @@ -70,12 +67,12 @@ namespace NzbDrone.Core.Music return artist; } - public void UpsertMany(List artists) + public bool UpsertMany(List artists) { - foreach (var artist in artists) - { - Upsert(artist); - } + var result = UpsertMany(artists.Select(x => x.Metadata.Value).ToList()); + artists.ForEach(x => x.ArtistMetadataId = x.Metadata.Value.Id); + + return result; } public void UpdateMany(List artists) @@ -93,22 +90,40 @@ namespace NzbDrone.Core.Music return Query.Where($"[ForeignArtistId] IN ('{string.Join("','", foreignIds)}')").ToList(); } - public void UpsertMany(List artists) + public bool UpsertMany(List data) { - foreach (var artist in artists) + var existingMetadata = FindById(data.Select(x => x.ForeignArtistId).ToList()); + var updateMetadataList = new List(); + var addMetadataList = new List(); + int upToDateMetadataCount = 0; + + foreach (var meta in data) { - var existing = FindById(artist.ForeignArtistId); + var existing = existingMetadata.SingleOrDefault(x => x.ForeignArtistId == meta.ForeignArtistId); if (existing != null) { - artist.Id = existing.Id; - Update(artist); + meta.Id = existing.Id; + if (!meta.Equals(existing)) + { + updateMetadataList.Add(meta); + } + else + { + upToDateMetadataCount++; + } } else { - Insert(artist); + addMetadataList.Add(meta); } - _logger.Debug("Upserted metadata with ID {0}", artist.Id); } + + UpdateMany(updateMetadataList); + InsertMany(addMetadataList); + + _logger.Debug($"{upToDateMetadataCount} artist metadata up to date; Updating {updateMetadataList.Count}, Adding {addMetadataList.Count} artist metadata entries."); + + return updateMetadataList.Count > 0 || addMetadataList.Count > 0; } } } diff --git a/src/NzbDrone.Core/Music/ArtistService.cs b/src/NzbDrone.Core/Music/ArtistService.cs index b6c471b6d..192bb718c 100644 --- a/src/NzbDrone.Core/Music/ArtistService.cs +++ b/src/NzbDrone.Core/Music/ArtistService.cs @@ -34,7 +34,6 @@ namespace NzbDrone.Core.Music public class ArtistService : IArtistService { private readonly IArtistRepository _artistRepository; - private readonly IArtistMetadataRepository _artistMetadataRepository; private readonly IEventAggregator _eventAggregator; private readonly ITrackService _trackService; private readonly IImportListExclusionService _importListExclusionService; @@ -43,7 +42,6 @@ namespace NzbDrone.Core.Music private readonly ICached> _cache; public ArtistService(IArtistRepository artistRepository, - IArtistMetadataRepository artistMetadataRepository, IEventAggregator eventAggregator, ITrackService trackService, IImportListExclusionService importListExclusionService, @@ -52,7 +50,6 @@ namespace NzbDrone.Core.Music Logger logger) { _artistRepository = artistRepository; - _artistMetadataRepository = artistMetadataRepository; _eventAggregator = eventAggregator; _trackService = trackService; _importListExclusionService = importListExclusionService; @@ -64,7 +61,6 @@ namespace NzbDrone.Core.Music public Artist AddArtist(Artist newArtist) { _cache.Clear(); - _artistMetadataRepository.Upsert(newArtist); _artistRepository.Insert(newArtist); _eventAggregator.PublishEvent(new ArtistAddedEvent(GetArtist(newArtist.Id))); @@ -74,7 +70,6 @@ namespace NzbDrone.Core.Music public List AddArtists(List newArtists) { _cache.Clear(); - _artistMetadataRepository.UpsertMany(newArtists); _artistRepository.InsertMany(newArtists); _eventAggregator.PublishEvent(new ArtistsImportedEvent(newArtists.Select(s => s.Id).ToList())); @@ -211,9 +206,8 @@ namespace NzbDrone.Core.Music public Artist UpdateArtist(Artist artist) { _cache.Clear(); - var storedArtist = GetArtist(artist.Id); // Is it Id or iTunesId? - var updatedArtist = _artistMetadataRepository.Update(artist); - updatedArtist = _artistRepository.Update(updatedArtist); + var storedArtist = GetArtist(artist.Id); + var updatedArtist = _artistRepository.Update(artist); _eventAggregator.PublishEvent(new ArtistEditedEvent(updatedArtist, storedArtist)); return updatedArtist; @@ -242,7 +236,6 @@ namespace NzbDrone.Core.Music } } - _artistMetadataRepository.UpdateMany(artist); _artistRepository.UpdateMany(artist); _logger.Debug("{0} artists updated", artist.Count); diff --git a/src/NzbDrone.Core/Music/RefreshAlbumReleaseService.cs b/src/NzbDrone.Core/Music/RefreshAlbumReleaseService.cs new file mode 100644 index 000000000..38ded63e4 --- /dev/null +++ b/src/NzbDrone.Core/Music/RefreshAlbumReleaseService.cs @@ -0,0 +1,138 @@ +using NLog; +using NzbDrone.Common.Extensions; +using System; +using System.Collections.Generic; +using System.Linq; +using NzbDrone.Core.MediaFiles; + +namespace NzbDrone.Core.Music +{ + public interface IRefreshAlbumReleaseService + { + bool RefreshEntityInfo(AlbumRelease entity, List remoteEntityList, bool forceChildRefresh, bool forceUpdateFileTags); + bool RefreshEntityInfo(List releases, List remoteEntityList, bool forceChildRefresh, bool forceUpdateFileTags); + } + + public class RefreshAlbumReleaseService : RefreshEntityServiceBase, IRefreshAlbumReleaseService + { + private readonly IReleaseService _releaseService; + private readonly IRefreshTrackService _refreshTrackService; + private readonly ITrackService _trackService; + private readonly IMediaFileService _mediaFileService; + private readonly Logger _logger; + + public RefreshAlbumReleaseService(IReleaseService releaseService, + IArtistMetadataRepository artistMetadataRepository, + IRefreshTrackService refreshTrackService, + ITrackService trackService, + IMediaFileService mediaFileService, + Logger logger) + : base(logger, artistMetadataRepository) + { + _releaseService = releaseService; + _trackService = trackService; + _refreshTrackService = refreshTrackService; + _mediaFileService = mediaFileService; + _logger = logger; + } + + protected override RemoteData GetRemoteData(AlbumRelease local, List remote) + { + var result = new RemoteData(); + result.Entity = remote.SingleOrDefault(x => x.ForeignReleaseId == local.ForeignReleaseId || x.OldForeignReleaseIds.Contains(local.ForeignReleaseId)); + return result; + } + + protected override bool IsMerge(AlbumRelease local, AlbumRelease remote) + { + return local.ForeignReleaseId != remote.ForeignReleaseId; + } + + protected override UpdateResult UpdateEntity(AlbumRelease local, AlbumRelease remote) + { + if (local.Equals(remote)) + { + return UpdateResult.None; + } + + local.OldForeignReleaseIds = remote.OldForeignReleaseIds; + local.Title = remote.Title; + local.Status = remote.Status; + local.Duration = remote.Duration; + local.Label = remote.Label; + local.Disambiguation = remote.Disambiguation; + local.Country = remote.Country; + local.ReleaseDate = remote.ReleaseDate; + local.Media = remote.Media; + local.TrackCount = remote.TrackCount; + + return UpdateResult.UpdateTags; + } + + protected override AlbumRelease GetEntityByForeignId(AlbumRelease local) + { + return _releaseService.GetReleaseByForeignReleaseId(local.ForeignReleaseId); + } + + protected override void SaveEntity(AlbumRelease local) + { + _releaseService.UpdateMany(new List { local }); + } + + protected override void DeleteEntity(AlbumRelease local, bool deleteFiles) + { + _releaseService.DeleteMany(new List { local }); + } + + protected override List GetRemoteChildren(AlbumRelease remote) + { + return remote.Tracks.Value.DistinctBy(m => m.ForeignTrackId).ToList(); + } + + protected override List GetLocalChildren(AlbumRelease entity, List remoteChildren) + { + return _trackService.GetTracksForRefresh(entity.Id, + remoteChildren.Select(x => x.ForeignTrackId) + .Concat(remoteChildren.SelectMany(x => x.OldForeignTrackIds))); + } + + protected override Tuple > GetMatchingExistingChildren(List existingChildren, Track remote) + { + var existingChild = existingChildren.SingleOrDefault(x => x.ForeignTrackId == remote.ForeignTrackId); + var mergeChildren = existingChildren.Where(x => remote.OldForeignTrackIds.Contains(x.ForeignTrackId)).ToList(); + return Tuple.Create(existingChild, mergeChildren); + } + + protected override void PrepareNewChild(Track child, AlbumRelease entity) + { + child.AlbumReleaseId = entity.Id; + child.AlbumRelease = entity; + child.ArtistMetadataId = child.ArtistMetadata.Value.Id; + + // make sure title is not null + child.Title = child.Title ?? "Unknown"; + } + + protected override void PrepareExistingChild(Track local, Track remote, AlbumRelease entity) + { + local.AlbumRelease = entity; + local.AlbumReleaseId = entity.Id; + local.ArtistMetadataId = remote.ArtistMetadata.Value.Id; + remote.Id = local.Id; + remote.TrackFileId = local.TrackFileId; + remote.AlbumReleaseId = local.AlbumReleaseId; + remote.ArtistMetadataId = local.ArtistMetadataId; + } + + protected override void AddChildren(List children) + { + _trackService.InsertMany(children); + } + + protected override bool RefreshChildren(SortedChildren localChildren, List remoteChildren, bool forceChildRefresh, bool forceUpdateFileTags) + { + return _refreshTrackService.RefreshTrackInfo(localChildren.Added, localChildren.Updated, localChildren.Merged, localChildren.Deleted, localChildren.UpToDate, remoteChildren, forceUpdateFileTags); + } + } +} + diff --git a/src/NzbDrone.Core/Music/RefreshAlbumService.cs b/src/NzbDrone.Core/Music/RefreshAlbumService.cs index e11cc809a..2ab7a6ad4 100644 --- a/src/NzbDrone.Core/Music/RefreshAlbumService.cs +++ b/src/NzbDrone.Core/Music/RefreshAlbumService.cs @@ -11,247 +11,323 @@ using NzbDrone.Core.Exceptions; using NzbDrone.Core.Messaging.Commands; using NzbDrone.Core.Music.Commands; using NzbDrone.Core.MediaFiles; -using NzbDrone.Common.EnsureThat; +using NzbDrone.Core.History; namespace NzbDrone.Core.Music { public interface IRefreshAlbumService { - bool RefreshAlbumInfo(Album album, bool forceUpdateFileTags); - bool RefreshAlbumInfo(List albums, bool forceAlbumRefresh, bool forceUpdateFileTags); + bool RefreshAlbumInfo(Album album, List remoteAlbums, bool forceUpdateFileTags); + bool RefreshAlbumInfo(List albums, List remoteAlbums, bool forceAlbumRefresh, bool forceUpdateFileTags); } - public class RefreshAlbumService : IRefreshAlbumService, IExecute + public class RefreshAlbumService : RefreshEntityServiceBase, IRefreshAlbumService, IExecute { private readonly IAlbumService _albumService; private readonly IArtistService _artistService; - private readonly IArtistMetadataRepository _artistMetadataRepository; + private readonly IAddArtistService _addArtistService; private readonly IReleaseService _releaseService; private readonly IProvideAlbumInfo _albumInfo; - private readonly IRefreshTrackService _refreshTrackService; - private readonly IAudioTagService _audioTagService; + private readonly IRefreshAlbumReleaseService _refreshAlbumReleaseService; + private readonly IMediaFileService _mediaFileService; + private readonly IHistoryService _historyService; private readonly IEventAggregator _eventAggregator; private readonly ICheckIfAlbumShouldBeRefreshed _checkIfAlbumShouldBeRefreshed; private readonly Logger _logger; public RefreshAlbumService(IAlbumService albumService, IArtistService artistService, + IAddArtistService addArtistService, IArtistMetadataRepository artistMetadataRepository, IReleaseService releaseService, IProvideAlbumInfo albumInfo, - IRefreshTrackService refreshTrackService, - IAudioTagService audioTagService, + IRefreshAlbumReleaseService refreshAlbumReleaseService, + IMediaFileService mediaFileService, + IHistoryService historyService, IEventAggregator eventAggregator, ICheckIfAlbumShouldBeRefreshed checkIfAlbumShouldBeRefreshed, Logger logger) + : base(logger, artistMetadataRepository) { _albumService = albumService; _artistService = artistService; - _artistMetadataRepository = artistMetadataRepository; + _addArtistService = addArtistService; _releaseService = releaseService; _albumInfo = albumInfo; - _refreshTrackService = refreshTrackService; - _audioTagService = audioTagService; + _refreshAlbumReleaseService = refreshAlbumReleaseService; + _mediaFileService = mediaFileService; + _historyService = historyService; _eventAggregator = eventAggregator; _checkIfAlbumShouldBeRefreshed = checkIfAlbumShouldBeRefreshed; _logger = logger; } - public bool RefreshAlbumInfo(List albums, bool forceAlbumRefresh, bool forceUpdateFileTags) + protected override RemoteData GetRemoteData(Album local, List remote) { - bool updated = false; - foreach (var album in albums) + var result = new RemoteData(); + + // remove not in remote list and ShouldDelete is true + if (remote != null && + !remote.Any(x => x.ForeignAlbumId == local.ForeignAlbumId || x.OldForeignAlbumIds.Contains(local.ForeignAlbumId)) && + ShouldDelete(local)) { - if (forceAlbumRefresh || _checkIfAlbumShouldBeRefreshed.ShouldRefresh(album)) - { - updated |= RefreshAlbumInfo(album, forceUpdateFileTags); - } + return result; } - return updated; - } - - public bool RefreshAlbumInfo(Album album, bool forceUpdateFileTags) - { - _logger.ProgressInfo("Updating Info for {0}", album.Title); - bool updated = false; - - Tuple> tuple; - + + Tuple> tuple = null; try { - tuple = _albumInfo.GetAlbumInfo(album.ForeignAlbumId); + tuple = _albumInfo.GetAlbumInfo(local.ForeignAlbumId); } catch (AlbumNotFoundException) { - _logger.Error($"{album} was not found, it may have been removed from Metadata sources."); - return updated; + return result; } if (tuple.Item2.AlbumReleases.Value.Count == 0) { - _logger.Debug($"{album} has no valid releases, removing."); - _albumService.DeleteMany(new List { album }); - return true; + _logger.Debug($"{local} has no valid releases, removing."); + return result; } + + result.Entity = tuple.Item2; + result.Metadata = tuple.Item3; + return result; + } + + protected override void EnsureNewParent(Album local, Album remote) + { + // Make sure the appropriate artist exists (it could be that an album changes parent) + // The artistMetadata entry will be in the db but make sure a corresponding artist is too + // so that the album doesn't just disappear. + + // TODO filter by metadata id before hitting database + _logger.Trace($"Ensuring parent artist exists [{remote.ArtistMetadata.Value.ForeignArtistId}]"); + + var newArtist = _artistService.FindById(remote.ArtistMetadata.Value.ForeignArtistId); + + if (newArtist == null) + { + var oldArtist = local.Artist.Value; + var addArtist = new Artist { + Metadata = remote.ArtistMetadata.Value, + MetadataProfileId = oldArtist.MetadataProfileId, + QualityProfileId = oldArtist.QualityProfileId, + LanguageProfileId = oldArtist.LanguageProfileId, + RootFolderPath = oldArtist.RootFolderPath, + Monitored = oldArtist.Monitored, + AlbumFolder = oldArtist.AlbumFolder + }; + _logger.Debug($"Adding missing parent artist {addArtist}"); + _addArtistService.AddArtist(addArtist); + } + } + + protected override bool ShouldDelete(Album local) + { + return !_mediaFileService.GetFilesByAlbum(local.Id).Any(); + } - var remoteMetadata = tuple.Item3.DistinctBy(x => x.ForeignArtistId).ToList(); - var existingMetadata = _artistMetadataRepository.FindById(remoteMetadata.Select(x => x.ForeignArtistId).ToList()); - var updateMetadataList = new List(); - var addMetadataList = new List(); - var upToDateMetadataCount = 0; + protected override void LogProgress(Album local) + { + _logger.ProgressInfo("Updating Info for {0}", local.Title); + } - foreach (var meta in remoteMetadata) + protected override bool IsMerge(Album local, Album remote) + { + return local.ForeignAlbumId != remote.ForeignAlbumId; + } + + protected override UpdateResult UpdateEntity(Album local, Album remote) + { + UpdateResult result; + + if (local.Title != (remote.Title ?? "Unknown") || + local.ForeignAlbumId != remote.ForeignAlbumId || + local.ArtistMetadata.Value.ForeignArtistId != remote.ArtistMetadata.Value.ForeignArtistId) { - var existing = existingMetadata.SingleOrDefault(x => x.ForeignArtistId == meta.ForeignArtistId); - if (existing != null) - { - meta.Id = existing.Id; - if (!meta.Equals(existing)) - { - updateMetadataList.Add(meta); - } - else - { - upToDateMetadataCount++; - } - } - else - { - addMetadataList.Add(meta); - } + result = UpdateResult.UpdateTags; + } + else if (!local.Equals(remote)) + { + result = UpdateResult.Standard; + } + else + { + result = UpdateResult.None; } + + local.ArtistMetadataId = remote.ArtistMetadata.Value.Id; + local.ForeignAlbumId = remote.ForeignAlbumId; + local.OldForeignAlbumIds = remote.OldForeignAlbumIds; + local.LastInfoSync = DateTime.UtcNow; + local.CleanTitle = remote.CleanTitle; + local.Title = remote.Title ?? "Unknown"; + local.Overview = remote.Overview.IsNullOrWhiteSpace() ? local.Overview : remote.Overview; + local.Disambiguation = remote.Disambiguation; + local.AlbumType = remote.AlbumType; + local.SecondaryTypes = remote.SecondaryTypes; + local.Genres = remote.Genres; + local.Images = remote.Images.Any() ? remote.Images : local.Images; + local.Links = remote.Links; + local.ReleaseDate = remote.ReleaseDate; + local.Ratings = remote.Ratings; + local.AlbumReleases = new List(); + + return result; + } - _logger.Debug($"{album}: {upToDateMetadataCount} artist metadata up to date; Updating {updateMetadataList.Count}, Adding {addMetadataList.Count} artist metadata entries."); + protected override UpdateResult MergeEntity(Album local, Album target, Album remote) + { + _logger.Warn($"Album {local} was merged with {remote} because the original was a duplicate."); - _artistMetadataRepository.UpdateMany(updateMetadataList); - _artistMetadataRepository.InsertMany(addMetadataList); + // move releases over to the new album and delete + var localReleases = _releaseService.GetReleasesByAlbum(local.Id); + var allReleases = localReleases.Concat(_releaseService.GetReleasesByAlbum(target.Id)).ToList(); + _logger.Trace($"Moving {localReleases.Count} releases from {local} to {remote}"); - forceUpdateFileTags |= updateMetadataList.Any(); - updated |= updateMetadataList.Any() || addMetadataList.Any(); + // Update album ID and unmonitor all releases from the old album + allReleases.ForEach(x => x.AlbumId = target.Id); + MonitorSingleRelease(allReleases); + _releaseService.UpdateMany(allReleases); - var albumInfo = tuple.Item2; + // Update album ids for trackfiles + var files = _mediaFileService.GetFilesByAlbum(local.Id); + files.ForEach(x => x.AlbumId = target.Id); + _mediaFileService.Update(files); - if (album.ForeignAlbumId != albumInfo.ForeignAlbumId) - { - _logger.Warn( - "Album '{0}' (Album {1}) was replaced with '{2}' (LidarrAPI {3}), because the original was a duplicate.", - album.Title, album.ForeignAlbumId, albumInfo.Title, albumInfo.ForeignAlbumId); - album.ForeignAlbumId = albumInfo.ForeignAlbumId; - } + // Update album ids for history + var items = _historyService.GetByAlbum(local.Id, null); + items.ForEach(x => x.AlbumId = target.Id); + _historyService.UpdateMany(items); - // the only thing written to tags from the album object is the title - forceUpdateFileTags |= album.Title != (albumInfo.Title ?? "Unknown"); - updated |= forceUpdateFileTags; - - album.OldForeignAlbumIds = albumInfo.OldForeignAlbumIds; - album.LastInfoSync = DateTime.UtcNow; - album.CleanTitle = albumInfo.CleanTitle; - album.Title = albumInfo.Title ?? "Unknown"; - album.Overview = albumInfo.Overview.IsNullOrWhiteSpace() ? album.Overview : albumInfo.Overview; - album.Disambiguation = albumInfo.Disambiguation; - album.AlbumType = albumInfo.AlbumType; - album.SecondaryTypes = albumInfo.SecondaryTypes; - album.Genres = albumInfo.Genres; - album.Images = albumInfo.Images.Any() ? albumInfo.Images : album.Images; - album.Links = albumInfo.Links; - album.ReleaseDate = albumInfo.ReleaseDate; - album.Ratings = albumInfo.Ratings; - album.AlbumReleases = new List(); - - var remoteReleases = albumInfo.AlbumReleases.Value.DistinctBy(m => m.ForeignReleaseId).ToList(); - var existingReleases = _releaseService.GetReleasesForRefresh(album.Id, remoteReleases.Select(x => x.ForeignReleaseId)); - // Keep track of which existing release we want to end up monitored - var existingToMonitor = existingReleases.Where(x => x.Monitored).OrderByDescending(x => x.TrackCount).FirstOrDefault(); - - var newReleaseList = new List(); - var updateReleaseList = new List(); - var upToDateReleaseList = new List(); - - foreach (var release in remoteReleases) - { - release.AlbumId = album.Id; - release.Album = album; + // Finally delete the old album + _albumService.DeleteMany(new List { local }); - // force to unmonitored, then fix monitored one later - // once we have made sure that it's unique. This make sure - // that we unmonitor anything in database that shouldn't be monitored. - release.Monitored = false; + return UpdateResult.UpdateTags; + } - var releaseToRefresh = existingReleases.SingleOrDefault(r => r.ForeignReleaseId == release.ForeignReleaseId); + protected override Album GetEntityByForeignId(Album local) + { + return _albumService.FindById(local.ForeignAlbumId); + } - if (releaseToRefresh != null) - { - existingReleases.Remove(releaseToRefresh); - - // copy across the db keys and check for equality - release.Id = releaseToRefresh.Id; - release.AlbumId = releaseToRefresh.AlbumId; - - if (!releaseToRefresh.Equals(release)) - { - updateReleaseList.Add(release); - } - else - { - upToDateReleaseList.Add(release); - } - } - else - { - newReleaseList.Add(release); - } - - album.AlbumReleases.Value.Add(release); - } - - var refreshedToMonitor = remoteReleases.SingleOrDefault(x => x.ForeignReleaseId == existingToMonitor?.ForeignReleaseId) ?? - remoteReleases.OrderByDescending(x => x.TrackCount).First(); - refreshedToMonitor.Monitored = true; - - if (upToDateReleaseList.Contains(refreshedToMonitor)) + protected override void SaveEntity(Album local) + { + // Use UpdateMany to avoid firing the album edited event + _albumService.UpdateMany(new List { local }); + } + + protected override void DeleteEntity(Album local, bool deleteFiles) + { + _albumService.DeleteAlbum(local.Id, true); + } + + protected override List GetRemoteChildren(Album remote) + { + return remote.AlbumReleases.Value.DistinctBy(m => m.ForeignReleaseId).ToList(); + } + + protected override List GetLocalChildren(Album entity, List remoteChildren) + { + var children = _releaseService.GetReleasesForRefresh(entity.Id, + remoteChildren.Select(x => x.ForeignReleaseId) + .Concat(remoteChildren.SelectMany(x => x.OldForeignReleaseIds))); + + // Make sure trackfiles point to the new album where we are grabbing a release from another album + var files = new List(); + foreach (var release in children.Where(x => x.AlbumId != entity.Id)) { - // we weren't going to update, but have changed monitored so now need to - upToDateReleaseList.Remove(refreshedToMonitor); - updateReleaseList.Add(refreshedToMonitor); + files.AddRange(_mediaFileService.GetFilesByRelease(release.Id)); } - else if (updateReleaseList.Contains(refreshedToMonitor) && refreshedToMonitor.Equals(existingToMonitor)) + files.ForEach(x => x.AlbumId = entity.Id); + _mediaFileService.Update(files); + + return children; + } + + protected override Tuple > GetMatchingExistingChildren(List existingChildren, AlbumRelease remote) + { + var existingChild = existingChildren.SingleOrDefault(x => x.ForeignReleaseId == remote.ForeignReleaseId); + var mergeChildren = existingChildren.Where(x => remote.OldForeignReleaseIds.Contains(x.ForeignReleaseId)).ToList(); + return Tuple.Create(existingChild, mergeChildren); + } + + protected override void PrepareNewChild(AlbumRelease child, Album entity) + { + child.AlbumId = entity.Id; + child.Album = entity; + } + + protected override void PrepareExistingChild(AlbumRelease local, AlbumRelease remote, Album entity) + { + local.AlbumId = entity.Id; + local.Album = entity; + remote.Id = local.Id; + remote.Album = entity; + remote.AlbumId = entity.Id; + remote.Monitored = local.Monitored; + } + + protected override void AddChildren(List children) + { + _releaseService.InsertMany(children); + } + + private void MonitorSingleRelease(List releases) + { + var monitored = releases.Where(x => x.Monitored).ToList(); + if (!monitored.Any()) { - // we were going to update because Monitored was incorrect but now it matches - // and so no need to update - updateReleaseList.Remove(refreshedToMonitor); - upToDateReleaseList.Add(refreshedToMonitor); + monitored = releases; } - Ensure.That(album.AlbumReleases.Value.Count(x => x.Monitored) == 1).IsTrue(); - - _logger.Debug($"{album} {upToDateReleaseList.Count} releases up to date; Deleting {existingReleases.Count}, Updating {updateReleaseList.Count}, Adding {newReleaseList.Count} releases."); + var toMonitor = monitored.OrderByDescending(x => _mediaFileService.GetFilesByRelease(x.Id).Count) + .ThenByDescending(x => x.TrackCount) + .First(); - // before deleting anything, remove musicbrainz ids for things we are deleting - _audioTagService.RemoveMusicBrainzTags(existingReleases); - - _releaseService.DeleteMany(existingReleases); - _releaseService.UpdateMany(updateReleaseList); - _releaseService.InsertMany(newReleaseList); - - // if we have updated a monitored release, refresh all file tags - forceUpdateFileTags |= updateReleaseList.Any(x => x.Monitored); - updated |= existingReleases.Any() || updateReleaseList.Any() || newReleaseList.Any(); + releases.ForEach(x => x.Monitored = false); + toMonitor.Monitored = true; + } - updated |= _refreshTrackService.RefreshTrackInfo(album, forceUpdateFileTags); - _albumService.UpdateMany(new List{album}); + protected override bool RefreshChildren(SortedChildren localChildren, List remoteChildren, bool forceChildRefresh, bool forceUpdateFileTags) + { + var refreshList = localChildren.All; + + // make sure only one of the releases ends up monitored + localChildren.Old.ForEach(x => x.Monitored = false); + MonitorSingleRelease(localChildren.Future); - _logger.Debug("Finished album refresh for {0}", album.Title); + refreshList.ForEach(x => _logger.Trace($"release: {x} monitored: {x.Monitored}")); + + return _refreshAlbumReleaseService.RefreshEntityInfo(refreshList, remoteChildren, forceChildRefresh, forceUpdateFileTags); + } + public bool RefreshAlbumInfo(List albums, List remoteAlbums, bool forceAlbumRefresh, bool forceUpdateFileTags) + { + bool updated = false; + foreach (var album in albums) + { + if (forceAlbumRefresh || _checkIfAlbumShouldBeRefreshed.ShouldRefresh(album)) + { + updated |= RefreshAlbumInfo(album, remoteAlbums, forceUpdateFileTags); + } + } return updated; } + public bool RefreshAlbumInfo(Album album, List remoteAlbums, bool forceUpdateFileTags) + { + return RefreshEntityInfo(album, remoteAlbums, true, forceUpdateFileTags); + } + public void Execute(RefreshAlbumCommand message) { if (message.AlbumId.HasValue) { var album = _albumService.GetAlbum(message.AlbumId.Value); var artist = _artistService.GetArtistByMetadataId(album.ArtistMetadataId); - var updated = RefreshAlbumInfo(album, false); + var updated = RefreshAlbumInfo(album, null, false); if (updated) { _eventAggregator.PublishEvent(new ArtistUpdatedEvent(artist)); diff --git a/src/NzbDrone.Core/Music/RefreshArtistService.cs b/src/NzbDrone.Core/Music/RefreshArtistService.cs index 0a96d4f78..82a7d47af 100644 --- a/src/NzbDrone.Core/Music/RefreshArtistService.cs +++ b/src/NzbDrone.Core/Music/RefreshArtistService.cs @@ -13,21 +13,22 @@ using System; using System.Collections.Generic; using System.IO; using System.Linq; -using System.Text; using NzbDrone.Core.ImportLists.Exclusions; +using NzbDrone.Common.EnsureThat; +using NzbDrone.Core.History; namespace NzbDrone.Core.Music { - public class RefreshArtistService : IExecute + public class RefreshArtistService : RefreshEntityServiceBase, IExecute { private readonly IProvideArtistInfo _artistInfo; private readonly IArtistService _artistService; - private readonly IAddAlbumService _addAlbumService; + private readonly IArtistMetadataRepository _artistMetadataRepository; private readonly IAlbumService _albumService; private readonly IRefreshAlbumService _refreshAlbumService; - private readonly IRefreshTrackService _refreshTrackService; - private readonly IAudioTagService _audioTagService; private readonly IEventAggregator _eventAggregator; + private readonly IMediaFileService _mediaFileService; + private readonly IHistoryService _historyService; private readonly IDiskScanService _diskScanService; private readonly ICheckIfArtistShouldBeRefreshed _checkIfArtistShouldBeRefreshed; private readonly IConfigService _configService; @@ -36,154 +37,219 @@ namespace NzbDrone.Core.Music public RefreshArtistService(IProvideArtistInfo artistInfo, IArtistService artistService, - IAddAlbumService addAlbumService, + IArtistMetadataRepository artistMetadataRepository, IAlbumService albumService, IRefreshAlbumService refreshAlbumService, - IRefreshTrackService refreshTrackService, - IAudioTagService audioTagService, IEventAggregator eventAggregator, + IMediaFileService mediaFileService, + IHistoryService historyService, IDiskScanService diskScanService, ICheckIfArtistShouldBeRefreshed checkIfArtistShouldBeRefreshed, IConfigService configService, IImportListExclusionService importListExclusionService, Logger logger) + : base(logger, artistMetadataRepository) { _artistInfo = artistInfo; _artistService = artistService; - _addAlbumService = addAlbumService; + _artistMetadataRepository = artistMetadataRepository; _albumService = albumService; _refreshAlbumService = refreshAlbumService; - _refreshTrackService = refreshTrackService; - _audioTagService = audioTagService; _eventAggregator = eventAggregator; + _mediaFileService = mediaFileService; + _historyService = historyService; _diskScanService = diskScanService; _checkIfArtistShouldBeRefreshed = checkIfArtistShouldBeRefreshed; _configService = configService; _importListExclusionService = importListExclusionService; _logger = logger; } - - private bool RefreshArtistInfo(Artist artist, bool forceAlbumRefresh) + + protected override RemoteData GetRemoteData(Artist local, List remote) { - _logger.ProgressInfo("Updating Info for {0}", artist.Name); - bool updated = false; - - Artist artistInfo; - + var result = new RemoteData(); try { - artistInfo = _artistInfo.GetArtistInfo(artist.Metadata.Value.ForeignArtistId, artist.MetadataProfileId); + result.Entity = _artistInfo.GetArtistInfo(local.Metadata.Value.ForeignArtistId, local.MetadataProfileId); + result.Metadata = new List { result.Entity.Metadata.Value }; } catch (ArtistNotFoundException) { - _logger.Error($"Artist {artist} was not found, it may have been removed from Metadata sources."); - return updated; + _logger.Error($"Could not find artist with id {local.Metadata.Value.ForeignArtistId}"); } - var forceUpdateFileTags = artist.Name != artistInfo.Name; - updated |= forceUpdateFileTags; + return result; + } + + protected override bool ShouldDelete(Artist local) + { + return !_mediaFileService.GetFilesByArtist(local.Id).Any(); + } + + protected override void LogProgress(Artist local) + { + _logger.ProgressInfo("Updating Info for {0}", local.Name); + } + + protected override bool IsMerge(Artist local, Artist remote) + { + return local.ArtistMetadataId != remote.Metadata.Value.Id; + } - if (artist.Metadata.Value.ForeignArtistId != artistInfo.Metadata.Value.ForeignArtistId) + protected override UpdateResult UpdateEntity(Artist local, Artist remote) + { + UpdateResult result = UpdateResult.None; + + if(!local.Metadata.Value.Equals(remote.Metadata.Value)) { - _logger.Warn($"Artist {artist} was replaced with {artistInfo} because the original was a duplicate."); - - // Update list exclusion if one exists - var importExclusion = _importListExclusionService.FindByForeignId(artist.Metadata.Value.ForeignArtistId); - - if (importExclusion != null) - { - importExclusion.ForeignId = artistInfo.Metadata.Value.ForeignArtistId; - _importListExclusionService.Update(importExclusion); - } - - artist.Metadata.Value.ForeignArtistId = artistInfo.Metadata.Value.ForeignArtistId; - forceUpdateFileTags = true; - updated = true; + result = UpdateResult.UpdateTags; } - artist.Metadata.Value.ApplyChanges(artistInfo.Metadata.Value); - artist.CleanName = artistInfo.CleanName; - artist.SortName = artistInfo.SortName; - artist.LastInfoSync = DateTime.UtcNow; + local.CleanName = remote.CleanName; + local.SortName = remote.SortName; + local.LastInfoSync = DateTime.UtcNow; try { - artist.Path = new DirectoryInfo(artist.Path).FullName; - artist.Path = artist.Path.GetActualCasing(); + local.Path = new DirectoryInfo(local.Path).FullName; + local.Path = local.Path.GetActualCasing(); } catch (Exception e) { - _logger.Warn(e, "Couldn't update artist path for " + artist.Path); + _logger.Warn(e, "Couldn't update artist path for " + local.Path); } - var remoteAlbums = artistInfo.Albums.Value.DistinctBy(m => m.ForeignAlbumId).ToList(); + return result; + } + + protected override UpdateResult MoveEntity(Artist local, Artist remote) + { + _logger.Debug($"Updating MusicBrainz id for {local} to {remote}"); + + // We are moving from one metadata to another (will already have been poplated) + local.ArtistMetadataId = remote.Metadata.Value.Id; + local.Metadata = remote.Metadata.Value; - // Get list of DB current db albums for artist - var existingAlbums = _albumService.GetAlbumsForRefresh(artist.ArtistMetadataId, remoteAlbums.Select(x => x.ForeignAlbumId)); - var newAlbumsList = new List(); - var updateAlbumsList = new List(); + // Update list exclusion if one exists + var importExclusion = _importListExclusionService.FindByForeignId(local.Metadata.Value.ForeignArtistId); - // Cycle thru albums - foreach (var album in remoteAlbums) + if (importExclusion != null) { - // Check for album in existing albums, if not set properties and add to new list - var albumToRefresh = existingAlbums.SingleOrDefault(s => s.ForeignAlbumId == album.ForeignAlbumId); + importExclusion.ForeignId = remote.Metadata.Value.ForeignArtistId; + _importListExclusionService.Update(importExclusion); + } - if (albumToRefresh != null) - { - albumToRefresh.Artist = artist; - existingAlbums.Remove(albumToRefresh); - updateAlbumsList.Add(albumToRefresh); - } - else + // Do the standard update + UpdateEntity(local, remote); + + // We know we need to update tags as artist id has changed + return UpdateResult.UpdateTags; + } + + protected override UpdateResult MergeEntity(Artist local, Artist target, Artist remote) + { + _logger.Warn($"Artist {local} was replaced with {remote} because the original was a duplicate."); + + // Update list exclusion if one exists + var importExclusionLocal = _importListExclusionService.FindByForeignId(local.Metadata.Value.ForeignArtistId); + + if (importExclusionLocal != null) + { + var importExclusionTarget = _importListExclusionService.FindByForeignId(target.Metadata.Value.ForeignArtistId); + if (importExclusionTarget == null) { - album.Artist = artist; - newAlbumsList.Add(album); + importExclusionLocal.ForeignId = remote.Metadata.Value.ForeignArtistId; + _importListExclusionService.Update(importExclusionLocal); } } - _logger.Debug("{0} Deleting {1}, Updating {2}, Adding {3} albums", - artist, existingAlbums.Count, updateAlbumsList.Count, newAlbumsList.Count); + // move any albums over to the new artist and remove the local artist + var albums = _albumService.GetAlbumsByArtist(local.Id); + albums.ForEach(x => x.ArtistMetadataId = target.ArtistMetadataId); + _albumService.UpdateMany(albums); + _artistService.DeleteArtist(local.Id, false); - // before deleting anything, remove musicbrainz ids for things we are deleting - _audioTagService.RemoveMusicBrainzTags(existingAlbums); + // Update history entries to new id + var items = _historyService.GetByArtist(local.Id, null); + items.ForEach(x => x.ArtistId = target.Id); + _historyService.UpdateMany(items); - // Delete old albums first - this avoids errors if albums have been merged and we'll - // end up trying to duplicate an existing release under a new album - _albumService.DeleteMany(existingAlbums); - - // Update new albums with artist info and correct monitored status - newAlbumsList = UpdateAlbums(artist, newAlbumsList); - _addAlbumService.AddAlbums(newAlbumsList); - - updated |= existingAlbums.Any() || newAlbumsList.Any(); - - updated |= _refreshAlbumService.RefreshAlbumInfo(updateAlbumsList, forceAlbumRefresh, forceUpdateFileTags); + // We know we need to update tags as artist id has changed + return UpdateResult.UpdateTags; + } - // Do this last so artist only marked as refreshed if refresh of tracks / albums completed successfully - _artistService.UpdateArtist(artist); + protected override Artist GetEntityByForeignId(Artist local) + { + return _artistService.FindById(local.ForeignArtistId); + } - _eventAggregator.PublishEvent(new AlbumInfoRefreshedEvent(artist, newAlbumsList, updateAlbumsList)); + protected override void SaveEntity(Artist local) + { + _artistService.UpdateArtist(local); + } - if (updated) - { - _eventAggregator.PublishEvent(new ArtistUpdatedEvent(artist)); - } + protected override void DeleteEntity(Artist local, bool deleteFiles) + { + _artistService.DeleteArtist(local.Id, true); + } - _logger.Debug("Finished artist refresh for {0}", artist.Name); + protected override List GetRemoteChildren(Artist remote) + { + return remote.Albums.Value.DistinctBy(m => m.ForeignAlbumId).ToList(); + } - return updated; + protected override List GetLocalChildren(Artist entity, List remoteChildren) + { + return _albumService.GetAlbumsForRefresh(entity.ArtistMetadataId, + remoteChildren.Select(x => x.ForeignAlbumId) + .Concat(remoteChildren.SelectMany(x => x.OldForeignAlbumIds))); + } + + protected override Tuple > GetMatchingExistingChildren(List existingChildren, Album remote) + { + var existingChild = existingChildren.SingleOrDefault(x => x.ForeignAlbumId == remote.ForeignAlbumId); + var mergeChildren = existingChildren.Where(x => remote.OldForeignAlbumIds.Contains(x.ForeignAlbumId)).ToList(); + return Tuple.Create(existingChild, mergeChildren); + } + + protected override void PrepareNewChild(Album child, Artist entity) + { + child.Artist = entity; + child.ArtistMetadata = entity.Metadata.Value; + child.ArtistMetadataId = entity.Metadata.Value.Id; + child.Added = DateTime.UtcNow; + child.LastInfoSync = DateTime.MinValue; + child.ProfileId = entity.QualityProfileId; + child.Monitored = entity.Monitored; + } + + protected override void PrepareExistingChild(Album local, Album remote, Artist entity) + { + local.Artist = entity; + local.ArtistMetadata = entity.Metadata.Value; + local.ArtistMetadataId = entity.Metadata.Value.Id; + } + + protected override void AddChildren(List children) + { + _albumService.InsertMany(children); } - private List UpdateAlbums(Artist artist, List albumsToUpdate) + protected override bool RefreshChildren(SortedChildren localChildren, List remoteChildren, bool forceChildRefresh, bool forceUpdateFileTags) { - foreach (var album in albumsToUpdate) - { - album.ProfileId = artist.QualityProfileId; - album.Monitored = artist.Monitored; - } + // we always want to end up refreshing the albums since we don't get have proper data + Ensure.That(localChildren.UpToDate.Count, () => localChildren.UpToDate.Count).IsLessThanOrEqualTo(0); + return _refreshAlbumService.RefreshAlbumInfo(localChildren.All, remoteChildren, forceChildRefresh, forceUpdateFileTags); + } + + protected override void PublishEntityUpdatedEvent(Artist entity) + { + _eventAggregator.PublishEvent(new ArtistUpdatedEvent(entity)); + } - return albumsToUpdate; + protected override void PublishChildrenUpdatedEvent(Artist entity, List newChildren, List updateChildren) + { + _eventAggregator.PublishEvent(new AlbumInfoRefreshedEvent(entity, newChildren, updateChildren)); } private void RescanArtist(Artist artist, bool isNew, CommandTrigger trigger, bool infoUpdated) @@ -239,7 +305,7 @@ namespace NzbDrone.Core.Music bool updated = false; try { - updated = RefreshArtistInfo(artist, true); + updated = RefreshEntityInfo(artist, null, true, false); RescanArtist(artist, isNew, trigger, updated); } catch (Exception e) @@ -262,7 +328,7 @@ namespace NzbDrone.Core.Music bool updated = false; try { - updated = RefreshArtistInfo(artist, manualTrigger); + updated = RefreshEntityInfo(artist, null, manualTrigger, false); } catch (Exception e) { @@ -271,7 +337,6 @@ namespace NzbDrone.Core.Music RescanArtist(artist, false, trigger, updated); } - else { _logger.Info("Skipping refresh of artist: {0}", artist.Name); diff --git a/src/NzbDrone.Core/Music/RefreshEntityServiceBase.cs b/src/NzbDrone.Core/Music/RefreshEntityServiceBase.cs new file mode 100644 index 000000000..bbc7b8e49 --- /dev/null +++ b/src/NzbDrone.Core/Music/RefreshEntityServiceBase.cs @@ -0,0 +1,277 @@ +using NLog; +using NzbDrone.Common.Extensions; +using System; +using System.Collections.Generic; +using System.Linq; + +namespace NzbDrone.Core.Music +{ + public abstract class RefreshEntityServiceBase + { + private readonly Logger _logger; + private readonly IArtistMetadataRepository _artistMetadataRepository; + + public RefreshEntityServiceBase(Logger logger, + IArtistMetadataRepository artistMetadataRepository) + { + _logger = logger; + _artistMetadataRepository = artistMetadataRepository; + } + + public enum UpdateResult + { + None, + Standard, + UpdateTags + }; + + public class SortedChildren + { + public SortedChildren() + { + UpToDate = new List(); + Added = new List(); + Updated = new List(); + Merged = new List >(); + Deleted = new List(); + } + + public List UpToDate { get; set; } + public List Added { get; set; } + public List Updated { get; set; } + public List > Merged { get; set; } + public List Deleted { get; set; } + + public List All => UpToDate.Concat(Added).Concat(Updated).Concat(Merged.Select(x => x.Item1)).Concat(Deleted).ToList(); + public List Future => UpToDate.Concat(Added).Concat(Updated).ToList(); + public List Old => Merged.Select(x => x.Item1).Concat(Deleted).ToList(); + } + + public class RemoteData + { + public Entity Entity { get; set; } + public List Metadata { get; set; } + } + + protected virtual void LogProgress(Entity local) + { + } + + protected abstract RemoteData GetRemoteData(Entity local, List remote); + + protected virtual void EnsureNewParent(Entity local, Entity remote) + { + return; + } + + protected abstract bool IsMerge(Entity local, Entity remote); + + protected virtual bool ShouldDelete(Entity local) + { + return true; + } + + protected abstract UpdateResult UpdateEntity(Entity local, Entity remote); + + protected virtual UpdateResult MoveEntity(Entity local, Entity remote) + { + return UpdateEntity(local, remote); + } + + protected virtual UpdateResult MergeEntity(Entity local, Entity target, Entity remote) + { + DeleteEntity(local, true); + return UpdateResult.UpdateTags; + } + + protected abstract Entity GetEntityByForeignId(Entity local); + protected abstract void SaveEntity(Entity local); + protected abstract void DeleteEntity(Entity local, bool deleteFiles); + + protected abstract List GetRemoteChildren(Entity remote); + protected abstract List GetLocalChildren(Entity entity, List remoteChildren); + protected abstract Tuple > GetMatchingExistingChildren(List existingChildren, Child remote); + + protected abstract void PrepareNewChild(Child remoteChild, Entity entity); + protected abstract void PrepareExistingChild(Child existingChild, Child remoteChild, Entity entity); + protected abstract void AddChildren(List children); + protected abstract bool RefreshChildren(SortedChildren localChildren, List remoteChildren, bool forceChildRefresh, bool forceUpdateFileTags); + + protected virtual void PublishEntityUpdatedEvent(Entity entity) + { + } + + protected virtual void PublishChildrenUpdatedEvent(Entity entity, List newChildren, List updateChildren) + { + } + + public bool RefreshEntityInfo(Entity local, List remoteList, bool forceChildRefresh, bool forceUpdateFileTags) + { + bool updated = false; + + LogProgress(local); + + var data = GetRemoteData(local, remoteList); + var remote = data.Entity; + + if (remote == null) + { + if (ShouldDelete(local)) + { + _logger.Warn($"{typeof(Entity).Name} {local} not found in metadata and is being deleted"); + DeleteEntity(local, true); + return false; + } + else + { + _logger.Error($"{typeof(Entity).Name} {local} was not found, it may have been removed from Metadata sources."); + return false; + } + } + + if (data.Metadata != null) + { + var metadataResult = UpdateArtistMetadata(data.Metadata); + updated |= metadataResult >= UpdateResult.Standard; + forceUpdateFileTags |= metadataResult == UpdateResult.UpdateTags; + } + + // Validate that the parent object exists (remote data might specify a different one) + EnsureNewParent(local, remote); + + UpdateResult result; + if (IsMerge(local, remote)) + { + // get entity we're merging into + var target = GetEntityByForeignId(remote); + + if (target == null) + { + _logger.Trace($"Moving {typeof(Entity).Name} {local} to {remote}"); + result = MoveEntity(local, remote); + } + else + { + _logger.Trace($"Merging {typeof(Entity).Name} {local} into {target}"); + result = MergeEntity(local, target, remote); + + // having merged local into target, do update for target using remote + local = target; + } + + // Save the entity early so that children see the updated ids + SaveEntity(local); + } + else + { + _logger.Trace($"Updating {typeof(Entity).Name} {local}"); + result = UpdateEntity(local, remote); + } + + updated |= result >= UpdateResult.Standard; + forceUpdateFileTags |= result == UpdateResult.UpdateTags; + + _logger.Trace($"updated: {updated} forceUpdateFileTags: {forceUpdateFileTags}"); + + var remoteChildren = GetRemoteChildren(remote); + updated |= SortChildren(local, remoteChildren, forceChildRefresh, forceUpdateFileTags); + + // Do this last so entity only marked as refreshed if refresh of children completed successfully + _logger.Trace($"Saving {typeof(Entity).Name} {local}"); + SaveEntity(local); + + if (updated) + { + PublishEntityUpdatedEvent(local); + } + + _logger.Debug($"Finished {typeof(Entity).Name} refresh for {local}"); + + return updated; + } + + public UpdateResult UpdateArtistMetadata(List data) + { + var remoteMetadata = data.DistinctBy(x => x.ForeignArtistId).ToList(); + var updated = _artistMetadataRepository.UpsertMany(data); + return updated ? UpdateResult.UpdateTags : UpdateResult.None; + } + + public bool RefreshEntityInfo(List localList, List remoteList, bool forceChildRefresh, bool forceUpdateFileTags) + { + bool updated = false; + foreach (var entity in localList) + { + updated |= RefreshEntityInfo(entity, remoteList, forceChildRefresh, forceUpdateFileTags); + } + return updated; + } + + protected bool SortChildren(Entity entity, List remoteChildren, bool forceChildRefresh, bool forceUpdateFileTags) + { + // Get existing children (and children to be) from the database + var localChildren = GetLocalChildren(entity, remoteChildren); + + var sortedChildren = new SortedChildren(); + sortedChildren.Deleted.AddRange(localChildren); + + // Cycle through children + foreach (var remoteChild in remoteChildren) + { + // Check for child in existing children, if not set properties and add to new list + var tuple = GetMatchingExistingChildren(localChildren, remoteChild); + var existingChild = tuple.Item1; + var mergedChildren = tuple.Item2; + + if (existingChild != null) + { + sortedChildren.Deleted.Remove(existingChild); + + PrepareExistingChild(existingChild, remoteChild, entity); + + if (existingChild.Equals(remoteChild)) + { + sortedChildren.UpToDate.Add(existingChild); + } + else + { + sortedChildren.Updated.Add(existingChild); + } + + // note the children that are going to be merged into existingChild + foreach (var child in mergedChildren) + { + sortedChildren.Merged.Add(Tuple.Create(child, existingChild)); + sortedChildren.Deleted.Remove(child); + } + } + else + { + PrepareNewChild(remoteChild, entity); + sortedChildren.Added.Add(remoteChild); + + // note the children that will be merged into remoteChild (once added) + foreach (var child in mergedChildren) + { + sortedChildren.Merged.Add(Tuple.Create(child, existingChild)); + sortedChildren.Deleted.Remove(child); + } + + } + } + + _logger.Debug("{0} {1} {2}s up to date. Adding {3}, Updating {4}, Merging {5}, Deleting {6}.", + entity, sortedChildren.UpToDate.Count, typeof(Child).Name.ToLower(), + sortedChildren.Added.Count, sortedChildren.Updated.Count, sortedChildren.Merged.Count, sortedChildren.Deleted.Count); + + // Add in the new children (we have checked that foreign IDs don't clash) + AddChildren(sortedChildren.Added); + + // now trigger updates + var updated = RefreshChildren(sortedChildren, remoteChildren, forceChildRefresh, forceUpdateFileTags); + + PublishChildrenUpdatedEvent(entity, sortedChildren.Added, sortedChildren.Updated); + return updated; + } + } +} diff --git a/src/NzbDrone.Core/Music/RefreshTrackService.cs b/src/NzbDrone.Core/Music/RefreshTrackService.cs index f193ea7b4..5e3a3b5b4 100644 --- a/src/NzbDrone.Core/Music/RefreshTrackService.cs +++ b/src/NzbDrone.Core/Music/RefreshTrackService.cs @@ -1,7 +1,5 @@ using NLog; -using NzbDrone.Common.Extensions; using NzbDrone.Core.MediaFiles; -using NzbDrone.Core.Messaging.Events; using System; using System.Collections.Generic; using System.Linq; @@ -10,127 +8,70 @@ namespace NzbDrone.Core.Music { public interface IRefreshTrackService { - bool RefreshTrackInfo(Album rg, bool forceUpdateFileTags); + bool RefreshTrackInfo(List add, List update, List > merge, List delete, List upToDate, List remoteTracks, bool forceUpdateFileTags); } public class RefreshTrackService : IRefreshTrackService { private readonly ITrackService _trackService; - private readonly IAlbumService _albumService; - private readonly IMediaFileService _mediaFileService; private readonly IAudioTagService _audioTagService; - private readonly IEventAggregator _eventAggregator; private readonly Logger _logger; public RefreshTrackService(ITrackService trackService, - IAlbumService albumService, - IMediaFileService mediaFileService, IAudioTagService audioTagService, - IEventAggregator eventAggregator, Logger logger) { _trackService = trackService; - _albumService = albumService; - _mediaFileService = mediaFileService; _audioTagService = audioTagService; - _eventAggregator = eventAggregator; _logger = logger; } - public bool RefreshTrackInfo(Album album, bool forceUpdateFileTags) + public bool RefreshTrackInfo(List add, List update, List > merge, List delete, List upToDate, List remoteTracks, bool forceUpdateFileTags) { - _logger.Info("Starting track info refresh for: {0}", album); - var successCount = 0; - var failCount = 0; - bool updated = false; + var updateList = new List(); - foreach (var release in album.AlbumReleases.Value) + // for tracks that need updating, just grab the remote track and set db ids + foreach (var trackToUpdate in update) { - var remoteTracks = release.Tracks.Value.DistinctBy(m => m.ForeignTrackId).ToList(); - var existingTracks = _trackService.GetTracksForRefresh(release.Id, remoteTracks.Select(x => x.ForeignTrackId)); - - var updateList = new List(); - var newList = new List(); - var upToDateList = new List(); + var track = remoteTracks.Single(e => e.ForeignTrackId == trackToUpdate.ForeignTrackId); + + // copy across the db keys to the remote track and check if we need to update + track.Id = trackToUpdate.Id; + track.TrackFileId = trackToUpdate.TrackFileId; + // make sure title is not null + track.Title = track.Title ?? "Unknown"; + updateList.Add(track); + } + + // Move trackfiles from merged entities into new one + foreach (var item in merge) + { + var trackToMerge = item.Item1; + var mergeTarget = item.Item2; - foreach (var track in remoteTracks) + if (mergeTarget.TrackFileId == 0) { - track.AlbumRelease = release; - track.AlbumReleaseId = release.Id; - // the artist metadata will have been inserted by RefreshAlbumInfo so the Id will now be populated - track.ArtistMetadataId = track.ArtistMetadata.Value.Id; - - try - { - var trackToUpdate = existingTracks.SingleOrDefault(e => e.ForeignTrackId == track.ForeignTrackId); - if (trackToUpdate != null) - { - existingTracks.Remove(trackToUpdate); - - // populate albumrelease for later - trackToUpdate.AlbumRelease = release; - - // copy across the db keys to the remote track and check if we need to update - track.Id = trackToUpdate.Id; - track.TrackFileId = trackToUpdate.TrackFileId; - // make sure title is not null - track.Title = track.Title ?? "Unknown"; - - if (!trackToUpdate.Equals(track)) - { - updateList.Add(track); - } - else - { - upToDateList.Add(track); - } - } - else - { - newList.Add(track); - } - - successCount++; - } - catch (Exception e) - { - _logger.Fatal(e, "An error has occurred while updating track info for album {0}. {1}", album, track); - failCount++; - } + mergeTarget.TrackFileId = trackToMerge.TrackFileId; } - // if any tracks with files are deleted, strip out the MB tags from the metadata - // so that we stand a chance of matching next time - _audioTagService.RemoveMusicBrainzTags(existingTracks); - - var tagsToUpdate = updateList; - if (forceUpdateFileTags) + if (!updateList.Contains(mergeTarget)) { - _logger.Debug("Forcing tag update due to Artist/Album/Release updates"); - tagsToUpdate = updateList.Concat(upToDateList).ToList(); + updateList.Add(mergeTarget); } - _audioTagService.SyncTags(tagsToUpdate); - - _logger.Debug($"{release}: {upToDateList.Count} tracks up to date; Deleting {existingTracks.Count}, Updating {updateList.Count}, Adding {newList.Count} tracks."); - - _trackService.DeleteMany(existingTracks); - _trackService.UpdateMany(updateList); - _trackService.InsertMany(newList); - - updated |= existingTracks.Any() || updateList.Any() || newList.Any(); } - if (failCount != 0) + var tagsToUpdate = updateList; + if (forceUpdateFileTags) { - _logger.Info("Finished track refresh for album: {0}. Successful: {1} - Failed: {2} ", - album.Title, successCount, failCount); - } - else - { - _logger.Info("Finished track refresh for album: {0}.", album); + _logger.Debug("Forcing tag update due to Artist/Album/Release updates"); + tagsToUpdate = updateList.Concat(upToDate).ToList(); } + _audioTagService.SyncTags(tagsToUpdate); + + _trackService.DeleteMany(delete.Concat(merge.Select(x => x.Item1)).ToList()); + _trackService.UpdateMany(updateList); - return updated; + return delete.Any() || updateList.Any() || merge.Any(); } } } diff --git a/src/NzbDrone.Core/Music/Track.cs b/src/NzbDrone.Core/Music/Track.cs index dad9cb7dc..d0a1bae23 100644 --- a/src/NzbDrone.Core/Music/Track.cs +++ b/src/NzbDrone.Core/Music/Track.cs @@ -42,7 +42,7 @@ namespace NzbDrone.Core.Music // These are retained for compatibility // TODO: Remove set, bodged in because tests expect this to be writable - public int AlbumId { get { return AlbumRelease.Value?.Album.Value?.Id ?? 0; } set { /* empty */ } } + public int AlbumId { get { return AlbumRelease?.Value?.Album?.Value?.Id ?? 0; } set { /* empty */ } } public Album Album { get; set; } public override string ToString() diff --git a/src/NzbDrone.Core/NzbDrone.Core.csproj b/src/NzbDrone.Core/NzbDrone.Core.csproj index c6b7edf98..9f2d9badd 100644 --- a/src/NzbDrone.Core/NzbDrone.Core.csproj +++ b/src/NzbDrone.Core/NzbDrone.Core.csproj @@ -847,7 +847,6 @@ - @@ -898,8 +897,10 @@ + +