Merge pull request #17710 from Shadowghost/fix-omdb-people

Fix OMDB People handling

2 files changed, 161 insertions, 23 deletions

diff --git a/MediaBrowser.Providers/Plugins/Omdb/OmdbProvider.cs b/MediaBrowser.Providers/Plugins/Omdb/OmdbProvider.cs
index f562d64ddd..d51d913caa 100644
--- a/MediaBrowser.Providers/Plugins/Omdb/OmdbProvider.cs
+++ b/MediaBrowser.Providers/Plugins/Omdb/OmdbProvider.cs
@@ -27,6 +27,9 @@ namespace MediaBrowser.Providers.Plugins.Omdb
     /// <summary>Provider for OMDB service.</summary>
     public class OmdbProvider
     {
+        /// <summary>Generational suffixes that OMDb separates from the name with a comma.</summary>
+        private static readonly string[] NameSuffixes = ["Jr", "Jnr", "Sr", "Snr", "II", "III", "IV", "V"];
+
         private readonly IFileSystem _fileSystem;
         private readonly IServerConfigurationManager _configurationManager;
         private readonly IHttpClientFactory _httpClientFactory;
@@ -420,42 +423,96 @@ namespace MediaBrowser.Providers.Plugins.Omdb
                 return;
             }
 
-            if (!string.IsNullOrWhiteSpace(result.Director))
-            {
-                var person = new PersonInfo
-                {
-                    Name = result.Director.Trim(),
-                    Type = PersonKind.Director
-                };
+            AddPeople(itemResult, result.Director, PersonKind.Director);
+            AddPeople(itemResult, result.Writer, PersonKind.Writer);
+            AddPeople(itemResult, result.Actors, PersonKind.Actor);
+        }
 
-                itemResult.AddPerson(person);
+        /// <summary>Adds the people from a comma separated OMDb credit list.</summary>
+        /// <typeparam name="T">The item type.</typeparam>
+        /// <param name="itemResult">The metadata result to add the people to.</param>
+        /// <param name="credits">The comma separated OMDb credit list.</param>
+        /// <param name="type">The kind of person each credit describes.</param>
+        internal static void AddPeople<T>(MetadataResult<T> itemResult, string credits, PersonKind type)
+            where T : BaseItem
+        {
+            if (string.IsNullOrWhiteSpace(credits))
+            {
+                return;
             }
 
-            if (!string.IsNullOrWhiteSpace(result.Writer))
+            var names = new List<string>();
+
+            foreach (var credit in SplitCredits(credits))
             {
-                var person = new PersonInfo
+                // OMDb annotates the credited role in parentheses, e.g. "Mari Okada (screenplay)". The same
+                // person can be credited more than once this way, so strip it and let AddPerson deduplicate.
+                var name = credit;
+                var annotation = name.IndexOf('(', StringComparison.Ordinal);
+                if (annotation >= 0)
+                {
+                    name = name[..annotation];
+                }
+
+                name = name.Trim();
+                if (name.Length == 0)
                 {
-                    Name = result.Writer.Trim(),
-                    Type = PersonKind.Writer
-                };
+                    continue;
+                }
+
+                // A generational suffix is separated from the name it belongs to by the same comma the list
+                // uses, e.g. "Jack Salvatore, Jr.", so it has to be joined back instead of becoming a credit.
+                if (names.Count > 0 && IsNameSuffix(name))
+                {
+                    names[^1] = names[^1] + ", " + name;
+                    continue;
+                }
 
-                itemResult.AddPerson(person);
+                names.Add(name);
             }
 
-            if (!string.IsNullOrWhiteSpace(result.Actors))
+            foreach (var name in names)
             {
-                var actorList = result.Actors.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
-                foreach (var actor in actorList)
+                itemResult.AddPerson(new PersonInfo
                 {
-                    var person = new PersonInfo
-                    {
-                        Name = actor,
-                        Type = PersonKind.Actor
-                    };
+                    Name = name,
+                    Type = type
+                });
+            }
+        }
+
+        // Only the commas between credits, never one inside an annotation: "Jerry Siegel (created by:
+        // Superman, Superboy)" is one credit, and splitting it blindly invents a person called "Superboy)".
+        private static IEnumerable<string> SplitCredits(string credits)
+        {
+            var depth = 0;
+            var start = 0;
 
-                    itemResult.AddPerson(person);
+            for (var i = 0; i < credits.Length; i++)
+            {
+                switch (credits[i])
+                {
+                    case '(':
+                        depth++;
+                        break;
+                    case ')':
+                        depth = Math.Max(0, depth - 1);
+                        break;
+                    case ',' when depth == 0:
+                        yield return credits[start..i];
+                        start = i + 1;
+                        break;
                 }
             }
+
+            yield return credits[start..];
+        }
+
+        private static bool IsNameSuffix(string value)
+        {
+            var suffix = value.EndsWith('.') ? value[..^1] : value;
+
+            return NameSuffixes.Contains(suffix, StringComparer.OrdinalIgnoreCase);
         }
 
         private static bool IsConfiguredForEnglish(BaseItem item, string language)
diff --git a/tests/Jellyfin.Providers.Tests/Omdb/OmdbProviderTests.cs b/tests/Jellyfin.Providers.Tests/Omdb/OmdbProviderTests.cs
new file mode 100644
index 0000000000..bd50a903f1
--- /dev/null
+++ b/tests/Jellyfin.Providers.Tests/Omdb/OmdbProviderTests.cs
@@ -0,0 +1,81 @@
+using System.Linq;
+using Jellyfin.Data.Enums;
+using MediaBrowser.Controller.Entities.Movies;
+using MediaBrowser.Controller.Providers;
+using MediaBrowser.Providers.Plugins.Omdb;
+using Xunit;
+
+namespace Jellyfin.Providers.Tests.Omdb
+{
+    public class OmdbProviderTests
+    {
+        [Fact]
+        public void AddPeople_CommaSeparatedList_SplitsIntoIndividualPeople()
+        {
+            var result = new MetadataResult<Movie>();
+
+            OmdbProvider.AddPeople(result, "Philip G. Epstein, Julius J. Epstein, Howard Koch", PersonKind.Writer);
+
+            Assert.Equal(
+                new[] { "Philip G. Epstein", "Julius J. Epstein", "Howard Koch" },
+                result.People!.Select(p => p.Name));
+            Assert.All(result.People!, p => Assert.Equal(PersonKind.Writer, p.Type));
+        }
+
+        [Fact]
+        public void AddPeople_RoleAnnotations_AreStrippedAndDeduplicated()
+        {
+            var result = new MetadataResult<Movie>();
+
+            OmdbProvider.AddPeople(result, "Mari Okada (screenplay), Mari Okada (story), Jun'ichi Satô (screenplay), Jun'ichi Satô (story)", PersonKind.Writer);
+
+            Assert.Equal(
+                new[] { "Mari Okada", "Jun'ichi Satô" },
+                result.People!.Select(p => p.Name));
+        }
+
+        [Theory]
+        [InlineData(
+            "Jerry Siegel (created by: Superman, Superboy), Bob Kane (created by: Batman)",
+            "Jerry Siegel|Bob Kane")]
+        [InlineData("Alan Moore (created by: John Constantine)", "Alan Moore")]
+        public void AddPeople_CommaInsideAnAnnotation_StaysOneCredit(string credits, string expected)
+        {
+            var result = new MetadataResult<Movie>();
+
+            OmdbProvider.AddPeople(result, credits, PersonKind.Writer);
+
+            Assert.Equal(expected.Split('|'), result.People!.Select(p => p.Name));
+        }
+
+        [Theory]
+        [InlineData("Jack Salvatore, Jr.", "Jack Salvatore, Jr.")]
+        [InlineData("Efrem Zimbalist, Jr., Tom Hanks", "Efrem Zimbalist, Jr.|Tom Hanks")]
+        [InlineData("Tom Hanks, Sammy Davis, Jr", "Tom Hanks|Sammy Davis, Jr")]
+        [InlineData("Harold Ramis, Ken Griffey, III (voice)", "Harold Ramis|Ken Griffey, III")]
+        [InlineData("Robert Downey Jr., Gwyneth Paltrow", "Robert Downey Jr.|Gwyneth Paltrow")]
+        [InlineData("Jr., Tom Hanks", "Jr.|Tom Hanks")]
+        public void AddPeople_GenerationalSuffix_StaysWithItsName(string credits, string expected)
+        {
+            var result = new MetadataResult<Movie>();
+
+            OmdbProvider.AddPeople(result, credits, PersonKind.Actor);
+
+            Assert.Equal(expected.Split('|'), result.People!.Select(p => p.Name));
+        }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("")]
+        [InlineData("   ")]
+        [InlineData("(uncredited)")]
+        public void AddPeople_NoUsableName_AddsNothing(string? credits)
+        {
+            var result = new MetadataResult<Movie>();
+
+            OmdbProvider.AddPeople(result, credits!, PersonKind.Actor);
+
+            Assert.Null(result.People);
+        }
+    }
+}