diff options
22 files changed, 759 insertions, 84 deletions
diff --git a/Jellyfin.Api/Controllers/ItemLookupController.cs b/Jellyfin.Api/Controllers/ItemLookupController.cs index d009f80a96..39ba5ab186 100644 --- a/Jellyfin.Api/Controllers/ItemLookupController.cs +++ b/Jellyfin.Api/Controllers/ItemLookupController.cs @@ -13,6 +13,7 @@ using MediaBrowser.Controller.Entities.Movies; using MediaBrowser.Controller.Entities.TV; using MediaBrowser.Controller.Library; using MediaBrowser.Controller.Providers; +using MediaBrowser.Model.Entities; using MediaBrowser.Model.IO; using MediaBrowser.Model.Providers; using Microsoft.AspNetCore.Authorization; @@ -263,7 +264,7 @@ public class ItemLookupController : BaseJellyfinApiController searchResult.ProviderIds); // Since the refresh process won't erase provider Ids, we need to set this explicitly now. - item.ProviderIds = searchResult.ProviderIds; + item.SetProviderIds(searchResult.ProviderIds); await _providerManager.RefreshFullItem( item, new MetadataRefreshOptions(new DirectoryService(_fileSystem)) diff --git a/Jellyfin.Api/Controllers/ItemUpdateController.cs b/Jellyfin.Api/Controllers/ItemUpdateController.cs index 36c82cf461..65fffc4181 100644 --- a/Jellyfin.Api/Controllers/ItemUpdateController.cs +++ b/Jellyfin.Api/Controllers/ItemUpdateController.cs @@ -428,15 +428,7 @@ public class ItemUpdateController : BaseJellyfinApiController if (request.ProviderIds is not null) { - foreach (var pair in request.ProviderIds.ToList()) - { - if (string.IsNullOrEmpty(pair.Value)) - { - request.ProviderIds.Remove(pair.Key); - } - } - - item.ProviderIds = request.ProviderIds; + item.SetProviderIds(request.ProviderIds); } if (item is Video video) diff --git a/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs b/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs index 385a86d31c..09eba92d9e 100644 --- a/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs +++ b/MediaBrowser.Model/Entities/ProviderIdsExtensions.cs @@ -1,14 +1,16 @@ using System; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Linq; +using System.Text.RegularExpressions; namespace MediaBrowser.Model.Entities; /// <summary> /// Class ProviderIdsExtensions. /// </summary> -public static class ProviderIdsExtensions +public static partial class ProviderIdsExtensions { /// <summary> /// Case-insensitive dictionary of <see cref="MetadataProvider"/> string representation. @@ -21,6 +23,27 @@ public static class ProviderIdsExtensions StringComparer.OrdinalIgnoreCase); /// <summary> + /// The known id formats, keyed by provider name. + /// </summary> + private static readonly Dictionary<string, Func<string, bool>> _providerIdValidators = + new(StringComparer.OrdinalIgnoreCase) + { + [MetadataProvider.Imdb.ToString()] = value => ImdbIdRegex().IsMatch(value), + [MetadataProvider.Tmdb.ToString()] = IsPositiveNumber, + [MetadataProvider.TmdbCollection.ToString()] = IsPositiveNumber, + [MetadataProvider.AudioDbArtist.ToString()] = IsPositiveNumber, + [MetadataProvider.AudioDbAlbum.ToString()] = IsPositiveNumber, + + // Every MusicBrainz id is an MBID. + [MetadataProvider.MusicBrainzAlbum.ToString()] = IsGuid, + [MetadataProvider.MusicBrainzAlbumArtist.ToString()] = IsGuid, + [MetadataProvider.MusicBrainzArtist.ToString()] = IsGuid, + [MetadataProvider.MusicBrainzReleaseGroup.ToString()] = IsGuid, + [MetadataProvider.MusicBrainzRecording.ToString()] = IsGuid, + [MetadataProvider.MusicBrainzTrack.ToString()] = IsGuid + }; + + /// <summary> /// Checks if this instance has an id for the given provider. /// </summary> /// <param name="instance">The instance.</param> @@ -102,6 +125,26 @@ public static class ProviderIdsExtensions } /// <summary> + /// Checks whether a value can be an id of the given provider. + /// </summary> + /// <param name="name">The provider name.</param> + /// <param name="value">The provider id.</param> + /// <returns><c>true</c> if the value has a plausible format for the provider; otherwise, <c>false</c>.</returns> + /// <remarks> + /// Providers regularly hand out an id belonging to a different service, e.g. an IMDb person id in the + /// TMDb field. Such an id is not just useless, it also makes the owning provider fail for the item. + /// </remarks> + public static bool IsValidProviderId(string? name, string? value) + { + if (string.IsNullOrWhiteSpace(name) || string.IsNullOrWhiteSpace(value)) + { + return false; + } + + return !_providerIdValidators.TryGetValue(name, out var isValid) || isValid(value); + } + + /// <summary> /// Sets a provider id. /// </summary> /// <param name="instance">The instance.</param> @@ -121,6 +164,14 @@ public static class ProviderIdsExtensions return false; } + name = name.Trim(); + value = value.Trim(); + + if (!IsValidProviderId(name, value)) + { + return false; + } + // Ensure it exists instance.ProviderIds ??= new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); @@ -153,7 +204,6 @@ public static class ProviderIdsExtensions /// <param name="instance">The instance.</param> /// <param name="name">The name, this should not contain a '=' character.</param> /// <param name="value">The value.</param> - /// <remarks>Due to how deserialization from the database works the name cannot contain '='.</remarks> public static void SetProviderId(this IHasProviderIds instance, string name, string value) { ArgumentNullException.ThrowIfNull(instance); @@ -166,17 +216,27 @@ public static class ProviderIdsExtensions throw new ArgumentException("Provider id name cannot contain '='", nameof(name)); } - // Ensure it exists - instance.ProviderIds ??= new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + instance.TrySetProviderId(name, value); + } - // Match on internal MetadataProvider enum string values before adding arbitrary providers - if (_metadataProviderEnumDictionary.TryGetValue(name, out var enumValue)) + /// <summary> + /// Replaces all provider ids, dropping the ones that cannot belong to the provider they are filed under. + /// </summary> + /// <param name="instance">The instance.</param> + /// <param name="providerIds">The provider ids to set.</param> + public static void SetProviderIds(this IHasProviderIds instance, IReadOnlyDictionary<string, string>? providerIds) + { + ArgumentNullException.ThrowIfNull(instance); + + instance.ProviderIds = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + if (providerIds is null) { - instance.ProviderIds[enumValue] = value; + return; } - else + + foreach (var (name, value) in providerIds) { - instance.ProviderIds[name] = value; + instance.TrySetProviderId(name, value); } } @@ -213,4 +273,15 @@ public static class ProviderIdsExtensions instance.ProviderIds?.Remove(provider.ToString()); } + + private static bool IsPositiveNumber(string value) + => int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out var id) && id > 0; + + private static bool IsGuid(string value) + => Guid.TryParse(value, CultureInfo.InvariantCulture, out _); + + // An IMDb id is a type prefix (tt for titles, nm for people, co for companies, ...) followed by + // digits. The prefix is optional because a bare number has always been accepted for a title. + [GeneratedRegex(@"^(tt|nm|co|ev|ch|ni)?[0-9]+$", RegexOptions.IgnoreCase)] + private static partial Regex ImdbIdRegex(); } diff --git a/MediaBrowser.Providers/Manager/MetadataService.cs b/MediaBrowser.Providers/Manager/MetadataService.cs index 40f2775bd3..d11db8f531 100644 --- a/MediaBrowser.Providers/Manager/MetadataService.cs +++ b/MediaBrowser.Providers/Manager/MetadataService.cs @@ -260,21 +260,40 @@ namespace MediaBrowser.Providers.Manager switch (lookupInfo) { case EpisodeInfo episodeInfo: - episodeInfo.SeriesProviderIds = result.ProviderIds; + episodeInfo.SeriesProviderIds = GetValidProviderIds(result.ProviderIds); episodeInfo.ProviderIds.Clear(); break; case SeasonInfo seasonInfo: - seasonInfo.SeriesProviderIds = result.ProviderIds; + seasonInfo.SeriesProviderIds = GetValidProviderIds(result.ProviderIds); seasonInfo.ProviderIds.Clear(); break; default: - lookupInfo.ProviderIds = result.ProviderIds; + lookupInfo.SetProviderIds(result.ProviderIds); lookupInfo.Name = result.Name; lookupInfo.Year = result.ProductionYear; break; } } + private static Dictionary<string, string> GetValidProviderIds(IReadOnlyDictionary<string, string> providerIds) + { + var validProviderIds = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + if (providerIds is null) + { + return validProviderIds; + } + + foreach (var (name, value) in providerIds) + { + if (ProviderIdsExtensions.IsValidProviderId(name, value)) + { + validProviderIds[name] = value; + } + } + + return validProviderIds; + } + protected async Task SaveItemAsync(MetadataResult<TItemType> result, ItemUpdateType reason, bool reattachUserData, CancellationToken cancellationToken) { await result.Item.UpdateToRepositoryAsync(reason, cancellationToken).ConfigureAwait(false); @@ -835,6 +854,7 @@ namespace MediaBrowser.Providers.Manager } } + var hasRemoteMetadata = false; var isLocalLocked = temp.Item.IsLocked; if (!isLocalLocked && (options.ReplaceAllMetadata || options.MetadataRefreshMode > MetadataRefreshMode.ValidationOnly)) { @@ -849,6 +869,7 @@ namespace MediaBrowser.Providers.Manager var remoteResult = await ExecuteRemoteProviders(temp, logName, false, id, remoteProviders, cancellationToken).ConfigureAwait(false); + hasRemoteMetadata = remoteResult.UpdateType.HasFlag(ItemUpdateType.MetadataDownload); refreshResult.UpdateType |= remoteResult.UpdateType; refreshResult.ErrorMessage = remoteResult.ErrorMessage; refreshResult.Failures += remoteResult.Failures; @@ -858,7 +879,12 @@ namespace MediaBrowser.Providers.Manager { if (refreshResult.UpdateType > ItemUpdateType.None) { - if (!options.RemoveOldMetadata) + // Erasing the old values is only safe when a remote provider returned something to + // replace them with. If every one of them failed there is no replacement, and wiping the + // item would turn a provider being temporarily unreachable into permanent data loss. + // A single failure is not enough: Identify asks for the erasure precisely because the + // previous match was wrong, and an unrelated provider throwing must not undo that. + if (!options.RemoveOldMetadata || (refreshResult.Failures > 0 && !hasRemoteMetadata)) { // Add existing metadata to provider result if it does not exist there MergeData(metadata, temp, [], false, false); @@ -932,6 +958,8 @@ namespace MediaBrowser.Providers.Manager { result.Provider = provider.Name; + LogInvalidProviderIds(result, providerName, logName); + MergeData(result, temp, [], replaceData, false); MergeNewData(temp.Item, id); @@ -957,6 +985,58 @@ namespace MediaBrowser.Providers.Manager return refreshResult; } + /// <summary> + /// Reports the ids a provider returned that cannot belong to the provider they are filed under. + /// </summary> + /// <remarks> + /// The ids are dropped when merging, this names the provider that produced them so the source of a + /// recurring bad id can be found. + /// </remarks> + private void LogInvalidProviderIds(MetadataResult<TItemType> result, string providerName, string logName) + { + if (!Logger.IsEnabled(LogLevel.Debug)) + { + return; + } + + LogInvalidProviderIds(result.Item?.ProviderIds, providerName, logName, null); + + if (result.People is null) + { + return; + } + + foreach (var person in result.People) + { + LogInvalidProviderIds(person.ProviderIds, providerName, logName, person.Name); + } + } + + private void LogInvalidProviderIds(IReadOnlyDictionary<string, string> providerIds, string providerName, string logName, string personName) + { + if (providerIds is null) + { + return; + } + + foreach (var (key, value) in providerIds) + { + if (ProviderIdsExtensions.IsValidProviderId(key, value)) + { + continue; + } + + if (personName is null) + { + Logger.LogDebug("Discarding {Key} id '{Value}' returned by {Provider} for {Item}", key, value, providerName, logName); + } + else + { + Logger.LogDebug("Discarding {Key} id '{Value}' returned by {Provider} for {Person} of {Item}", key, value, providerName, personName, logName); + } + } + } + private void MergeNewData(TItemType source, TIdType lookupInfo) { // Copy new provider id's that may have been obtained @@ -964,8 +1044,18 @@ namespace MediaBrowser.Providers.Manager { var key = providerId.Key; - // Don't replace existing Id's. - lookupInfo.ProviderIds.TryAdd(key, providerId.Value); + if (!ProviderIdsExtensions.IsValidProviderId(key, providerId.Value)) + { + continue; + } + + // Don't replace existing Id's, unless the one already there is unusable - handing that + // one to the providers that have yet to run is what makes them fail. + if (!lookupInfo.ProviderIds.TryGetValue(key, out var existingId) + || !ProviderIdsExtensions.IsValidProviderId(key, existingId)) + { + lookupInfo.ProviderIds[key] = providerId.Value; + } } } @@ -1104,6 +1194,9 @@ namespace MediaBrowser.Providers.Manager if (!lockedFields.Contains(MetadataField.Cast)) { + RemoveInvalidProviderIds(sourceResult.People); + RemoveInvalidProviderIds(targetResult.People); + if (replaceData || targetResult.People is null || targetResult.People.Count == 0) { targetResult.People = sourceResult.People; @@ -1175,17 +1268,33 @@ namespace MediaBrowser.Providers.Manager { var key = id.Key; - // Don't replace existing Id's. - if (replaceData) + // An id that cannot belong to the provider it is filed under only breaks that provider on + // the next refresh, so never let one in - not even when replacing all metadata. + if (!ProviderIdsExtensions.IsValidProviderId(key, id.Value)) { - target.ProviderIds[key] = id.Value; + continue; } - else + + // Don't replace existing Id's, unless the stored one is unusable - that one is the bad + // match the refresh is meant to repair. + if (replaceData + || !target.ProviderIds.TryGetValue(key, out var existingId) + || !ProviderIdsExtensions.IsValidProviderId(key, existingId)) { - target.ProviderIds.TryAdd(key, id.Value); + target.ProviderIds[key] = id.Value; } } + // A bad id no provider offered a replacement for still has to go, otherwise the item keeps + // failing the same way on every refresh. + foreach (var key in target.ProviderIds + .Where(id => !ProviderIdsExtensions.IsValidProviderId(id.Key, id.Value)) + .Select(id => id.Key) + .ToArray()) + { + target.ProviderIds.Remove(key); + } + if (replaceData || !target.CriticRating.HasValue) { target.CriticRating = source.CriticRating; @@ -1251,6 +1360,32 @@ namespace MediaBrowser.Providers.Manager } } + private static void RemoveInvalidProviderIds(IReadOnlyList<PersonInfo> people) + { + if (people is null) + { + return; + } + + foreach (var person in people) + { + if (person.ProviderIds is null || person.ProviderIds.Count == 0) + { + continue; + } + + var invalidKeys = person.ProviderIds + .Where(id => !ProviderIdsExtensions.IsValidProviderId(id.Key, id.Value)) + .Select(id => id.Key) + .ToArray(); + + foreach (var key in invalidKeys) + { + person.ProviderIds.Remove(key); + } + } + } + private static void MergePeople(IReadOnlyList<PersonInfo> source, IReadOnlyList<PersonInfo> target) { var sourceByName = source.ToLookup(p => p.Name.RemoveDiacritics(), StringComparer.OrdinalIgnoreCase); diff --git a/MediaBrowser.Providers/Music/AlbumInfoExtensions.cs b/MediaBrowser.Providers/Music/AlbumInfoExtensions.cs index d3fce37c71..2923dd3290 100644 --- a/MediaBrowser.Providers/Music/AlbumInfoExtensions.cs +++ b/MediaBrowser.Providers/Music/AlbumInfoExtensions.cs @@ -23,11 +23,11 @@ namespace MediaBrowser.Providers.Music public static string? GetReleaseGroupId(this AlbumInfo info) { - var id = info.GetProviderId(MetadataProvider.MusicBrainzReleaseGroup); + var id = MusicBrainzId(MetadataProvider.MusicBrainzReleaseGroup, info.GetProviderId(MetadataProvider.MusicBrainzReleaseGroup)); if (string.IsNullOrEmpty(id)) { - return info.SongInfos.Select(i => i.GetProviderId(MetadataProvider.MusicBrainzReleaseGroup)) + return info.SongInfos.Select(i => MusicBrainzId(MetadataProvider.MusicBrainzReleaseGroup, i.GetProviderId(MetadataProvider.MusicBrainzReleaseGroup))) .FirstOrDefault(i => !string.IsNullOrEmpty(i)); } @@ -36,11 +36,11 @@ namespace MediaBrowser.Providers.Music public static string? GetReleaseId(this AlbumInfo info) { - var id = info.GetProviderId(MetadataProvider.MusicBrainzAlbum); + var id = MusicBrainzId(MetadataProvider.MusicBrainzAlbum, info.GetProviderId(MetadataProvider.MusicBrainzAlbum)); if (string.IsNullOrEmpty(id)) { - return info.SongInfos.Select(i => i.GetProviderId(MetadataProvider.MusicBrainzAlbum)) + return info.SongInfos.Select(i => MusicBrainzId(MetadataProvider.MusicBrainzAlbum, i.GetProviderId(MetadataProvider.MusicBrainzAlbum))) .FirstOrDefault(i => !string.IsNullOrEmpty(i)); } @@ -50,15 +50,17 @@ namespace MediaBrowser.Providers.Music public static string? GetMusicBrainzArtistId(this AlbumInfo info) { info.ProviderIds.TryGetValue(MetadataProvider.MusicBrainzAlbumArtist.ToString(), out string? id); + id = MusicBrainzId(MetadataProvider.MusicBrainzAlbumArtist, id); if (string.IsNullOrEmpty(id)) { info.ArtistProviderIds.TryGetValue(MetadataProvider.MusicBrainzArtist.ToString(), out id); + id = MusicBrainzId(MetadataProvider.MusicBrainzArtist, id); } if (string.IsNullOrEmpty(id)) { - return info.SongInfos.Select(i => i.GetProviderId(MetadataProvider.MusicBrainzAlbumArtist)) + return info.SongInfos.Select(i => MusicBrainzId(MetadataProvider.MusicBrainzAlbumArtist, i.GetProviderId(MetadataProvider.MusicBrainzAlbumArtist))) .FirstOrDefault(i => !string.IsNullOrEmpty(i)); } @@ -68,14 +70,21 @@ namespace MediaBrowser.Providers.Music public static string? GetMusicBrainzArtistId(this ArtistInfo info) { info.ProviderIds.TryGetValue(MetadataProvider.MusicBrainzArtist.ToString(), out var id); + id = MusicBrainzId(MetadataProvider.MusicBrainzArtist, id); if (string.IsNullOrEmpty(id)) { - return info.SongInfos.Select(i => i.GetProviderId(MetadataProvider.MusicBrainzAlbumArtist)) + return info.SongInfos.Select(i => MusicBrainzId(MetadataProvider.MusicBrainzAlbumArtist, i.GetProviderId(MetadataProvider.MusicBrainzAlbumArtist))) .FirstOrDefault(i => !string.IsNullOrEmpty(i)); } return id; } + + /// <summary> + /// Returns the id if it can be an id of the given provider, otherwise <c>null</c>. + /// </summary> + private static string? MusicBrainzId(MetadataProvider provider, string? id) + => ProviderIdsExtensions.IsValidProviderId(provider.ToString(), id) ? id : null; } } diff --git a/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetImageProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetImageProvider.cs index 78be5804e3..23f8d89c67 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetImageProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetImageProvider.cs @@ -1,6 +1,4 @@ -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Threading; @@ -56,7 +54,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.BoxSets /// <inheritdoc /> public async Task<IEnumerable<RemoteImageInfo>> GetImages(BaseItem item, CancellationToken cancellationToken) { - var tmdbId = Convert.ToInt32(item.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + item.TryGetTmdbId(out var tmdbId); if (tmdbId <= 0) { diff --git a/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetProvider.cs index a7bba2d539..11ac477378 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/BoxSets/TmdbBoxSetProvider.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -42,7 +41,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.BoxSets /// <inheritdoc /> public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(BoxSetInfo searchInfo, CancellationToken cancellationToken) { - var tmdbId = Convert.ToInt32(searchInfo.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + searchInfo.TryGetTmdbId(out var tmdbId); var language = searchInfo.MetadataLanguage; if (tmdbId > 0) @@ -97,7 +96,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.BoxSets /// <inheritdoc /> public async Task<MetadataResult<BoxSet>> GetMetadata(BoxSetInfo info, CancellationToken cancellationToken) { - var tmdbId = Convert.ToInt32(info.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + info.TryGetTmdbId(out var tmdbId); var language = info.MetadataLanguage; // We don't already have an Id, need to fetch it diff --git a/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieImageProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieImageProvider.cs index b188f5deb4..e686577311 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieImageProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieImageProvider.cs @@ -1,6 +1,4 @@ -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Threading; @@ -61,7 +59,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.Movies var language = item.GetPreferredMetadataLanguage(); var countryCode = item.GetPreferredMetadataCountryCode(); - var movieTmdbId = Convert.ToInt32(item.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + item.TryGetTmdbId(out var movieTmdbId); if (movieTmdbId <= 0) { var movieImdbId = item.GetProviderId(MetadataProvider.Imdb); diff --git a/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieProvider.cs index 8811a1787a..ef952082da 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/Movies/TmdbMovieProvider.cs @@ -54,11 +54,11 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.Movies /// <inheritdoc /> public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(MovieInfo searchInfo, CancellationToken cancellationToken) { - if (searchInfo.TryGetProviderId(MetadataProvider.Tmdb, out var id)) + if (searchInfo.TryGetTmdbId(out var tmdbId)) { var movie = await _tmdbClientManager .GetMovieAsync( - int.Parse(id, CultureInfo.InvariantCulture), + tmdbId, searchInfo.MetadataLanguage, TmdbUtils.GetImageLanguagesParam(searchInfo.MetadataLanguage, searchInfo.MetadataCountryCode), searchInfo.MetadataCountryCode, @@ -90,7 +90,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.Movies } IReadOnlyList<SearchMovie>? movieResults = null; - if (searchInfo.TryGetProviderId(MetadataProvider.Imdb, out id)) + if (searchInfo.TryGetProviderId(MetadataProvider.Imdb, out var id)) { var result = await _tmdbClientManager.FindByExternalIdAsync( id, @@ -151,11 +151,13 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.Movies /// <inheritdoc /> public async Task<MetadataResult<Movie>> GetMetadata(MovieInfo info, CancellationToken cancellationToken) { - var tmdbId = info.GetProviderId(MetadataProvider.Tmdb); + // A stored id that is not a TMDb id is treated as no id, so the search below can repair it + // rather than the lookup failing for as long as the bad id stays on the item. + info.TryGetTmdbId(out var tmdbId); var imdbId = info.GetProviderId(MetadataProvider.Imdb); var config = Plugin.Instance.Configuration; - if (string.IsNullOrEmpty(tmdbId) && string.IsNullOrEmpty(imdbId)) + if (tmdbId <= 0 && string.IsNullOrEmpty(imdbId)) { // ParseName is required here. // Caller provides the filename with extension stripped and NOT the parsed filename @@ -166,26 +168,26 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.Movies if (searchResults?.Count > 0) { - tmdbId = searchResults[0].Id.ToString(CultureInfo.InvariantCulture); + tmdbId = searchResults[0].Id; } } - if (string.IsNullOrEmpty(tmdbId) && !string.IsNullOrEmpty(imdbId)) + if (tmdbId <= 0 && !string.IsNullOrEmpty(imdbId)) { var movieResultFromImdbId = await _tmdbClientManager.FindByExternalIdAsync(imdbId, FindExternalSource.Imdb, info.MetadataLanguage, info.MetadataCountryCode, cancellationToken).ConfigureAwait(false); if (movieResultFromImdbId?.MovieResults?.Count > 0) { - tmdbId = movieResultFromImdbId.MovieResults[0].Id.ToString(CultureInfo.InvariantCulture); + tmdbId = movieResultFromImdbId.MovieResults[0].Id; } } - if (string.IsNullOrEmpty(tmdbId)) + if (tmdbId <= 0) { return new MetadataResult<Movie>(); } var movieResult = await _tmdbClientManager - .GetMovieAsync(Convert.ToInt32(tmdbId, CultureInfo.InvariantCulture), info.MetadataLanguage, TmdbUtils.GetImageLanguagesParam(info.MetadataLanguage, info.MetadataCountryCode), info.MetadataCountryCode, cancellationToken) + .GetMovieAsync(tmdbId, info.MetadataLanguage, TmdbUtils.GetImageLanguagesParam(info.MetadataLanguage, info.MetadataCountryCode), info.MetadataCountryCode, cancellationToken) .ConfigureAwait(false); if (movieResult is null) @@ -208,7 +210,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.Movies Item = movie }; - movie.SetProviderId(MetadataProvider.Tmdb, tmdbId); + movie.SetProviderId(MetadataProvider.Tmdb, tmdbId.ToString(CultureInfo.InvariantCulture)); movie.TrySetProviderId(MetadataProvider.Imdb, movieResult.ImdbId); if (movieResult.BelongsToCollection is not null) { diff --git a/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonImageProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonImageProvider.cs index 33888ddf4f..d38614811c 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonImageProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonImageProvider.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Threading; @@ -54,14 +53,14 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.People { var person = (Person)item; - if (!person.TryGetProviderId(MetadataProvider.Tmdb, out var personTmdbId)) + if (!person.TryGetTmdbId(out var personTmdbId)) { return Enumerable.Empty<RemoteImageInfo>(); } var language = item.GetPreferredMetadataLanguage(); var countryCode = item.GetPreferredMetadataCountryCode(); - var personResult = await _tmdbClientManager.GetPersonAsync(int.Parse(personTmdbId, CultureInfo.InvariantCulture), language, countryCode, cancellationToken).ConfigureAwait(false); + var personResult = await _tmdbClientManager.GetPersonAsync(personTmdbId, language, countryCode, cancellationToken).ConfigureAwait(false); if (personResult?.Images?.Profiles is null) { return Enumerable.Empty<RemoteImageInfo>(); diff --git a/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonProvider.cs index 64ab98b262..61294676f7 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/People/TmdbPersonProvider.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using System.Globalization; using System.Net.Http; @@ -37,9 +36,9 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.People /// <inheritdoc /> public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(PersonLookupInfo searchInfo, CancellationToken cancellationToken) { - if (searchInfo.TryGetProviderId(MetadataProvider.Tmdb, out var personTmdbId)) + if (searchInfo.TryGetTmdbId(out var personTmdbId)) { - var personResult = await _tmdbClientManager.GetPersonAsync(int.Parse(personTmdbId, CultureInfo.InvariantCulture), searchInfo.MetadataLanguage, searchInfo.MetadataCountryCode, cancellationToken).ConfigureAwait(false); + var personResult = await _tmdbClientManager.GetPersonAsync(personTmdbId, searchInfo.MetadataLanguage, searchInfo.MetadataCountryCode, cancellationToken).ConfigureAwait(false); if (personResult is not null) { @@ -89,7 +88,9 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.People /// <inheritdoc /> public async Task<MetadataResult<Person>> GetMetadata(PersonLookupInfo info, CancellationToken cancellationToken) { - var personTmdbId = Convert.ToInt32(info.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + // A person can carry another provider's id under the TMDb key, which is no more usable here + // than no id at all, so both take the search path and get the stored id repaired. + info.TryGetTmdbId(out var personTmdbId); // We don't already have an Id, need to fetch it if (personTmdbId <= 0) diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeImageProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeImageProvider.cs index 7ae54cdcd3..1f8c87397d 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeImageProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeImageProvider.cs @@ -1,6 +1,4 @@ -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Threading; @@ -56,9 +54,9 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV var episode = (Controller.Entities.TV.Episode)item; var series = episode.Series; - var seriesTmdbId = Convert.ToInt32(series?.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + var seriesTmdbId = 0; - if (series is null || seriesTmdbId <= 0) + if (series?.TryGetTmdbId(out seriesTmdbId) != true) { return Enumerable.Empty<RemoteImageInfo>(); } diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeProvider.cs index 21b822c97c..8172ab14df 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbEpisodeProvider.cs @@ -91,8 +91,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV info.SeriesProviderIds.TryGetValue(MetadataProvider.Tmdb.ToString(), out string? tmdbId); - var seriesTmdbId = Convert.ToInt32(tmdbId, CultureInfo.InvariantCulture); - if (seriesTmdbId <= 0) + if (!TmdbUtils.TryParseTmdbId(tmdbId, out var seriesTmdbId)) { return metadataResult; } diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonImageProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonImageProvider.cs index 5b2f0d26e4..bc44d0266d 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonImageProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonImageProvider.cs @@ -1,6 +1,4 @@ -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Threading; @@ -57,9 +55,9 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV var season = (Season)item; var series = season?.Series; - var seriesTmdbId = Convert.ToInt32(series?.GetProviderId(MetadataProvider.Tmdb), CultureInfo.InvariantCulture); + var seriesTmdbId = 0; - if (seriesTmdbId <= 0 || season?.IndexNumber is null) + if (season?.IndexNumber is null || series?.TryGetTmdbId(out seriesTmdbId) != true) { return Enumerable.Empty<RemoteImageInfo>(); } diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonProvider.cs index 9c41d64253..06313810a1 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeasonProvider.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using System.Globalization; using System.Linq; @@ -48,13 +47,13 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV var seasonNumber = info.IndexNumber; - if (string.IsNullOrWhiteSpace(seriesTmdbId) || !seasonNumber.HasValue) + if (!seasonNumber.HasValue || !TmdbUtils.TryParseTmdbId(seriesTmdbId, out var seriesId)) { return result; } var seasonResult = await _tmdbClientManager - .GetSeasonAsync(Convert.ToInt32(seriesTmdbId, CultureInfo.InvariantCulture), seasonNumber.Value, info.MetadataLanguage, TmdbUtils.GetImageLanguagesParam(info.MetadataLanguage, info.MetadataCountryCode), info.MetadataCountryCode, cancellationToken) + .GetSeasonAsync(seriesId, seasonNumber.Value, info.MetadataLanguage, TmdbUtils.GetImageLanguagesParam(info.MetadataLanguage, info.MetadataCountryCode), info.MetadataCountryCode, cancellationToken) .ConfigureAwait(false); if (seasonResult is null) diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesImageProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesImageProvider.cs index f2e7d0c6e4..dc4f860604 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesImageProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesImageProvider.cs @@ -1,6 +1,4 @@ -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; using System.Net.Http; using System.Threading; @@ -57,9 +55,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV /// <inheritdoc /> public async Task<IEnumerable<RemoteImageInfo>> GetImages(BaseItem item, CancellationToken cancellationToken) { - var tmdbId = item.GetProviderId(MetadataProvider.Tmdb); - - if (string.IsNullOrEmpty(tmdbId)) + if (!item.TryGetTmdbId(out var tmdbId)) { return Enumerable.Empty<RemoteImageInfo>(); } @@ -68,7 +64,7 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV // TODO use image languages if All Languages isn't toggled, but there's currently no way to get that value in here var series = await _tmdbClientManager - .GetSeriesAsync(Convert.ToInt32(tmdbId, CultureInfo.InvariantCulture), null, null, null, cancellationToken) + .GetSeriesAsync(tmdbId, null, null, null, cancellationToken) .ConfigureAwait(false); if (series?.Images is null) diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesProvider.cs b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesProvider.cs index 9bb15ca479..9e201f2d7c 100755 --- a/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesProvider.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TV/TmdbSeriesProvider.cs @@ -54,10 +54,10 @@ namespace MediaBrowser.Providers.Plugins.Tmdb.TV /// <inheritdoc /> public async Task<IEnumerable<RemoteSearchResult>> GetSearchResults(SeriesInfo searchInfo, CancellationToken cancellationToken) { - if (searchInfo.TryGetProviderId(MetadataProvider.Tmdb, out var tmdbId)) + if (searchInfo.TryGetTmdbId(out var tmdbId)) { var series = await _tmdbClientManager - .GetSeriesAsync(Convert.ToInt32(tmdbId, CultureInfo.InvariantCulture), searchInfo.MetadataLanguage, searchInfo.MetadataLanguage, searchInfo.MetadataCountryCode, cancellationToken) + .GetSeriesAsync(tmdbId, searchInfo.MetadataLanguage, searchInfo.MetadataLanguage, searchInfo.MetadataCountryCode, cancellationToken) .ConfigureAwait(false); if (series is not null) diff --git a/MediaBrowser.Providers/Plugins/Tmdb/TmdbUtils.cs b/MediaBrowser.Providers/Plugins/Tmdb/TmdbUtils.cs index 7e6b9beee9..c83174f97f 100644 --- a/MediaBrowser.Providers/Plugins/Tmdb/TmdbUtils.cs +++ b/MediaBrowser.Providers/Plugins/Tmdb/TmdbUtils.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Frozen; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Text.RegularExpressions; using Jellyfin.Data.Enums; using MediaBrowser.Model.Entities; @@ -63,6 +64,33 @@ namespace MediaBrowser.Providers.Plugins.Tmdb private static partial Regex NonWordRegex(); /// <summary> + /// Gets the TMDb id of an item, if it has one TMDb can be queried with. + /// </summary> + /// <param name="instance">The item.</param> + /// <param name="tmdbId">The TMDb id.</param> + /// <returns><c>true</c> if the item has a usable TMDb id; otherwise, <c>false</c>.</returns> + public static bool TryGetTmdbId(this IHasProviderIds instance, out int tmdbId) + { + instance.TryGetProviderId(MetadataProvider.Tmdb, out var value); + + return TryParseTmdbId(value, out tmdbId); + } + + /// <summary> + /// Parses a TMDb id. + /// </summary> + /// <param name="value">The stored id.</param> + /// <param name="tmdbId">The TMDb id.</param> + /// <returns><c>true</c> if the value is a usable TMDb id; otherwise, <c>false</c>.</returns> + public static bool TryParseTmdbId(string? value, out int tmdbId) + { + // Another provider can have filed one of its own ids under the TMDb key, e.g. an IMDb person + // id. Reporting that as "no id" lets the caller fall back to a search and repair the id, + // instead of throwing on every refresh of the item. + return int.TryParse(value, NumberStyles.None, CultureInfo.InvariantCulture, out tmdbId) && tmdbId > 0; + } + + /// <summary> /// Cleans the name according to TMDb requirements. /// </summary> /// <param name="name">The name of the entity.</param> diff --git a/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs b/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs index a6f4164144..2347c08961 100644 --- a/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs +++ b/tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs @@ -186,6 +186,109 @@ namespace Jellyfin.Model.Tests.Entities Assert.Null(nullProvider.ProviderIds); } + [Theory] + [InlineData(nameof(MetadataProvider.Imdb), "tt0113375", true)] + [InlineData(nameof(MetadataProvider.Imdb), "nm0000123", true)] + [InlineData(nameof(MetadataProvider.Imdb), "0113375", true)] + [InlineData(nameof(MetadataProvider.Imdb), "https://www.imdb.com/title/tt0113375", false)] + [InlineData(nameof(MetadataProvider.Tmdb), "11", true)] + [InlineData(nameof(MetadataProvider.Tmdb), "nm0000123", false)] + [InlineData(nameof(MetadataProvider.Tmdb), "0", false)] + [InlineData(nameof(MetadataProvider.Tmdb), "-11", false)] + [InlineData(nameof(MetadataProvider.TmdbCollection), "nm0000123", false)] + [InlineData(nameof(MetadataProvider.AudioDbArtist), "111239", true)] + [InlineData(nameof(MetadataProvider.AudioDbArtist), "a3cb23fc-acd3-4ce0-8f36-1e5aa6a18432", false)] + [InlineData(nameof(MetadataProvider.MusicBrainzArtist), "a3cb23fc-acd3-4ce0-8f36-1e5aa6a18432", true)] + [InlineData(nameof(MetadataProvider.MusicBrainzArtist), "111239", false)] + [InlineData(nameof(MetadataProvider.MusicBrainzAlbum), "not-an-mbid", false)] + [InlineData(nameof(MetadataProvider.Tvdb), "anything-goes", true)] + [InlineData("SomePlugin", "anything-goes", true)] + [InlineData(nameof(MetadataProvider.Tmdb), null, false)] + [InlineData(null, "11", false)] + public void IsValidProviderId_ChecksKnownFormats(string? name, string? value, bool expected) + { + Assert.Equal(expected, ProviderIdsExtensions.IsValidProviderId(name, value)); + } + + [Fact] + public void TrySetProviderId_ForeignId_False() + { + var provider = new ProviderIdsExtensionsTestsObject(); + + Assert.False(provider.TrySetProviderId(MetadataProvider.Tmdb, "nm0000123")); + Assert.Empty(provider.ProviderIds); + } + + [Fact] + public void TrySetProviderId_ForeignId_KeepsExisting() + { + var provider = new ProviderIdsExtensionsTestsObject(); + provider.ProviderIds[MetadataProvider.Tmdb.ToString()] = "11"; + + Assert.False(provider.TrySetProviderId(MetadataProvider.Tmdb, "nm0000123")); + Assert.Equal("11", provider.GetProviderId(MetadataProvider.Tmdb)); + } + + [Theory] + [InlineData(nameof(MetadataProvider.Imdb), " tt0113375 ")] + [InlineData(" Imdb", ExampleImdbId)] + public void TrySetProviderId_SurroundingWhitespace_Trimmed(string name, string value) + { + var provider = new ProviderIdsExtensionsTestsObject(); + + Assert.True(provider.TrySetProviderId(name, value)); + Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb)); + } + + [Fact] + public void SetProviderIds_ReplacesAll() + { + var provider = new ProviderIdsExtensionsTestsObject(); + provider.ProviderIds[MetadataProvider.Tvdb.ToString()] = "12345"; + + provider.SetProviderIds(new Dictionary<string, string> + { + [MetadataProvider.Imdb.ToString()] = ExampleImdbId + }); + + Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb)); + Assert.False(provider.HasProviderId(MetadataProvider.Tvdb)); + } + + [Fact] + public void SetProviderIds_ForeignId_Dropped() + { + var provider = new ProviderIdsExtensionsTestsObject(); + + provider.SetProviderIds(new Dictionary<string, string> + { + [MetadataProvider.Tmdb.ToString()] = "nm0000123", + [MetadataProvider.Imdb.ToString()] = ExampleImdbId, + [MetadataProvider.Tvdb.ToString()] = string.Empty + }); + + Assert.False(provider.HasProviderId(MetadataProvider.Tmdb)); + Assert.False(provider.HasProviderId(MetadataProvider.Tvdb)); + Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb)); + } + + [Fact] + public void SetProviderIds_Null_Clears() + { + var provider = new ProviderIdsExtensionsTestsObject(); + provider.ProviderIds[MetadataProvider.Imdb.ToString()] = ExampleImdbId; + + provider.SetProviderIds(null); + + Assert.Empty(provider.ProviderIds); + } + + [Fact] + public void SetProviderIds_NullInstance_ThrowsArgumentNullException() + { + Assert.Throws<ArgumentNullException>(() => ProviderIdsExtensions.SetProviderIds(null!, new Dictionary<string, string>())); + } + [Fact] public void RemoveProviderId_Null_Remove() { diff --git a/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs b/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs new file mode 100644 index 0000000000..1d2fb2e760 --- /dev/null +++ b/tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs @@ -0,0 +1,253 @@ +using System; +using System.Collections.Generic; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Jellyfin.Data.Enums; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Entities.Movies; +using MediaBrowser.Controller.IO; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Persistence; +using MediaBrowser.Controller.Providers; +using MediaBrowser.Model.Entities; +using MediaBrowser.Model.IO; +using MediaBrowser.Providers.Manager; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Providers.Tests.Manager +{ + public class MetadataServiceRefreshTests + { + [Theory] + // RemoveOldMetadata is only ever set by an explicit user action - a refresh with "replace all + // metadata", or Identify. A provider failing must not silently downgrade that to a merge: the + // providers that did answer supplied the replacement, and the old values are the wrong match + // the user asked to get rid of. + [InlineData(false)] + [InlineData(true)] + public async Task RefreshWithProviders_ReplaceAllMetadata_ErasesOldDataWhenAProviderAnswers(bool allProvidersSucceed) + { + var item = new Movie + { + Name = "Test Movie", + Overview = "existing overview" + }; + + // The provider owning the overview fails, so it contributes nothing to the replacement. + var failing = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + failing.Setup(p => p.Name).Returns("Failing"); + failing.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .Returns(allProvidersSucceed + ? Task.FromResult(new MetadataResult<Movie> { HasMetadata = true, Item = new Movie() }) + : Task.FromException<MetadataResult<Movie>>(new FormatException("bad id"))); + + var succeeding = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + succeeding.Setup(p => p.Name).Returns("Succeeding"); + succeeding.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .ReturnsAsync(new MetadataResult<Movie> + { + HasMetadata = true, + Item = new Movie { Name = "Test Movie", Tagline = "new tagline" } + }); + + var service = new TestMetadataService(); + var result = await service.RefreshWithProvidersInternal( + new MetadataResult<Movie> { Item = item }, + new MovieInfo { Name = item.Name }, + new MetadataRefreshOptions(Mock.Of<IDirectoryService>()) + { + MetadataRefreshMode = MetadataRefreshMode.FullRefresh, + ReplaceAllMetadata = true, + RemoveOldMetadata = true + }, + [failing.Object, succeeding.Object]).ConfigureAwait(true); + + Assert.Equal(allProvidersSucceed ? 0 : 1, result.Failures); + Assert.Equal("new tagline", item.Tagline); + Assert.Null(item.Overview); + } + + [Fact] + public async Task RefreshWithProviders_ReplaceAllMetadata_KeepsExistingDataWhenEveryRemoteProviderFails() + { + var item = new Movie + { + Name = "Test Movie", + Overview = "existing overview" + }; + + // Something has to contribute for the merge to run at all, otherwise the item is never touched + // and the case is moot. The local provider is the replacement the remote ones did not deliver. + var local = new Mock<ILocalMetadataProvider<Movie>>(MockBehavior.Loose); + local.Setup(p => p.Name).Returns("Local"); + local.Setup(p => p.GetMetadata(It.IsAny<ItemInfo>(), It.IsAny<IDirectoryService>(), It.IsAny<CancellationToken>())) + .ReturnsAsync(new MetadataResult<Movie> + { + HasMetadata = true, + Item = new Movie { Name = "Test Movie", Tagline = "new tagline" } + }); + + var remote = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + remote.Setup(p => p.Name).Returns("Failing"); + remote.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .Returns(Task.FromException<MetadataResult<Movie>>(new HttpRequestException("unreachable"))); + + var service = new TestMetadataService(); + var result = await service.RefreshWithProvidersInternal( + new MetadataResult<Movie> { Item = item }, + new MovieInfo { Name = item.Name }, + new MetadataRefreshOptions(Mock.Of<IDirectoryService>()) + { + MetadataRefreshMode = MetadataRefreshMode.FullRefresh, + ReplaceAllMetadata = true, + RemoveOldMetadata = true + }, + [local.Object, remote.Object]).ConfigureAwait(true); + + Assert.Equal(1, result.Failures); + Assert.Equal("new tagline", item.Tagline); + + // No remote provider answered, so erasing the overview would lose it for good. + Assert.Equal("existing overview", item.Overview); + } + + [Fact] + public async Task RefreshWithProviders_ForeignProviderId_NotStored() + { + var item = new Movie { Name = "Test Movie" }; + + var provider = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + provider.Setup(p => p.Name).Returns("Provider"); + provider.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .ReturnsAsync(() => + { + var found = new Movie { Name = "Test Movie" }; + found.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123"; + found.ProviderIds[MetadataProvider.Imdb.ToString()] = "tt0113375"; + return new MetadataResult<Movie> { HasMetadata = true, Item = found }; + }); + + var service = new TestMetadataService(); + await service.RefreshWithProvidersInternal( + new MetadataResult<Movie> { Item = item }, + new MovieInfo { Name = item.Name }, + new MetadataRefreshOptions(Mock.Of<IDirectoryService>()) + { + MetadataRefreshMode = MetadataRefreshMode.FullRefresh, + ReplaceAllMetadata = true + }, + [provider.Object]).ConfigureAwait(true); + + Assert.False(item.HasProviderId(MetadataProvider.Tmdb)); + Assert.Equal("tt0113375", item.GetProviderId(MetadataProvider.Imdb)); + } + + [Fact] + public async Task RefreshWithProviders_ForeignProviderId_ReplacedInLookupInfo() + { + var item = new Movie { Name = "Test Movie" }; + var lookupInfo = new MovieInfo { Name = item.Name }; + lookupInfo.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123"; + + var answering = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + answering.Setup(p => p.Name).Returns("Answering"); + answering.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .ReturnsAsync(() => + { + var found = new Movie { Name = "Test Movie" }; + found.ProviderIds[MetadataProvider.Tmdb.ToString()] = "12345"; + return new MetadataResult<Movie> { HasMetadata = true, Item = found }; + }); + + string? tmdbIdSeenBySecondProvider = null; + var following = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + following.Setup(p => p.Name).Returns("Following"); + following.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .ReturnsAsync((MovieInfo info, CancellationToken _) => + { + tmdbIdSeenBySecondProvider = info.GetProviderId(MetadataProvider.Tmdb); + return new MetadataResult<Movie> { HasMetadata = false }; + }); + + var service = new TestMetadataService(); + await service.RefreshWithProvidersInternal( + new MetadataResult<Movie> { Item = item }, + lookupInfo, + new MetadataRefreshOptions(Mock.Of<IDirectoryService>()) + { + MetadataRefreshMode = MetadataRefreshMode.FullRefresh, + ReplaceAllMetadata = true + }, + [answering.Object, following.Object]).ConfigureAwait(true); + + // The stored id cannot be a TMDb one, so the provider that still has to run must get the id + // that was just found instead of failing on the same bad one. + Assert.Equal("12345", tmdbIdSeenBySecondProvider); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task RefreshWithProviders_ForeignPersonProviderId_NotStored(bool replaceAllMetadata) + { + var item = new Movie { Name = "Test Movie" }; + var existing = new MetadataResult<Movie> { Item = item }; + existing.AddPerson(new PersonInfo { Name = "Some Actor", Type = PersonKind.Actor }); + + var provider = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose); + provider.Setup(p => p.Name).Returns("Provider"); + provider.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>())) + .ReturnsAsync(() => + { + var person = new PersonInfo { Name = "Some Actor", Type = PersonKind.Actor }; + person.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123"; + person.ProviderIds[MetadataProvider.Imdb.ToString()] = "nm0000123"; + + var found = new MetadataResult<Movie> { HasMetadata = true, Item = new Movie { Name = "Test Movie" } }; + found.AddPerson(person); + return found; + }); + + var service = new TestMetadataService(); + await service.RefreshWithProvidersInternal( + existing, + new MovieInfo { Name = item.Name }, + new MetadataRefreshOptions(Mock.Of<IDirectoryService>()) + { + MetadataRefreshMode = MetadataRefreshMode.FullRefresh, + ReplaceAllMetadata = replaceAllMetadata + }, + [provider.Object]).ConfigureAwait(true); + + var mergedPerson = Assert.Single(existing.People); + Assert.False(mergedPerson.HasProviderId(MetadataProvider.Tmdb)); + Assert.Equal("nm0000123", mergedPerson.GetProviderId(MetadataProvider.Imdb)); + } + + private sealed class TestMetadataService : MetadataService<Movie, MovieInfo> + { + public TestMetadataService() + : base( + Mock.Of<IServerConfigurationManager>(), + NullLogger<MetadataService<Movie, MovieInfo>>.Instance, + Mock.Of<IProviderManager>(), + Mock.Of<IFileSystem>(), + Mock.Of<ILibraryManager>(), + Mock.Of<IExternalDataManager>(), + Mock.Of<IItemRepository>()) + { + } + + public Task<RefreshResult> RefreshWithProvidersInternal( + MetadataResult<Movie> metadata, + MovieInfo id, + MetadataRefreshOptions options, + ICollection<IMetadataProvider> providers) + => RefreshWithProviders(metadata, id, options, providers, ImageProvider, false, CancellationToken.None); + } + } +} diff --git a/tests/Jellyfin.Providers.Tests/Music/AlbumInfoExtensionsTests.cs b/tests/Jellyfin.Providers.Tests/Music/AlbumInfoExtensionsTests.cs new file mode 100644 index 0000000000..c5ec0de02c --- /dev/null +++ b/tests/Jellyfin.Providers.Tests/Music/AlbumInfoExtensionsTests.cs @@ -0,0 +1,59 @@ +using MediaBrowser.Controller.Providers; +using MediaBrowser.Model.Entities; +using MediaBrowser.Providers.Music; +using Xunit; + +namespace Jellyfin.Providers.Tests.Music; + +public static class AlbumInfoExtensionsTests +{ + private const string ExampleMbid = "59b5a40b-e2fd-3f18-a218-e8c9aae12ab5"; + private const string SongMbid = "6c301dbd-6ccb-3403-a6c4-6a22240a0297"; + + [Theory] + [InlineData(ExampleMbid, ExampleMbid)] + // Another provider's id under a MusicBrainz key reads as no id, so the caller searches instead of + // handing a value the MusicBrainz client throws on. + [InlineData("111239", null)] + [InlineData("", null)] + public static void GetReleaseId_OnlyReturnsMbids(string id, string? expected) + { + var info = new AlbumInfo(); + info.ProviderIds[MetadataProvider.MusicBrainzAlbum.ToString()] = id; + + Assert.Equal(expected, info.GetReleaseId()); + } + + [Fact] + public static void GetReleaseId_ForeignId_FallsBackToSongs() + { + var song = new SongInfo(); + song.ProviderIds[MetadataProvider.MusicBrainzAlbum.ToString()] = SongMbid; + + var info = new AlbumInfo { SongInfos = [song] }; + info.ProviderIds[MetadataProvider.MusicBrainzAlbum.ToString()] = "111239"; + + Assert.Equal(SongMbid, info.GetReleaseId()); + } + + [Fact] + public static void GetMusicBrainzArtistId_ForeignId_FallsBackToArtistIds() + { + var info = new AlbumInfo(); + info.ProviderIds[MetadataProvider.MusicBrainzAlbumArtist.ToString()] = "111239"; + info.ArtistProviderIds[MetadataProvider.MusicBrainzArtist.ToString()] = ExampleMbid; + + Assert.Equal(ExampleMbid, info.GetMusicBrainzArtistId()); + } + + [Theory] + [InlineData(ExampleMbid, ExampleMbid)] + [InlineData("111239", null)] + public static void GetMusicBrainzArtistId_ArtistInfo_OnlyReturnsMbids(string id, string? expected) + { + var info = new ArtistInfo(); + info.ProviderIds[MetadataProvider.MusicBrainzArtist.ToString()] = id; + + Assert.Equal(expected, info.GetMusicBrainzArtistId()); + } +} diff --git a/tests/Jellyfin.Providers.Tests/Tmdb/TmdbUtilsTests.cs b/tests/Jellyfin.Providers.Tests/Tmdb/TmdbUtilsTests.cs index fb0a08c29c..4c4dd5e92f 100644 --- a/tests/Jellyfin.Providers.Tests/Tmdb/TmdbUtilsTests.cs +++ b/tests/Jellyfin.Providers.Tests/Tmdb/TmdbUtilsTests.cs @@ -1,3 +1,5 @@ +using MediaBrowser.Controller.Entities.Movies; +using MediaBrowser.Model.Entities; using MediaBrowser.Providers.Plugins.Tmdb; using Xunit; @@ -34,5 +36,40 @@ namespace Jellyfin.Providers.Tests.Tmdb { Assert.Equal(expected, TmdbUtils.AdjustImageLanguage(imageLanguage, requestLanguage)); } + + [Theory] + [InlineData("11", true, 11)] + // An id another provider filed under the TMDb key must not throw, it is simply not a TMDb id. + [InlineData("nm0000123", false, 0)] + [InlineData("tt0113375", false, 0)] + [InlineData("11.0", false, 0)] + [InlineData("-11", false, 0)] + [InlineData("0", false, 0)] + [InlineData("", false, 0)] + [InlineData(null, false, 0)] + public static void TryParseTmdbId_OnlyAcceptsTmdbIds(string? value, bool expected, int expectedId) + { + Assert.Equal(expected, TmdbUtils.TryParseTmdbId(value, out var tmdbId)); + Assert.Equal(expectedId, tmdbId); + } + + [Theory] + [InlineData("11", true, 11)] + [InlineData("nm0000123", false, 0)] + public static void TryGetTmdbId_OnlyAcceptsTmdbIds(string value, bool expected, int expectedId) + { + var item = new Movie(); + item.ProviderIds[MetadataProvider.Tmdb.ToString()] = value; + + Assert.Equal(expected, item.TryGetTmdbId(out var tmdbId)); + Assert.Equal(expectedId, tmdbId); + } + + [Fact] + public static void TryGetTmdbId_NoId_False() + { + Assert.False(new Movie().TryGetTmdbId(out var tmdbId)); + Assert.Equal(0, tmdbId); + } } } |