Skip to content

Commit

Permalink
Fixed: Don't fail entire import if Validation error on list item
Browse files Browse the repository at this point in the history 
Fixes #2021
Fixes #2133
  • Loading branch information
@Qstick
Qstick committed Apr 2, 2021
1 parent 3a7e5c9 commit d198c99
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 38 deletions.
22 changes: 6 additions & 16 deletions src/NzbDrone.Common.Test/Http/HttpClientFixture.cs
View file
Expand Up @@ -46,7 +46,7 @@ public void FixtureSetUp()

TestLogger.Info($"{candidates.Length} TestSites available.");

_httpBinSleep = _httpBinHosts.Count() < 2 ? 100 : 10;
_httpBinSleep = _httpBinHosts.Length < 2 ? 100 : 10;
}

private bool IsTestSiteAvailable(string site)
Expand Down Expand Up @@ -100,7 +100,7 @@ public void SetUp()
Mocker.SetConstant<ICacheManager>(Mocker.Resolve<CacheManager>());
Mocker.SetConstant<ICreateManagedWebProxy>(Mocker.Resolve<ManagedWebProxyFactory>());
Mocker.SetConstant<IRateLimitService>(Mocker.Resolve<RateLimitService>());
Mocker.SetConstant<IEnumerable<IHttpRequestInterceptor>>(new IHttpRequestInterceptor[0]);
Mocker.SetConstant<IEnumerable<IHttpRequestInterceptor>>(Array.Empty<IHttpRequestInterceptor>());
Mocker.SetConstant<IHttpDispatcher>(Mocker.Resolve<TDispatcher>());

// Used for manual testing of socks proxies.
Expand Down Expand Up @@ -193,31 +193,21 @@ public void should_throw_on_unsuccessful_status_codes(int statusCode)
[Test]
public void should_not_throw_on_suppressed_status_codes()
{
var request = new HttpRequest($"http://{_httpBinHost}/status/{HttpStatusCode.NotFound}");
var request = new HttpRequest($"https://{_httpBinHost}/status/{HttpStatusCode.NotFound}");
request.SuppressHttpErrorStatusCodes = new[] { HttpStatusCode.NotFound };

var exception = Assert.Throws<HttpException>(() => Subject.Get<HttpBinResource>(request));
Assert.Throws<HttpException>(() => Subject.Get<HttpBinResource>(request));

ExceptionVerification.IgnoreWarns();
}

[Test]
public void should_log_unsuccessful_status_codes()
{
var request = new HttpRequest($"http://{_httpBinHost}/status/{HttpStatusCode.NotFound}");

var exception = Assert.Throws<HttpException>(() => Subject.Get<HttpBinResource>(request));

ExceptionVerification.ExpectedWarns(1);
}

[Test]
public void should_not_log_unsuccessful_status_codes()
{
var request = new HttpRequest($"http://{_httpBinHost}/status/{HttpStatusCode.NotFound}");
var request = new HttpRequest($"https://{_httpBinHost}/status/{HttpStatusCode.NotFound}");
request.LogHttpError = false;

var exception = Assert.Throws<HttpException>(() => Subject.Get<HttpBinResource>(request));
Assert.Throws<HttpException>(() => Subject.Get<HttpBinResource>(request));

ExceptionVerification.ExpectedWarns(0);
}
Expand Down
24 changes: 12 additions & 12 deletions src/NzbDrone.Core.Test/ImportListTests/ImportListSyncServiceFixture.cs
View file
Expand Up @@ -50,12 +50,12 @@ public void SetUp()
.Returns(new List<ImportListExclusion>());

Mocker.GetMock<IAddArtistService>()
.Setup(v => v.AddArtists(It.IsAny<List<Artist>>(), false))
.Returns((List<Artist> artists, bool doRefresh) => artists);
.Setup(v => v.AddArtists(It.IsAny<List<Artist>>(), false, true))
.Returns((List<Artist> artists, bool doRefresh, bool ignoreErrors) => artists);

Mocker.GetMock<IAddAlbumService>()
.Setup(v => v.AddAlbums(It.IsAny<List<Album>>(), false))
.Returns((List<Album> albums, bool doRefresh) => albums);
.Setup(v => v.AddAlbums(It.IsAny<List<Album>>(), false, true))
.Returns((List<Album> albums, bool doRefresh, bool ignoreErrors) => albums);
}

private void WithAlbum()
Expand Down Expand Up @@ -184,7 +184,7 @@ public void should_not_add_if_existing_artist()
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddArtistService>()
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 0), false));
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 0), false, It.IsAny<bool>()));
}

[Test]
Expand All @@ -196,7 +196,7 @@ public void should_not_add_if_existing_album()
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddArtistService>()
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 0), false));
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 0), false, It.IsAny<bool>()));
}

[Test]
Expand All @@ -208,7 +208,7 @@ public void should_add_if_existing_artist_but_new_album()
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddAlbumService>()
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 1), false));
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 1), false, It.IsAny<bool>()));
}

[TestCase(ImportListMonitorType.None, false)]
Expand All @@ -222,7 +222,7 @@ public void should_add_if_not_existing_artist(ImportListMonitorType monitor, boo
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddArtistService>()
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 1 && t.First().Monitored == expectedArtistMonitored), false));
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 1 && t.First().Monitored == expectedArtistMonitored), false, It.IsAny<bool>()));
}

[TestCase(ImportListMonitorType.None, false)]
Expand All @@ -236,7 +236,7 @@ public void should_add_if_not_existing_album(ImportListMonitorType monitor, bool
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddAlbumService>()
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 1 && t.First().Monitored == expectedAlbumMonitored), false));
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 1 && t.First().Monitored == expectedAlbumMonitored), false, It.IsAny<bool>()));
}

[Test]
Expand All @@ -248,7 +248,7 @@ public void should_not_add_artist_if_excluded_artist()
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddArtistService>()
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 0), false));
.Verify(v => v.AddArtists(It.Is<List<Artist>>(t => t.Count == 0), false, It.IsAny<bool>()));
}

[Test]
Expand All @@ -260,7 +260,7 @@ public void should_not_add_album_if_excluded_album()
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddAlbumService>()
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 0), false));
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 0), false, It.IsAny<bool>()));
}

[Test]
Expand All @@ -273,7 +273,7 @@ public void should_not_add_album_if_excluded_artist()
Subject.Execute(new ImportListSyncCommand());

Mocker.GetMock<IAddAlbumService>()
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 0), false));
.Verify(v => v.AddAlbums(It.Is<List<Album>>(t => t.Count == 0), false, It.IsAny<bool>()));
}
}
}
4 changes: 2 additions & 2 deletions src/NzbDrone.Core/ImportLists/ImportListSyncService.cs
View file
Expand Up @@ -117,8 +117,8 @@ private List<Album> ProcessReports(List<ImportListItemInfo> reports)
}
}

var addedArtists = _addArtistService.AddArtists(artistsToAdd, false);
var addedAlbums = _addAlbumService.AddAlbums(albumsToAdd, false);
var addedArtists = _addArtistService.AddArtists(artistsToAdd, false, true);
var addedAlbums = _addAlbumService.AddAlbums(albumsToAdd, false, true);

var message = string.Format($"Import List Sync Completed. Items found: {reports.Count}, Artists added: {addedArtists.Count}, Albums added: {addedAlbums.Count}");

Expand Down
26 changes: 22 additions & 4 deletions src/NzbDrone.Core/Music/Services/AddAlbumService.cs
View file
Expand Up @@ -13,7 +13,7 @@ namespace NzbDrone.Core.Music
public interface IAddAlbumService
{
Album AddAlbum(Album album, bool doRefresh = true);
List<Album> AddAlbums(List<Album> albums, bool doRefresh = true);
List<Album> AddAlbums(List<Album> albums, bool doRefresh = true, bool ignoreErrors = false);
}

public class AddAlbumService : IAddAlbumService
Expand Down Expand Up @@ -77,15 +77,33 @@ public Album AddAlbum(Album album, bool doRefresh = true)
return album;
}

public List<Album> AddAlbums(List<Album> albums, bool doRefresh = true)
public List<Album> AddAlbums(List<Album> albums, bool doRefresh = true, bool ignoreErrors = false)
{
var added = DateTime.UtcNow;
var addedAlbums = new List<Album>();

foreach (var a in albums)
{
a.Added = added;
addedAlbums.Add(AddAlbum(a, doRefresh));
try
{
a.Added = added;
if (addedAlbums.Any(f => f.ForeignAlbumId == a.ForeignAlbumId))
{
_logger.Debug("Musicbrainz ID {0} was not added due to validation failure: Album already exists on list", a.ForeignAlbumId);
continue;
}

addedAlbums.Add(AddAlbum(a, doRefresh));
}
catch (ValidationException ex)
{
if (!ignoreErrors)
{
throw;
}

_logger.Debug("Musicbrainz ID {0} was not added due to validation failures. {1}", a.ForeignAlbumId, ex.Message);
}
}

return addedAlbums;
Expand Down
19 changes: 15 additions & 4 deletions src/NzbDrone.Core/Music/Services/AddArtistService.cs
View file
Expand Up @@ -17,7 +17,7 @@ namespace NzbDrone.Core.Music
public interface IAddArtistService
{
Artist AddArtist(Artist newArtist, bool doRefresh = true);
List<Artist> AddArtists(List<Artist> newArtists, bool doRefresh = true);
List<Artist> AddArtists(List<Artist> newArtists, bool doRefresh = true, bool ignoreErrors = false);
}

public class AddArtistService : IAddArtistService
Expand Down Expand Up @@ -63,7 +63,7 @@ public Artist AddArtist(Artist newArtist, bool doRefresh = true)
return newArtist;
}

public List<Artist> AddArtists(List<Artist> newArtists, bool doRefresh = true)
public List<Artist> AddArtists(List<Artist> newArtists, bool doRefresh = true, bool ignoreErrors = false)
{
var added = DateTime.UtcNow;
var artistsToAdd = new List<Artist>();
Expand All @@ -75,12 +75,23 @@ public List<Artist> AddArtists(List<Artist> newArtists, bool doRefresh = true)
var artist = AddSkyhookData(s);
artist = SetPropertiesAndValidate(artist);
artist.Added = added;
if (artistsToAdd.Any(f => f.ForeignArtistId == artist.ForeignArtistId))
{
_logger.Debug("Musicbrainz ID {0} was not added due to validation failure: Artist already exists on list", s.ForeignArtistId);
continue;
}

artistsToAdd.Add(artist);
}
catch (Exception ex)
catch (ValidationException ex)
{
if (!ignoreErrors)
{
throw;
}

// Catch Import Errors for now until we get things fixed up
_logger.Error(ex, "Failed to import id: {0} - {1}", s.Metadata.Value.ForeignArtistId, s.Metadata.Value.Name);
_logger.Debug(ex, "Failed to import id: {0} - {1}", s.Metadata.Value.ForeignArtistId, s.Metadata.Value.Name);
}
}

Expand Down

0 comments on commit d198c99

Please sign in to comment.