Fix handling of unordered multi-episode NFOs

5 files changed, 208 insertions, 24 deletions

diff --git a/MediaBrowser.Providers/TV/EpisodeMetadataService.cs b/MediaBrowser.Providers/TV/EpisodeMetadataService.cs
index 596ca8d201..4ec6afe45b 100644
--- a/MediaBrowser.Providers/TV/EpisodeMetadataService.cs
+++ b/MediaBrowser.Providers/TV/EpisodeMetadataService.cs
@@ -44,6 +44,33 @@ public class EpisodeMetadataService : MetadataService<Episode, EpisodeInfo>
     {
         var updatedType = base.BeforeSaveInternal(item, isFullRefresh, updateType);
 
+        // An episode cannot end before it starts. Providers and nfo files occasionally report the range
+        // transposed, which makes clients render the episode number backwards. Both numbers describe the
+        // same set of episodes either way, so restore their order instead of dropping the range.
+        if (item.IndexNumber.HasValue && item.IndexNumberEnd < item.IndexNumber)
+        {
+            Logger.LogWarning(
+                "Correcting reversed episode range {IndexNumber}-{IndexNumberEnd} for {Path}",
+                item.IndexNumber,
+                item.IndexNumberEnd,
+                item.Path);
+
+            (item.IndexNumber, item.IndexNumberEnd) = (item.IndexNumberEnd, item.IndexNumber);
+            updatedType |= ItemUpdateType.MetadataImport;
+        }
+        else if (item.IndexNumberEnd.HasValue && !item.IndexNumber.HasValue)
+        {
+            // Without a first episode the end does not describe a range. Promoting it to the episode number
+            // would invent an identity the metadata never supplied, so drop the orphaned value instead.
+            Logger.LogWarning(
+                "Discarding episode range end {IndexNumberEnd} without an episode number for {Path}",
+                item.IndexNumberEnd,
+                item.Path);
+
+            item.IndexNumberEnd = null;
+            updatedType |= ItemUpdateType.MetadataImport;
+        }
+
         var seriesName = item.FindSeriesName();
         if (!string.Equals(item.SeriesName, seriesName, StringComparison.Ordinal))
         {
diff --git a/MediaBrowser.XbmcMetadata/Parsers/EpisodeNfoParser.cs b/MediaBrowser.XbmcMetadata/Parsers/EpisodeNfoParser.cs
index 19b1bbe7b6..c3e5c791c0 100644
--- a/MediaBrowser.XbmcMetadata/Parsers/EpisodeNfoParser.cs
+++ b/MediaBrowser.XbmcMetadata/Parsers/EpisodeNfoParser.cs
@@ -1,5 +1,7 @@
 using System;
+using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using System.Text;
 using System.Threading;
 using System.Xml;
@@ -44,41 +46,60 @@ namespace MediaBrowser.XbmcMetadata.Parsers
 
             var xmlFile = File.ReadAllText(metadataFile);
 
-            var srch = "</episodedetails>";
-            var index = xmlFile.IndexOf(srch, StringComparison.OrdinalIgnoreCase);
-
-            var xml = xmlFile;
+            // Split the nfo into its episodedetails blocks.
+            // This is needed because XBMC metadata uses multiple episodedetails blocks instead of an episodenumberend tag.
+            const string Srch = "</episodedetails>";
+            var blocks = new List<string>();
+            int index;
+            while ((index = xmlFile.IndexOf(Srch, StringComparison.OrdinalIgnoreCase)) != -1)
+            {
+                blocks.Add(xmlFile.Substring(0, index + Srch.Length));
+                xmlFile = xmlFile.Substring(index + Srch.Length);
+            }
 
-            if (index != -1)
+            if (blocks.Count == 0)
             {
-                xml = xmlFile.Substring(0, index + srch.Length);
-                xmlFile = xmlFile.Substring(index + srch.Length);
+                // No closing tag, let the xml reader deal with whatever is in the file
+                blocks.Add(xmlFile);
             }
 
             // These are not going to be valid xml so no sense in causing the provider to fail and spamming the log with exceptions
             try
             {
-                // Extract episode details from the first episodedetails block
-                ReadEpisodeDetailsFromXml(item, xml, settings, cancellationToken);
+                if (blocks.Count == 1)
+                {
+                    ReadEpisodeDetailsFromXml(item, blocks[0], settings, cancellationToken);
+                    return;
+                }
+
+                // The blocks are not guaranteed to be written in ascending episode order, so parse them all
+                // and sort them before merging.
+                var episodes = blocks
+                    .Select(block =>
+                    {
+                        var episode = new MetadataResult<Episode>()
+                        {
+                            Item = new Episode()
+                        };
 
-                // Extract the last episode number from nfo
-                // Retrieves all additional episodedetails blocks from the rest of the nfo and concatenates the name, originalTitle and overview tags with the first episode
-                // This is needed because XBMC metadata uses multiple episodedetails blocks instead of episodenumberend tag
+                        ReadEpisodeDetailsFromXml(episode, block, settings, cancellationToken);
+
+                        return (Xml: block, Result: episode);
+                    })
+                    .OrderBy(episode => episode.Result.Item.IndexNumber ?? int.MaxValue)
+                    .ToList();
+
+                // Extract the details of the lowest numbered episode into the item that is returned to the caller
+                ReadEpisodeDetailsFromXml(item, episodes[0].Xml, settings, cancellationToken);
+
+                // Concatenate the name, originalTitle and overview tags of the remaining episodes with the first one
+                // and take the highest episode number as the last episode of the file
                 var name = new StringBuilder(item.Item.Name);
                 var originalTitle = new StringBuilder(item.Item.OriginalTitle);
                 var overview = new StringBuilder(item.Item.Overview);
-                while ((index = xmlFile.IndexOf(srch, StringComparison.OrdinalIgnoreCase)) != -1)
+                for (var i = 1; i < episodes.Count; i++)
                 {
-                    xml = xmlFile.Substring(0, index + srch.Length);
-                    xmlFile = xmlFile.Substring(index + srch.Length);
-
-                    var additionalEpisode = new MetadataResult<Episode>()
-                    {
-                        Item = new Episode()
-                    };
-
-                    // Extract episode details from additional episodedetails block
-                    ReadEpisodeDetailsFromXml(additionalEpisode, xml, settings, cancellationToken);
+                    var additionalEpisode = episodes[i].Result;
 
                     if (!string.IsNullOrEmpty(additionalEpisode.Item.Name))
                     {
diff --git a/tests/Jellyfin.Providers.Tests/TV/EpisodeMetadataServiceTests.cs b/tests/Jellyfin.Providers.Tests/TV/EpisodeMetadataServiceTests.cs
index 8f5b1b3c48..24fd6a58e9 100644
--- a/tests/Jellyfin.Providers.Tests/TV/EpisodeMetadataServiceTests.cs
+++ b/tests/Jellyfin.Providers.Tests/TV/EpisodeMetadataServiceTests.cs
@@ -1,5 +1,6 @@
 using System;
 using MediaBrowser.Controller.Configuration;
+using MediaBrowser.Controller.Entities;
 using MediaBrowser.Controller.Entities.TV;
 using MediaBrowser.Controller.IO;
 using MediaBrowser.Controller.Library;
@@ -15,9 +16,23 @@ using Xunit;
 
 namespace Jellyfin.Providers.Tests.TV;
 
-public class EpisodeMetadataServiceTests
+// put tests that mock the static LibraryManager in the same collection to avoid test interference
+[Collection("LibraryManagerTests")]
+public sealed class EpisodeMetadataServiceTests : IDisposable
 {
     private readonly TestEpisodeMetadataService _service = new();
+    private readonly ILibraryManager? _previousLibraryManager;
+
+    public EpisodeMetadataServiceTests()
+    {
+        _previousLibraryManager = BaseItem.LibraryManager;
+        BaseItem.LibraryManager = Mock.Of<ILibraryManager>();
+    }
+
+    public void Dispose()
+    {
+        BaseItem.LibraryManager = _previousLibraryManager;
+    }
 
     [Fact]
     public void MergeData_ProviderSeasonOverridesPathDerivedSeason()
@@ -88,6 +103,58 @@ public class EpisodeMetadataServiceTests
         Assert.Equal(1, target.Item.ParentIndexNumber);
     }
 
+    [Theory]
+    [InlineData(2, 1)] // e.g. an nfo with its episodedetails blocks in descending order
+    [InlineData(22, 21)]
+    public void BeforeSave_ReversedEpisodeRange_RestoresOrder(int indexNumber, int indexNumberEnd)
+    {
+        var item = new Episode
+        {
+            IndexNumber = indexNumber,
+            IndexNumberEnd = indexNumberEnd
+        };
+
+        var updateType = _service.BeforeSave(item);
+
+        // The range still covers the same episodes, it is just no longer transposed
+        Assert.Equal(indexNumberEnd, item.IndexNumber);
+        Assert.Equal(indexNumber, item.IndexNumberEnd);
+        Assert.True(updateType.HasFlag(ItemUpdateType.MetadataImport));
+    }
+
+    [Fact]
+    public void BeforeSave_EpisodeRangeWithoutStart_ClearsIndexNumberEnd()
+    {
+        var item = new Episode
+        {
+            IndexNumber = null,
+            IndexNumberEnd = 2
+        };
+
+        var updateType = _service.BeforeSave(item);
+
+        Assert.Null(item.IndexNumberEnd);
+        Assert.Null(item.IndexNumber);
+        Assert.True(updateType.HasFlag(ItemUpdateType.MetadataImport));
+    }
+
+    [Theory]
+    [InlineData(1, 2)] // Regular multi episode file
+    [InlineData(1, 1)] // Degenerate but not contradictory
+    public void BeforeSave_ValidEpisodeRange_KeepsIndexNumberEnd(int indexNumber, int indexNumberEnd)
+    {
+        var item = new Episode
+        {
+            IndexNumber = indexNumber,
+            IndexNumberEnd = indexNumberEnd
+        };
+
+        _service.BeforeSave(item);
+
+        Assert.Equal(indexNumber, item.IndexNumber);
+        Assert.Equal(indexNumberEnd, item.IndexNumberEnd);
+    }
+
     private sealed class TestEpisodeMetadataService : EpisodeMetadataService
     {
         public TestEpisodeMetadataService()
@@ -106,5 +173,10 @@ public class EpisodeMetadataServiceTests
         {
             MergeData(source, target, Array.Empty<MetadataField>(), replaceData, mergeMetadataSettings);
         }
+
+        public ItemUpdateType BeforeSave(Episode item)
+        {
+            return BeforeSaveInternal(item, false, ItemUpdateType.None);
+        }
     }
 }
diff --git a/tests/Jellyfin.XbmcMetadata.Tests/Parsers/EpisodeNfoProviderTests.cs b/tests/Jellyfin.XbmcMetadata.Tests/Parsers/EpisodeNfoProviderTests.cs
index a04b37f215..3767b5c954 100644
--- a/tests/Jellyfin.XbmcMetadata.Tests/Parsers/EpisodeNfoProviderTests.cs
+++ b/tests/Jellyfin.XbmcMetadata.Tests/Parsers/EpisodeNfoProviderTests.cs
@@ -124,6 +124,27 @@ namespace Jellyfin.XbmcMetadata.Tests.Parsers
         }
 
         [Fact]
+        public void Fetch_Valid_MultiEpisode_Unordered_Success()
+        {
+            var result = new MetadataResult<Episode>()
+            {
+                Item = new Episode()
+            };
+
+            _parser.Fetch(result, "Test Data/Rising-Reversed.nfo", CancellationToken.None);
+
+            var item = result.Item;
+            // The episodedetails blocks are stored in descending order, the merged episode must still be in ascending order
+            Assert.Equal("Rising (1) / Rising (2)", item.Name);
+            Assert.Equal(1, item.IndexNumber);
+            Assert.Equal(2, item.IndexNumberEnd);
+            Assert.Equal(1, item.ParentIndexNumber);
+            Assert.Equal("A new Stargate team embarks on a dangerous mission to a distant galaxy, where they discover a mythical lost city -- and a deadly new enemy. / Sheppard tries to convince Weir to mount a rescue mission to free Colonel Sumner, Teyla, and the others captured by the Wraith.", item.Overview);
+            Assert.Equal(new DateTime(2004, 7, 16), item.PremiereDate);
+            Assert.Equal(2004, item.ProductionYear);
+        }
+
+        [Fact]
         public void Fetch_Valid_MultiEpisode_With_Missing_Tags_Success()
         {
             var result = new MetadataResult<Episode>()
diff --git a/tests/Jellyfin.XbmcMetadata.Tests/Test Data/Rising-Reversed.nfo b/tests/Jellyfin.XbmcMetadata.Tests/Test Data/Rising-Reversed.nfo
new file mode 100644
index 0000000000..6dbab13566
--- /dev/null
+++ b/tests/Jellyfin.XbmcMetadata.Tests/Test Data/Rising-Reversed.nfo
@@ -0,0 +1,43 @@
+<episodedetails>
+  <title>Rising (2)</title>
+  <season>1</season>
+  <episode>2</episode>
+  <aired>2004-07-16</aired>
+  <plot>Sheppard tries to convince Weir to mount a rescue mission to free Colonel Sumner, Teyla, and the others captured by the Wraith.</plot>
+  <thumb>https://artworks.thetvdb.com/banners/episodes/70851/25334.jpg</thumb>
+  <watched>false</watched>
+  <rating>7.9</rating>
+  <actor>
+    <name>Joe Flanigan</name>
+    <role>John Sheppard</role>
+    <order>0</order>
+    <thumb>https://image.tmdb.org/t/p/w300_and_h450_bestv2/5AA1ORKIsnMakT6fCVy3JKlzMs6.jpg</thumb>
+  </actor>
+  <actor>
+    <name>David Hewlett</name>
+    <role>Rodney McKay</role>
+    <order>1</order>
+    <thumb>https://image.tmdb.org/t/p/w300_and_h450_bestv2/hUcYyssAPCqnZ4GjolhOWXHTWSa.jpg</thumb>
+  </actor>
+</episodedetails><episodedetails>
+  <title>Rising (1)</title>
+  <season>1</season>
+  <episode>1</episode>
+  <aired>2004-07-16</aired>
+  <plot>A new Stargate team embarks on a dangerous mission to a distant galaxy, where they discover a mythical lost city -- and a deadly new enemy.</plot>
+  <thumb>https://artworks.thetvdb.com/banners/episodes/70851/25333.jpg</thumb>
+  <watched>false</watched>
+  <rating>8.0</rating>
+  <actor>
+    <name>Joe Flanigan</name>
+    <role>John Sheppard</role>
+    <order>0</order>
+    <thumb>https://image.tmdb.org/t/p/w300_and_h450_bestv2/5AA1ORKIsnMakT6fCVy3JKlzMs6.jpg</thumb>
+  </actor>
+  <actor>
+    <name>David Hewlett</name>
+    <role>Rodney McKay</role>
+    <order>1</order>
+    <thumb>https://image.tmdb.org/t/p/w300_and_h450_bestv2/hUcYyssAPCqnZ4GjolhOWXHTWSa.jpg</thumb>
+  </actor>
+</episodedetails>