Fixed: Don't download artist images if match existing (#362)

* Fixed: Don't download artist images if existing exists

* fixup: Wrap FileSetWriteTime in try

* fixup! Tests and Rework

src/NzbDrone.Common/Http/HttpHeader.cs

@@ -117,6 +117,18 @@ namespace NzbDrone.Common.Http
             }
         }
 
+        public DateTime? LastModified
+        {
+            get
+            {
+                return GetSingleValue("Last-Modified", Convert.ToDateTime);
+            }
+            set
+            {
+                SetSingleValue("Last-Modified", value);
+            }
+        }
+
         public new IEnumerator<KeyValuePair<string, string>> GetEnumerator()
         {
             return AllKeys.SelectMany(GetValues, (k, c) => new KeyValuePair<string, string>(k, c)).ToList().GetEnumerator();

src/NzbDrone.Core.Test/MediaCoverTests/CoverExistsSpecificationFixture.cs

@@ -1,72 +1,51 @@
-using System.Net;
 using FluentAssertions;
 using Moq;
 using NUnit.Framework;
 using NzbDrone.Common.Disk;
-using NzbDrone.Common.Http;
 using NzbDrone.Core.MediaCover;
 using NzbDrone.Core.Test.Framework;
+using System;
 
 namespace NzbDrone.Core.Test.MediaCoverTests
 {
     [TestFixture]
     public class CoverAlreadyExistsSpecificationFixture : CoreTest<CoverAlreadyExistsSpecification>
     {
-        private HttpResponse _httpResponse;
-
-        [SetUp]
-        public void Setup()
-        {
-            _httpResponse = new HttpResponse(null, new HttpHeader(), "", HttpStatusCode.OK);
-            Mocker.GetMock<IDiskProvider>().Setup(c => c.GetFileSize(It.IsAny<string>())).Returns(100);
-            Mocker.GetMock<IHttpClient>().Setup(c => c.Head(It.IsAny<HttpRequest>())).Returns(_httpResponse);
-
-        }
-
-
         private void GivenFileExistsOnDisk()
         {
             Mocker.GetMock<IDiskProvider>().Setup(c => c.FileExists(It.IsAny<string>())).Returns(true);
         }
 
 
-        private void GivenExistingFileSize(long bytes)
+        private void GivenExistingFileDate(DateTime lastModifiedDate)
         {
             GivenFileExistsOnDisk();
-            Mocker.GetMock<IDiskProvider>().Setup(c => c.GetFileSize(It.IsAny<string>())).Returns(bytes);
+            Mocker.GetMock<IDiskProvider>().Setup(c => c.FileGetLastWrite(It.IsAny<string>())).Returns(lastModifiedDate);
 
         }
 
-
         [Test]
         public void should_return_false_if_file_not_exists()
         {
-            Subject.AlreadyExists("http://url", "c:\\file.exe").Should().BeFalse();
+            Subject.AlreadyExists(DateTime.Now, "c:\\file.exe").Should().BeFalse();
         }
 
         [Test]
-        public void should_return_false_if_file_exists_but_diffrent_size()
+        public void should_return_false_if_file_exists_but_diffrent_date()
         {
-            GivenExistingFileSize(100);
+            GivenExistingFileDate(DateTime.Now);
-            _httpResponse.Headers.ContentLength = 200;
 
-            Subject.AlreadyExists("http://url", "c:\\file.exe").Should().BeFalse();
+            Subject.AlreadyExists(DateTime.Now.AddHours(-5), "c:\\file.exe").Should().BeFalse();
-        }
-
-
-        [Test]
-        public void should_return_ture_if_file_exists_and_same_size()
-        {
-            GivenExistingFileSize(100);
-            _httpResponse.Headers.ContentLength = 100;
-            Subject.AlreadyExists("http://url", "c:\\file.exe").Should().BeTrue();
         }
 
         [Test]
-        public void should_return_true_if_there_is_no_size_header_and_file_exist()
+        public void should_return_ture_if_file_exists_and_same_date()
         {
-            GivenExistingFileSize(100);
+            var givenDate = DateTime.Now;
-            Subject.AlreadyExists("http://url", "c:\\file.exe").Should().BeFalse();
+
+            GivenExistingFileDate(givenDate);
+
+            Subject.AlreadyExists(givenDate, "c:\\file.exe").Should().BeTrue();
         }
     }
 }

src/NzbDrone.Core.Test/MediaCoverTests/MediaCoverServiceFixture.cs

@@ -7,6 +7,7 @@ using Moq;
 using NUnit.Framework;
 using NzbDrone.Common.Disk;
 using NzbDrone.Common.EnvironmentInfo;
+using NzbDrone.Common.Http;
 using NzbDrone.Core.MediaCover;
 using NzbDrone.Core.Test.Framework;
 using NzbDrone.Core.Music;
@@ -19,6 +20,7 @@ namespace NzbDrone.Core.Test.MediaCoverTests
     {
         Artist _artist;
         Album _album;
+        private HttpResponse _httpResponse;
 
         [SetUp]
         public void Setup()
@@ -34,6 +36,9 @@ namespace NzbDrone.Core.Test.MediaCoverTests
                 .With(v => v.Id = 4)
                 .With(v => v.Images = new List<MediaCover.MediaCover> { new MediaCover.MediaCover(MediaCoverTypes.Cover, "") })
                 .Build();
+
+            _httpResponse = new HttpResponse(null, new HttpHeader(), "");
+            Mocker.GetMock<IHttpClient>().Setup(c => c.Head(It.IsAny<HttpRequest>())).Returns(_httpResponse);
         }
 
         [Test]
@@ -95,7 +100,7 @@ namespace NzbDrone.Core.Test.MediaCoverTests
         public void should_resize_covers_if_main_downloaded()
         {
             Mocker.GetMock<ICoverExistsSpecification>()
-                  .Setup(v => v.AlreadyExists(It.IsAny<string>(), It.IsAny<string>()))
+                  .Setup(v => v.AlreadyExists(It.IsAny<DateTime>(), It.IsAny<string>()))
                   .Returns(false);
 
             Mocker.GetMock<IDiskProvider>()
@@ -112,7 +117,7 @@ namespace NzbDrone.Core.Test.MediaCoverTests
         public void should_resize_covers_if_missing()
         {
             Mocker.GetMock<ICoverExistsSpecification>()
-                  .Setup(v => v.AlreadyExists(It.IsAny<string>(), It.IsAny<string>()))
+                  .Setup(v => v.AlreadyExists(It.IsAny<DateTime>(), It.IsAny<string>()))
                   .Returns(true);
 
             Mocker.GetMock<IDiskProvider>()
@@ -129,7 +134,7 @@ namespace NzbDrone.Core.Test.MediaCoverTests
         public void should_not_resize_covers_if_exists()
         {
             Mocker.GetMock<ICoverExistsSpecification>()
-                  .Setup(v => v.AlreadyExists(It.IsAny<string>(), It.IsAny<string>()))
+                  .Setup(v => v.AlreadyExists(It.IsAny<DateTime>(), It.IsAny<string>()))
                   .Returns(true);
 
             Mocker.GetMock<IDiskProvider>()
@@ -150,7 +155,7 @@ namespace NzbDrone.Core.Test.MediaCoverTests
         public void should_resize_covers_if_existing_is_empty()
         {
             Mocker.GetMock<ICoverExistsSpecification>()
-                  .Setup(v => v.AlreadyExists(It.IsAny<string>(), It.IsAny<string>()))
+                  .Setup(v => v.AlreadyExists(It.IsAny<DateTime>(), It.IsAny<string>()))
                   .Returns(true);
 
             Mocker.GetMock<IDiskProvider>()
@@ -171,7 +176,7 @@ namespace NzbDrone.Core.Test.MediaCoverTests
         public void should_log_error_if_resize_failed()
         {
             Mocker.GetMock<ICoverExistsSpecification>()
-                  .Setup(v => v.AlreadyExists(It.IsAny<string>(), It.IsAny<string>()))
+                  .Setup(v => v.AlreadyExists(It.IsAny<DateTime>(), It.IsAny<string>()))
                   .Returns(true);
 
             Mocker.GetMock<IDiskProvider>()

src/NzbDrone.Core/MediaCover/CoverAlreadyExistsSpecification.cs

@@ -1,34 +1,32 @@
-using NzbDrone.Common.Disk;
+using System;
-using NzbDrone.Common.Http;
+using NzbDrone.Common.Disk;
 
 namespace NzbDrone.Core.MediaCover
 {
     public interface ICoverExistsSpecification
     {
-        bool AlreadyExists(string url, string path);
+        bool AlreadyExists(DateTime serverModifiedDate, string localPath);
     }
 
     public class CoverAlreadyExistsSpecification : ICoverExistsSpecification
     {
         private readonly IDiskProvider _diskProvider;
-        private readonly IHttpClient _httpClient;
 
-        public CoverAlreadyExistsSpecification(IDiskProvider diskProvider, IHttpClient httpClient)
+        public CoverAlreadyExistsSpecification(IDiskProvider diskProvider)
         {
             _diskProvider = diskProvider;
-            _httpClient = httpClient;
         }
 
-        public bool AlreadyExists(string url, string path)
+        public bool AlreadyExists(DateTime lastModifiedDateServer, string localPath)
         {
-            if (!_diskProvider.FileExists(path))
+            if (!_diskProvider.FileExists(localPath))
             {
                 return false;
             }
 
-            var headers = _httpClient.Head(new HttpRequest(url)).Headers;
+            DateTime? lastModifiedLocal = _diskProvider.FileGetLastWrite(localPath);
-            var fileSize = _diskProvider.GetFileSize(path);
+
-            return fileSize == headers.ContentLength;
+            return lastModifiedLocal.Value.ToUniversalTime() == lastModifiedDateServer.ToUniversalTime();
         }
     }
 }

src/NzbDrone.Core/MediaCover/MediaCoverService.cs

@@ -109,12 +109,16 @@ namespace NzbDrone.Core.MediaCover
             {
                 var fileName = GetCoverPath(artist.Id, cover.CoverType);
                 var alreadyExists = false;
+                
                 try
                 {
-                    alreadyExists = _coverExistsSpecification.AlreadyExists(cover.Url, fileName);
+                    var lastModifiedServer = GetCoverModifiedDate(cover.Url);
+
+                    alreadyExists = _coverExistsSpecification.AlreadyExists(lastModifiedServer, fileName);
+
                     if (!alreadyExists)
                     {
-                        DownloadCover(artist, cover);
+                        DownloadCover(artist, cover, lastModifiedServer);
                     }
                 }
                 catch (WebException e)
@@ -130,48 +134,58 @@ namespace NzbDrone.Core.MediaCover
             }
         }
 
-        private void EnsureAlbumCovers(Album album)
+        //TODO Decide if we want to cache album art local
-        {
+        //private void EnsureAlbumCovers(Album album)
-            foreach (var cover in album.Images)
+        //{
-            {
+        //    foreach (var cover in album.Images)
-                var fileName = GetCoverPath(album.ArtistId, cover.CoverType, null,  album.Id);
+        //    {
-                var alreadyExists = false;
+        //        var fileName = GetCoverPath(album.ArtistId, cover.CoverType, null,  album.Id);
-                try
+        //        var alreadyExists = false;
-                {
+        //        try
-                    alreadyExists = _coverExistsSpecification.AlreadyExists(cover.Url, fileName);
+        //        {
-                    if (!alreadyExists)
+        //            alreadyExists = _coverExistsSpecification.AlreadyExists(cover.Url, fileName);
-                    {
+        //            if (!alreadyExists)
-                        DownloadAlbumCover(album, cover);
+        //            {
-                    }
+        //                DownloadAlbumCover(album, cover);
-                }
+        //            }
-                catch (WebException e)
+        //        }
-                {
+        //        catch (WebException e)
-                    _logger.Warn("Couldn't download media cover for {0}. {1}", album, e.Message);
+        //        {
-                }
+        //            _logger.Warn("Couldn't download media cover for {0}. {1}", album, e.Message);
-                catch (Exception e)
+        //        }
-                {
+        //        catch (Exception e)
-                    _logger.Error(e, "Couldn't download media cover for {0}", album);
+        //        {
-                }
+        //            _logger.Error(e, "Couldn't download media cover for {0}", album);
+        //        }
 
-                EnsureResizedCovers(album.Artist, cover, !alreadyExists, album);
+        //        EnsureResizedCovers(album.Artist, cover, !alreadyExists, album);
-            }
+        //    }
-        }
+        //}
 
-        private void DownloadCover(Artist artist, MediaCover cover)
+        private void DownloadCover(Artist artist, MediaCover cover, DateTime lastModified)
         {
             var fileName = GetCoverPath(artist.Id, cover.CoverType);
 
             _logger.Info("Downloading {0} for {1} {2}", cover.CoverType, artist, cover.Url);
             _httpClient.DownloadFile(cover.Url, fileName);
+
+            try
+            {
+                _diskProvider.FileSetLastWriteTime(fileName, lastModified);
+            }
+            catch (Exception ex)
+            {
+                _logger.Debug(ex, "Unable to set modified date for {0} image for artist {1}", cover.CoverType, artist);
+            }
         }
 
-        private void DownloadAlbumCover(Album album, MediaCover cover)
+        //private void DownloadAlbumCover(Album album, MediaCover cover)
-        {
+        //{
-            var fileName = GetCoverPath(album.ArtistId, cover.CoverType, null, album.Id);
+        //    var fileName = GetCoverPath(album.ArtistId, cover.CoverType, null, album.Id);
 
-            _logger.Info("Downloading {0} for {1} {2}", cover.CoverType, album, cover.Url);
+        //    _logger.Info("Downloading {0} for {1} {2}", cover.CoverType, album, cover.Url);
-            _httpClient.DownloadFile(cover.Url, fileName);
+        //    _httpClient.DownloadFile(cover.Url, fileName);
-        }
+        //}
 
         private void EnsureResizedCovers(Artist artist, MediaCover cover, bool forceResize, Album album = null)
         {
@@ -270,5 +284,19 @@ namespace NzbDrone.Core.MediaCover
                 _diskProvider.DeleteFolder(path, true);
             }
         }
+
+        private DateTime GetCoverModifiedDate(string url)
+        {
+            var lastModifiedServer = DateTime.Now;
+
+            var headers = _httpClient.Head(new HttpRequest(url)).Headers;
+
+            if (headers.LastModified.HasValue)
+            {
+                lastModifiedServer = headers.LastModified.Value;
+            }
+
+            return lastModifiedServer;
+        }
     }
 }