From 580641a600d38d45b681689678d546db081e342d Mon Sep 17 00:00:00 2001 From: ta264 Date: Fri, 22 Mar 2019 09:33:48 +0000 Subject: [PATCH] Fixed: Don't attempt to insert duplicate ids or monitor multiple releases (#684) --- .../MusicTests/AddAlbumFixture.cs | 98 +++++++- .../MusicTests/RefreshAlbumServiceFixture.cs | 233 +++++++++++++++++- .../MetadataSource/SkyHook/SkyHookProxy.cs | 1 - src/NzbDrone.Core/Music/AddAlbumService.cs | 87 +++++-- src/NzbDrone.Core/Music/AlbumService.cs | 25 +- .../Music/RefreshAlbumService.cs | 49 ++-- 6 files changed, 430 insertions(+), 63 deletions(-) diff --git a/src/NzbDrone.Core.Test/MusicTests/AddAlbumFixture.cs b/src/NzbDrone.Core.Test/MusicTests/AddAlbumFixture.cs index b7cbe2ddf..3b646dbec 100644 --- a/src/NzbDrone.Core.Test/MusicTests/AddAlbumFixture.cs +++ b/src/NzbDrone.Core.Test/MusicTests/AddAlbumFixture.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using FizzWare.NBuilder; using FluentAssertions; using FluentValidation; @@ -20,8 +21,8 @@ namespace NzbDrone.Core.Test.MusicTests { private Album _fakeAlbum; private AlbumRelease _fakeRelease; + private List _fakeArtists; private readonly string _fakeArtistForeignId = "xxx-xxx-xxx"; - private readonly List _fakeArtists = new List { new ArtistMetadata() }; [SetUp] public void Setup() @@ -34,6 +35,19 @@ namespace NzbDrone.Core.Test.MusicTests .Build(); _fakeRelease.Tracks = new List(); _fakeAlbum.AlbumReleases = new List {_fakeRelease}; + + _fakeArtists = Builder.CreateListOfSize(1) + .TheFirst(1) + .With(x => x.ForeignArtistId = _fakeArtistForeignId) + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(It.IsAny(), It.IsAny>())) + .Returns(new List()); + + Mocker.GetMock() + .Setup(x => x.FindById(_fakeArtistForeignId)) + .Returns(_fakeArtists[0]); } private void GivenValidAlbum(string lidarrId) @@ -74,5 +88,87 @@ namespace NzbDrone.Core.Test.MusicTests ExceptionVerification.ExpectedErrors(1); } + + [Test] + public void should_not_add_duplicate_releases() + { + var newAlbum = Builder.CreateNew().Build(); + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.AlbumId = newAlbum.Id) + .TheFirst(4) + .With(x => x.ForeignReleaseId = "DuplicateId1") + .TheLast(1) + .With(x => x.ForeignReleaseId = "DuplicateId2") + .Build() as List; + + newAlbum.AlbumReleases = releases; + + var existingReleases = Builder.CreateListOfSize(1) + .All() + .With(x => x.ForeignReleaseId = "DuplicateId2") + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(newAlbum.Id, It.IsAny>())) + .Returns(existingReleases); + + var updatedReleases = Subject.AddAlbumReleases(newAlbum); + + updatedReleases.Should().HaveCount(7); + + 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() == 6 && + !l.Select(r => r.ForeignReleaseId).Contains("DuplicateId2"))), + Times.Once()); + } + + [Test] + public void should_only_add_one_monitored_release_ignoring_skyhook() + { + var newAlbum = Builder.CreateNew().Build(); + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.Monitored = true) + .Build() as List; + + newAlbum.AlbumReleases = releases; + + var updatedReleases = Subject.AddAlbumReleases(newAlbum); + + updatedReleases.Count(x => x.Monitored).Should().Be(1); + } + + [Test] + public void should_only_add_one_monitored_release_combining_with_existing() + { + var newAlbum = Builder.CreateNew().Build(); + + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.Monitored = false) + .Build() as List; + releases[1].Monitored = true; + + newAlbum.AlbumReleases = releases; + + var existingReleases = Builder.CreateListOfSize(1) + .All() + .With(x => x.ForeignReleaseId = releases[0].ForeignReleaseId) + .With(x => x.Monitored = true) + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(newAlbum.Id, It.IsAny>())) + .Returns(existingReleases); + + var updatedReleases = Subject.AddAlbumReleases(newAlbum); + + updatedReleases.Count(x => x.Monitored).Should().Be(1); + } } } diff --git a/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs b/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs index c39a90caf..46cbb222e 100644 --- a/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs +++ b/src/NzbDrone.Core.Test/MusicTests/RefreshAlbumServiceFixture.cs @@ -143,12 +143,235 @@ namespace NzbDrone.Core.Test.MusicTests } [Test] - public void should_remove_items_from_list() + public void should_not_add_duplicate_releases() { - var releases = Builder.CreateListOfSize(2).Build(); - var release = releases[0]; - releases.Remove(release); - releases.Should().HaveCount(1); + var newAlbum = Builder.CreateNew().Build(); + // this is required because RefreshAlbumInfo will edit the album passed in + var albumCopy = Builder.CreateNew().Build(); + + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.AlbumId = newAlbum.Id) + .With(x => x.Monitored = true) + .TheFirst(4) + .With(x => x.ForeignReleaseId = "DuplicateId1") + .TheLast(1) + .With(x => x.ForeignReleaseId = "DuplicateId2") + .Build() as List; + + newAlbum.AlbumReleases = releases; + albumCopy.AlbumReleases = releases; + + var existingReleases = Builder.CreateListOfSize(1) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "DuplicateId2") + .With(x => x.Monitored = true) + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(It.IsAny(), It.IsAny>())) + .Returns(existingReleases); + + Mocker.GetMock() + .Setup(x => x.GetAlbumInfo(It.IsAny())) + .Returns(Tuple.Create("dummy string", albumCopy, new List())); + + Subject.RefreshAlbumInfo(newAlbum, 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()); + } + + [TestCase(true, true, 1)] + [TestCase(true, false, 0)] + [TestCase(false, true, 1)] + [TestCase(false, false, 0)] + public void should_only_leave_one_release_monitored(bool skyhookMonitored, bool existingMonitored, int expectedUpdates) + { + var newAlbum = Builder.CreateNew().Build(); + // this is required because RefreshAlbumInfo will edit the album passed in + var albumCopy = Builder.CreateNew().Build(); + + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.AlbumId = newAlbum.Id) + .With(x => x.Monitored = skyhookMonitored) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "ExistingId1") + .TheNext(1) + .With(x => x.ForeignReleaseId = "ExistingId2") + .Build() as List; + + newAlbum.AlbumReleases = releases; + albumCopy.AlbumReleases = releases; + + var existingReleases = Builder.CreateListOfSize(2) + .All() + .With(x => x.Monitored = existingMonitored) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "ExistingId1") + .TheNext(1) + .With(x => x.ForeignReleaseId = "ExistingId2") + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(It.IsAny(), It.IsAny>())) + .Returns(existingReleases); + + Mocker.GetMock() + .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()); + + 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.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(); + // this is required because RefreshAlbumInfo will edit the album passed in + var albumCopy = Builder.CreateNew().Build(); + + // only ExistingId1 is monitored from dummy skyhook + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.AlbumId = newAlbum.Id) + .With(x => x.Monitored = false) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "ExistingId1") + .With(x => x.Monitored = true) + .TheNext(1) + .With(x => x.ForeignReleaseId = "ExistingId2") + .Build() as List; + + newAlbum.AlbumReleases = releases; + albumCopy.AlbumReleases = releases; + + // ExistingId2 is monitored in DB + var existingReleases = Builder.CreateListOfSize(2) + .All() + .With(x => x.Monitored = false) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "ExistingId1") + .TheNext(1) + .With(x => x.ForeignReleaseId = "ExistingId2") + .With(x => x.Monitored = true) + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(It.IsAny(), It.IsAny>())) + .Returns(existingReleases); + + Mocker.GetMock() + .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"); + + 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()); + } + + [Test] + public void refreshing_album_should_change_monitored_release_if_monitored_release_deleted() + { + var newAlbum = Builder.CreateNew().Build(); + // this is required because RefreshAlbumInfo will edit the album passed in + var albumCopy = Builder.CreateNew().Build(); + + // Only existingId1 monitored in skyhook. ExistingId2 is missing + var releases = Builder.CreateListOfSize(10) + .All() + .With(x => x.AlbumId = newAlbum.Id) + .With(x => x.Monitored = false) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "ExistingId1") + .With(x => x.Monitored = true) + .TheNext(1) + .With(x => x.ForeignReleaseId = "NotExistingId2") + .Build() as List; + + newAlbum.AlbumReleases = releases; + albumCopy.AlbumReleases = releases; + + // ExistingId2 is monitored but will be deleted + var existingReleases = Builder.CreateListOfSize(2) + .All() + .With(x => x.Monitored = false) + .TheFirst(1) + .With(x => x.ForeignReleaseId = "ExistingId1") + .TheNext(1) + .With(x => x.ForeignReleaseId = "ExistingId2") + .With(x => x.Monitored = true) + .Build() as List; + + Mocker.GetMock() + .Setup(x => x.GetReleasesForRefresh(It.IsAny(), It.IsAny>())) + .Returns(existingReleases); + + Mocker.GetMock() + .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()); + + 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()); } } } diff --git a/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs b/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs index e836a1490..789c5ae3e 100644 --- a/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs +++ b/src/NzbDrone.Core/MetadataSource/SkyHook/SkyHookProxy.cs @@ -311,7 +311,6 @@ namespace NzbDrone.Core.MetadataSource.SkyHook if (resource.Releases != null) { album.AlbumReleases = resource.Releases.Select(x => MapRelease(x, artistDict)).ToList(); - album.AlbumReleases.Value.OrderByDescending(x => x.TrackCount).First().Monitored = true; } album.AnyReleaseOk = true; diff --git a/src/NzbDrone.Core/Music/AddAlbumService.cs b/src/NzbDrone.Core/Music/AddAlbumService.cs index 170482b8d..b2afaaa75 100644 --- a/src/NzbDrone.Core/Music/AddAlbumService.cs +++ b/src/NzbDrone.Core/Music/AddAlbumService.cs @@ -5,6 +5,7 @@ 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; @@ -20,40 +21,93 @@ namespace NzbDrone.Core.Music 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 Album AddAlbum(Album newAlbum) + public List AddAlbumReleases(Album album) { - Ensure.That(newAlbum, () => newAlbum).IsNotNull(); - - var tuple = AddSkyhookData(newAlbum); - newAlbum = tuple.Item2; + 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; _logger.ProgressInfo("Adding Album {0}", newAlbum.Title); - _artistMetadataRepository.UpsertMany(tuple.Item3); - _albumService.AddAlbum(newAlbum, tuple.Item1); + + _artistMetadataRepository.UpsertMany(skyHookData.Item3); + newAlbum.ArtistMetadata = _artistMetadataRepository.FindById(skyHookData.Item1); + newAlbum.ArtistMetadataId = newAlbum.ArtistMetadata.Value.Id; + + _albumService.AddAlbum(newAlbum); + AddAlbumReleases(newAlbum); - // make sure releases are populated for tag writing in the track refresh - newAlbum.AlbumReleases.Value.ForEach(x => x.Album = 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) { @@ -63,17 +117,10 @@ namespace NzbDrone.Core.Music foreach (var newAlbum in newAlbums) { var tuple = AddSkyhookData(newAlbum); - var album = tuple.Item2; - album.Added = added; - album.LastInfoSync = added; - _logger.ProgressInfo("Adding Album {0}", newAlbum.Title); - _artistMetadataRepository.UpsertMany(tuple.Item3); - album = _albumService.AddAlbum(album, tuple.Item1); - - // make sure releases are populated for tag writing in the track refresh - album.AlbumReleases.Value.ForEach(x => x.Album = album); - _refreshTrackService.RefreshTrackInfo(album, false); - albumsToAdd.Add(album); + tuple.Item2.Added = added; + tuple.Item2.LastInfoSync = added; + + albumsToAdd.Add(AddAlbum(tuple)); } return albumsToAdd; diff --git a/src/NzbDrone.Core/Music/AlbumService.cs b/src/NzbDrone.Core/Music/AlbumService.cs index d4e5633e3..d8c096244 100644 --- a/src/NzbDrone.Core/Music/AlbumService.cs +++ b/src/NzbDrone.Core/Music/AlbumService.cs @@ -17,7 +17,7 @@ namespace NzbDrone.Core.Music List GetAlbumsByArtist(int artistId); List GetAlbumsByArtistMetadataId(int artistMetadataId); List GetAlbumsForRefresh(int artistMetadataId, IEnumerable foreignIds); - Album AddAlbum(Album newAlbum, string albumArtistId); + Album AddAlbum(Album newAlbum); Album FindById(string foreignId); Album FindByTitle(int artistId, string title); Album FindByTitleInexact(int artistId, string title); @@ -44,41 +44,22 @@ namespace NzbDrone.Core.Music IHandle { private readonly IAlbumRepository _albumRepository; - private readonly IReleaseRepository _releaseRepository; - private readonly IArtistMetadataRepository _artistMetadataRepository; private readonly IEventAggregator _eventAggregator; - private readonly ITrackService _trackService; private readonly Logger _logger; public AlbumService(IAlbumRepository albumRepository, - IReleaseRepository releaseRepository, - IArtistMetadataRepository artistMetadataRepository, IEventAggregator eventAggregator, - ITrackService trackService, Logger logger) { _albumRepository = albumRepository; - _releaseRepository = releaseRepository; - _artistMetadataRepository = artistMetadataRepository; _eventAggregator = eventAggregator; - _trackService = trackService; _logger = logger; } - public Album AddAlbum(Album newAlbum, string albumArtistId) + public Album AddAlbum(Album newAlbum) { _albumRepository.Insert(newAlbum); - - foreach (var release in newAlbum.AlbumReleases.Value) - { - release.AlbumId = newAlbum.Id; - } - _releaseRepository.InsertMany(newAlbum.AlbumReleases.Value); - - newAlbum.ArtistMetadata = _artistMetadataRepository.FindById(albumArtistId); - newAlbum.ArtistMetadataId = newAlbum.ArtistMetadata.Value.Id; - - _albumRepository.Update(newAlbum); + //_eventAggregator.PublishEvent(new AlbumAddedEvent(GetAlbum(newAlbum.Id))); return newAlbum; diff --git a/src/NzbDrone.Core/Music/RefreshAlbumService.cs b/src/NzbDrone.Core/Music/RefreshAlbumService.cs index 9aac86fed..b4b1160dc 100644 --- a/src/NzbDrone.Core/Music/RefreshAlbumService.cs +++ b/src/NzbDrone.Core/Music/RefreshAlbumService.cs @@ -11,6 +11,7 @@ using NzbDrone.Core.Exceptions; using NzbDrone.Core.Messaging.Commands; using NzbDrone.Core.Music.Commands; using NzbDrone.Core.MediaFiles; +using NzbDrone.Common.EnsureThat; namespace NzbDrone.Core.Music { @@ -146,14 +147,23 @@ namespace NzbDrone.Core.Music 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 upToDateCount = 0; + var upToDateReleaseList = new List(); foreach (var release in remoteReleases) { release.AlbumId = album.Id; release.Album = album; + + // 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; + var releaseToRefresh = existingReleases.SingleOrDefault(r => r.ForeignReleaseId == release.ForeignReleaseId); if (releaseToRefresh != null) @@ -163,7 +173,6 @@ namespace NzbDrone.Core.Music // copy across the db keys and check for equality release.Id = releaseToRefresh.Id; release.AlbumId = releaseToRefresh.AlbumId; - release.Monitored = releaseToRefresh.Monitored; if (!releaseToRefresh.Equals(release)) { @@ -171,18 +180,38 @@ namespace NzbDrone.Core.Music } else { - upToDateCount++; + upToDateReleaseList.Add(release); } } else { - release.Monitored = false; newReleaseList.Add(release); } + album.AlbumReleases.Value.Add(release); } - - _logger.Debug($"{album} {upToDateCount} releases up to date; Deleting {existingReleases.Count}, Updating {updateReleaseList.Count}, Adding {newReleaseList.Count} releases."); + + var refreshedToMonitor = remoteReleases.SingleOrDefault(x => x.ForeignReleaseId == existingToMonitor?.ForeignReleaseId) ?? + remoteReleases.OrderByDescending(x => x.TrackCount).First(); + refreshedToMonitor.Monitored = true; + + if (upToDateReleaseList.Contains(refreshedToMonitor)) + { + // we weren't going to update, but have changed monitored so now need to + upToDateReleaseList.Remove(refreshedToMonitor); + updateReleaseList.Add(refreshedToMonitor); + } + else if (updateReleaseList.Contains(refreshedToMonitor) && refreshedToMonitor.Equals(existingToMonitor)) + { + // 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); + } + + 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."); // before deleting anything, remove musicbrainz ids for things we are deleting _audioTagService.RemoveMusicBrainzTags(existingReleases); @@ -191,13 +220,6 @@ namespace NzbDrone.Core.Music _releaseService.UpdateMany(updateReleaseList); _releaseService.InsertMany(newReleaseList); - if (album.AlbumReleases.Value.Count(x => x.Monitored) == 0) - { - var toMonitor = album.AlbumReleases.Value.OrderByDescending(x => x.TrackCount).First(); - toMonitor.Monitored = true; - _releaseService.UpdateMany(new List { toMonitor }); - } - // if we have updated a monitored release, refresh all file tags forceUpdateFileTags |= updateReleaseList.Any(x => x.Monitored); @@ -205,7 +227,6 @@ namespace NzbDrone.Core.Music _albumService.UpdateMany(new List{album}); _logger.Debug("Finished album refresh for {0}", album.Title); - } public void Execute(RefreshAlbumCommand message)