More resilient MusicBrainz lookup

3 files changed, 196 insertions, 43 deletions

diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs
index 397c916a4f..7dee5fd31d 100644
--- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs
+++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzAlbumProvider.cs
@@ -14,6 +14,7 @@ using MediaBrowser.Providers.Music;
 using MetaBrainz.MusicBrainz;
 using MetaBrainz.MusicBrainz.Interfaces.Entities;
 using MetaBrainz.MusicBrainz.Interfaces.Searches;
+using Microsoft.Extensions.Logging;
 
 namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 
@@ -22,6 +23,17 @@ namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 /// </summary>
 public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, AlbumInfo>, IHasOrder
 {
+    private readonly ILogger<MusicBrainzAlbumProvider> _logger;
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="MusicBrainzAlbumProvider"/> class.
+    /// </summary>
+    /// <param name="logger">The logger.</param>
+    public MusicBrainzAlbumProvider(ILogger<MusicBrainzAlbumProvider> logger)
+    {
+        _logger = logger;
+    }
+
     /// <inheritdoc />
     public string Name => "MusicBrainz";
 
@@ -32,21 +44,26 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
     public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(AlbumInfo searchInfo, CancellationToken cancellationToken)
     {
         var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
-        var releaseId = searchInfo.GetReleaseId();
-        var releaseGroupId = searchInfo.GetReleaseGroupId();
+        var releaseId = MusicBrainzQueryExtensions.ParseMusicBrainzId(searchInfo.GetReleaseId(), "release", _logger);
+        var releaseGroupId = MusicBrainzQueryExtensions.ParseMusicBrainzId(searchInfo.GetReleaseGroupId(), "release group", _logger);
 
-        if (!string.IsNullOrEmpty(releaseId))
+        if (releaseId is not null)
         {
-            var releaseResult = await query.LookupReleaseAsync(new Guid(releaseId), Include.Artists | Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
-            return GetReleaseResult(releaseResult).SingleItemAsEnumerable();
+            var releaseResult = await query.LookupReleaseOrNullAsync(releaseId.Value, Include.Artists | Include.ReleaseGroups, _logger, cancellationToken).ConfigureAwait(false);
+            if (releaseResult is not null)
+            {
+                return GetReleaseResult(releaseResult).SingleItemAsEnumerable();
+            }
         }
 
-        if (!string.IsNullOrEmpty(releaseGroupId))
+        if (releaseGroupId is not null)
         {
-            var releaseGroupResult = await query.LookupReleaseGroupAsync(new Guid(releaseGroupId), Include.Releases, null, cancellationToken).ConfigureAwait(false);
-
-            // No need to pass the cancellation token to GetReleaseGroupResultAsync as we're already passing it to ToBlockingEnumerable
-            return GetReleaseGroupResultAsync(releaseGroupResult.Releases, CancellationToken.None).ToBlockingEnumerable(cancellationToken);
+            var releaseGroupResult = await query.LookupReleaseGroupOrNullAsync(releaseGroupId.Value, Include.Releases, _logger, cancellationToken).ConfigureAwait(false);
+            if (releaseGroupResult is not null)
+            {
+                // No need to pass the cancellation token to GetReleaseGroupResultAsync as we're already passing it to ToBlockingEnumerable
+                return GetReleaseGroupResultAsync(releaseGroupResult.Releases, CancellationToken.None).ToBlockingEnumerable(cancellationToken);
+            }
         }
 
         var artistMusicBrainzId = searchInfo.GetMusicBrainzArtistId();
@@ -102,8 +119,11 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
         foreach (var result in releaseSearchResults)
         {
             // Fetch full release info, otherwise artists are missing
-            var fullResult = await query.LookupReleaseAsync(result.Id, Include.Artists | Include.ReleaseGroups, cancellationToken).ConfigureAwait(false);
-            yield return GetReleaseResult(fullResult);
+            var fullResult = await query.LookupReleaseOrNullAsync(result.Id, Include.Artists | Include.ReleaseGroups, _logger, cancellationToken).ConfigureAwait(false);
+            if (fullResult is not null)
+            {
+                yield return GetReleaseResult(fullResult);
+            }
         }
     }
 
@@ -156,8 +176,8 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
     public async Task<MetadataResult<MusicAlbum>> GetMetadata(AlbumInfo info, CancellationToken cancellationToken)
     {
         var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
-        var releaseId = info.GetReleaseId();
-        var releaseGroupId = info.GetReleaseGroupId();
+        var releaseId = MusicBrainzQueryExtensions.ParseMusicBrainzId(info.GetReleaseId(), "release", _logger);
+        var releaseGroupId = MusicBrainzQueryExtensions.ParseMusicBrainzId(info.GetReleaseGroupId(), "release group", _logger);
 
         var result = new MetadataResult<MusicAlbum>
         {
@@ -165,15 +185,15 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
         };
 
         // If there is a release group, but no release ID, try to match the release
-        if (string.IsNullOrWhiteSpace(releaseId) && !string.IsNullOrWhiteSpace(releaseGroupId))
+        if (releaseId is null && releaseGroupId is not null)
         {
             // TODO: Actually try to match the release. Simply taking the first result is stupid.
-            var releaseGroupLookup = await query.LookupReleaseGroupAsync(new Guid(releaseGroupId), Include.None, null, cancellationToken).ConfigureAwait(false);
-            releaseId = releaseGroupLookup.Releases?.Count > 0 ? releaseGroupLookup.Releases[0].Id.ToString() : null;
+            var releaseGroupLookup = await query.LookupReleaseGroupOrNullAsync(releaseGroupId.Value, Include.None, _logger, cancellationToken).ConfigureAwait(false);
+            releaseId = releaseGroupLookup?.Releases?.Count > 0 ? releaseGroupLookup.Releases[0].Id : null;
         }
 
         // If there is no release ID, lookup a release with the info we have
-        if (string.IsNullOrWhiteSpace(releaseId))
+        if (releaseId is null)
         {
             var artistMusicBrainzId = info.GetMusicBrainzArtistId();
             IRelease? releaseResult = null;
@@ -193,55 +213,61 @@ public class MusicBrainzAlbumProvider : IRemoteMetadataProvider<MusicAlbum, Albu
 
             if (releaseResult is not null)
             {
-                releaseId = releaseResult.Id.ToString();
+                releaseId = releaseResult.Id;
 
                 if (releaseResult.ReleaseGroup?.Id is not null)
                 {
-                    releaseGroupId = releaseResult.ReleaseGroup.Id.ToString();
+                    releaseGroupId = releaseResult.ReleaseGroup.Id;
                 }
             }
         }
 
-        if (string.IsNullOrWhiteSpace(releaseId) && string.IsNullOrWhiteSpace(releaseGroupId))
+        if (releaseId is null && releaseGroupId is null)
         {
             return result;
         }
 
         // Fetch the full release (and its release group) so we can populate everything MusicBrainz returns.
         IRelease? release = null;
-        if (!string.IsNullOrWhiteSpace(releaseId))
+        if (releaseId is not null)
         {
-            release = await query.LookupReleaseAsync(
-                new Guid(releaseId),
+            release = await query.LookupReleaseOrNullAsync(
+                releaseId.Value,
                 Include.Artists | Include.ReleaseGroups | Include.Labels | Include.Genres | Include.Tags,
+                _logger,
                 cancellationToken).ConfigureAwait(false);
 
-            if (string.IsNullOrWhiteSpace(releaseGroupId) && release?.ReleaseGroup?.Id is not null)
+            if (releaseGroupId is null && release?.ReleaseGroup?.Id is not null)
             {
-                releaseGroupId = release.ReleaseGroup.Id.ToString();
+                releaseGroupId = release.ReleaseGroup.Id;
             }
         }
 
         IReleaseGroup? releaseGroup = null;
-        if (!string.IsNullOrWhiteSpace(releaseGroupId))
+        if (releaseGroupId is not null)
         {
-            releaseGroup = await query.LookupReleaseGroupAsync(
-                new Guid(releaseGroupId),
+            releaseGroup = await query.LookupReleaseGroupOrNullAsync(
+                releaseGroupId.Value,
                 Include.Artists | Include.Genres | Include.Tags,
-                null,
+                _logger,
                 cancellationToken).ConfigureAwait(false);
         }
 
+        if (release is null && releaseGroup is null)
+        {
+            return result;
+        }
+
         result.HasMetadata = true;
 
-        if (!string.IsNullOrEmpty(releaseId))
+        if (releaseId is not null)
         {
-            result.Item.SetProviderId(MetadataProvider.MusicBrainzAlbum, releaseId);
+            result.Item.SetProviderId(MetadataProvider.MusicBrainzAlbum, releaseId.Value.ToString());
         }
 
-        if (!string.IsNullOrEmpty(releaseGroupId))
+        if (releaseGroupId is not null)
         {
-            result.Item.SetProviderId(MetadataProvider.MusicBrainzReleaseGroup, releaseGroupId);
+            result.Item.SetProviderId(MetadataProvider.MusicBrainzReleaseGroup, releaseGroupId.Value.ToString());
         }
 
         Populate(result.Item, release, releaseGroup);
diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs
index a9e950fb64..c3d13ed42c 100644
--- a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs
+++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzArtistProvider.cs
@@ -13,6 +13,7 @@ using MediaBrowser.Providers.Music;
 using MetaBrainz.MusicBrainz;
 using MetaBrainz.MusicBrainz.Interfaces.Entities;
 using MetaBrainz.MusicBrainz.Interfaces.Searches;
+using Microsoft.Extensions.Logging;
 
 namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 
@@ -21,6 +22,17 @@ namespace MediaBrowser.Providers.Plugins.MusicBrainz;
 /// </summary>
 public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, ArtistInfo>, IHasOrder
 {
+    private readonly ILogger<MusicBrainzArtistProvider> _logger;
+
+    /// <summary>
+    /// Initializes a new instance of the <see cref="MusicBrainzArtistProvider"/> class.
+    /// </summary>
+    /// <param name="logger">The logger.</param>
+    public MusicBrainzArtistProvider(ILogger<MusicBrainzArtistProvider> logger)
+    {
+        _logger = logger;
+    }
+
     /// <inheritdoc />
     public string Name => "MusicBrainz";
 
@@ -32,12 +44,15 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, Ar
     public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(ArtistInfo searchInfo, CancellationToken cancellationToken)
     {
         var query = MusicBrainz.Plugin.Instance!.MusicBrainzQuery;
-        var artistId = searchInfo.GetMusicBrainzArtistId();
+        var artistId = MusicBrainzQueryExtensions.ParseMusicBrainzId(searchInfo.GetMusicBrainzArtistId(), "artist", _logger);
 
-        if (!string.IsNullOrWhiteSpace(artistId))
+        if (artistId is not null)
         {
-            var artistResult = await query.LookupArtistAsync(new Guid(artistId), Include.Aliases, null, null, cancellationToken).ConfigureAwait(false);
-            return GetResultFromResponse(artistResult).SingleItemAsEnumerable();
+            var artistResult = await query.LookupArtistOrNullAsync(artistId.Value, Include.Aliases, _logger, cancellationToken).ConfigureAwait(false);
+            if (artistResult is not null)
+            {
+                return GetResultFromResponse(artistResult).SingleItemAsEnumerable();
+            }
         }
 
         if (string.IsNullOrWhiteSpace(searchInfo.Name))
@@ -99,22 +114,22 @@ public class MusicBrainzArtistProvider : IRemoteMetadataProvider<MusicArtist, Ar
     {
         var result = new MetadataResult<MusicArtist> { Item = new MusicArtist() };
 
-        var musicBrainzId = info.GetMusicBrainzArtistId();
+        var musicBrainzId = MusicBrainzQueryExtensions.ParseMusicBrainzId(info.GetMusicBrainzArtistId(), "artist", _logger);
 
         // If we don't have an id yet, resolve one by name so we can look the artist up.
-        if (string.IsNullOrWhiteSpace(musicBrainzId))
+        if (musicBrainzId is null)
         {
             var searchResults = await GetSearchResults(info, cancellationToken).ConfigureAwait(false);
-            musicBrainzId = searchResults.FirstOrDefault()?.GetProviderId(MetadataProvider.MusicBrainzArtist);
+            musicBrainzId = MusicBrainzQueryExtensions.ParseMusicBrainzId(searchResults.FirstOrDefault()?.GetProviderId(MetadataProvider.MusicBrainzArtist), "artist", _logger);
         }
 
-        if (string.IsNullOrWhiteSpace(musicBrainzId))
+        if (musicBrainzId is null)
         {
             return result;
         }
 
         var query = Plugin.Instance!.MusicBrainzQuery;
-        var artist = await query.LookupArtistAsync(new Guid(musicBrainzId), Include.Genres | Include.Tags, null, null, cancellationToken).ConfigureAwait(false);
+        var artist = await query.LookupArtistOrNullAsync(musicBrainzId.Value, Include.Genres | Include.Tags, _logger, cancellationToken).ConfigureAwait(false);
 
         if (artist is null)
         {
diff --git a/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs
new file mode 100644
index 0000000000..f3df41e942
--- /dev/null
+++ b/MediaBrowser.Providers/Plugins/MusicBrainz/MusicBrainzQueryExtensions.cs
@@ -0,0 +1,112 @@
+using System;
+using System.Net;
+using System.Threading;
+using System.Threading.Tasks;
+using MetaBrainz.Common;
+using MetaBrainz.MusicBrainz;
+using MetaBrainz.MusicBrainz.Interfaces.Entities;
+using Microsoft.Extensions.Logging;
+
+namespace MediaBrowser.Providers.Plugins.MusicBrainz;
+
+/// <summary>
+/// Helpers for talking to MusicBrainz with identifiers that are not guaranteed to be valid.
+/// </summary>
+internal static class MusicBrainzQueryExtensions
+{
+    /// <summary>
+    /// Parses a MusicBrainz identifier, which may come from user-supplied tags or NFO files and is therefore not
+    /// guaranteed to be a valid GUID.
+    /// </summary>
+    /// <param name="id">The identifier to parse.</param>
+    /// <param name="entityType">The type of entity the identifier refers to, used for logging.</param>
+    /// <param name="logger">The logger.</param>
+    /// <returns>The parsed identifier, or <see langword="null"/> if it is missing or malformed.</returns>
+    public static Guid? ParseMusicBrainzId(string? id, string entityType, ILogger logger)
+    {
+        if (string.IsNullOrWhiteSpace(id))
+        {
+            return null;
+        }
+
+        if (!Guid.TryParse(id, out var parsedId))
+        {
+            logger.LogDebug("Ignoring malformed MusicBrainz {EntityType} id {Id}", entityType, id);
+            return null;
+        }
+
+        return parsedId;
+    }
+
+    /// <summary>
+    /// Looks up a release, treating an unknown identifier as missing data rather than an error.
+    /// </summary>
+    /// <param name="query">The MusicBrainz query client.</param>
+    /// <param name="releaseId">The release identifier.</param>
+    /// <param name="include">The additional data to include in the lookup.</param>
+    /// <param name="logger">The logger.</param>
+    /// <param name="cancellationToken">The cancellation token.</param>
+    /// <returns>The release, or <see langword="null"/> if MusicBrainz does not have it.</returns>
+    public static Task<IRelease?> LookupReleaseOrNullAsync(this Query query, Guid releaseId, Include include, ILogger logger, CancellationToken cancellationToken)
+        => NotFoundAsNullAsync(
+            () => query.LookupReleaseAsync(releaseId, include, cancellationToken),
+            "release",
+            releaseId,
+            logger);
+
+    /// <summary>
+    /// Looks up a release group, treating an unknown identifier as missing data rather than an error.
+    /// </summary>
+    /// <param name="query">The MusicBrainz query client.</param>
+    /// <param name="releaseGroupId">The release group identifier.</param>
+    /// <param name="include">The additional data to include in the lookup.</param>
+    /// <param name="logger">The logger.</param>
+    /// <param name="cancellationToken">The cancellation token.</param>
+    /// <returns>The release group, or <see langword="null"/> if MusicBrainz does not have it.</returns>
+    public static Task<IReleaseGroup?> LookupReleaseGroupOrNullAsync(this Query query, Guid releaseGroupId, Include include, ILogger logger, CancellationToken cancellationToken)
+        => NotFoundAsNullAsync(
+            () => query.LookupReleaseGroupAsync(releaseGroupId, include, null, cancellationToken),
+            "release group",
+            releaseGroupId,
+            logger);
+
+    /// <summary>
+    /// Looks up an artist, treating an unknown identifier as missing data rather than an error.
+    /// </summary>
+    /// <param name="query">The MusicBrainz query client.</param>
+    /// <param name="artistId">The artist identifier.</param>
+    /// <param name="include">The additional data to include in the lookup.</param>
+    /// <param name="logger">The logger.</param>
+    /// <param name="cancellationToken">The cancellation token.</param>
+    /// <returns>The artist, or <see langword="null"/> if MusicBrainz does not have it.</returns>
+    public static Task<IArtist?> LookupArtistOrNullAsync(this Query query, Guid artistId, Include include, ILogger logger, CancellationToken cancellationToken)
+        => NotFoundAsNullAsync(
+            () => query.LookupArtistAsync(artistId, include, null, null, cancellationToken),
+            "artist",
+            artistId,
+            logger);
+
+    /// <summary>
+    /// Runs a lookup, mapping a "not found" response to <see langword="null"/>. Identifiers stored on a library item
+    /// can refer to entities that no longer exist in MusicBrainz, which is not an error worth failing a refresh over.
+    /// </summary>
+    /// <typeparam name="T">The type of entity being looked up.</typeparam>
+    /// <param name="lookup">The lookup to run.</param>
+    /// <param name="entityType">The type of entity being looked up, used for logging.</param>
+    /// <param name="id">The identifier being looked up, used for logging.</param>
+    /// <param name="logger">The logger.</param>
+    /// <returns>The entity, or <see langword="null"/> if MusicBrainz does not have it.</returns>
+    private static async Task<T?> NotFoundAsNullAsync<T>(Func<Task<T>> lookup, string entityType, Guid id, ILogger logger)
+        where T : class
+    {
+        try
+        {
+            return await lookup().ConfigureAwait(false);
+        }
+        catch (HttpError ex) when (ex.Status == HttpStatusCode.NotFound)
+        {
+            logger.LogDebug("MusicBrainz has no {EntityType} with id {Id}", entityType, id);
+            return null;
+        }
+    }
+}