Merge pull request #21468 from Piggey/fix-exported-replay-overwrite

Fix `LegacyExporter` classes overwriting existing files

osu.Game.Tests/Utils/NamingUtilsTest.cs

@@ -11,7 +11,7 @@ namespace osu.Game.Tests.Utils
     public class NamingUtilsTest
     {
         [Test]
-        public void TestEmptySet()
+        public void TestNextBestNameEmptySet()
         {
             string nextBestName = NamingUtils.GetNextBestName(Enumerable.Empty<string>(), "New Difficulty");
 
@@ -19,7 +19,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestNotTaken()
+        public void TestNextBestNameNotTaken()
         {
             string[] existingNames =
             {
@@ -34,7 +34,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestNotTakenButClose()
+        public void TestNextBestNameNotTakenButClose()
         {
             string[] existingNames =
             {
@@ -49,7 +49,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestAlreadyTaken()
+        public void TestNextBestNameAlreadyTaken()
         {
             string[] existingNames =
             {
@@ -62,7 +62,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestAlreadyTakenWithDifferentCase()
+        public void TestNextBestNameAlreadyTakenWithDifferentCase()
         {
             string[] existingNames =
             {
@@ -75,7 +75,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestAlreadyTakenWithBrackets()
+        public void TestNextBestNameAlreadyTakenWithBrackets()
         {
             string[] existingNames =
             {
@@ -88,7 +88,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestMultipleAlreadyTaken()
+        public void TestNextBestNameMultipleAlreadyTaken()
         {
             string[] existingNames =
             {
@@ -104,7 +104,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestEvenMoreAlreadyTaken()
+        public void TestNextBestNameEvenMoreAlreadyTaken()
         {
             string[] existingNames = Enumerable.Range(1, 30).Select(i => $"New Difficulty ({i})").Append("New Difficulty").ToArray();
 
@@ -114,7 +114,7 @@ namespace osu.Game.Tests.Utils
         }
 
         [Test]
-        public void TestMultipleAlreadyTakenWithGaps()
+        public void TestNextBestNameMultipleAlreadyTakenWithGaps()
         {
             string[] existingNames =
             {
@@ -128,5 +128,153 @@ namespace osu.Game.Tests.Utils
 
             Assert.AreEqual("New Difficulty (2)", nextBestName);
         }
+
+        [Test]
+        public void TestNextBestFilenameEmptySet()
+        {
+            string nextBestFilename = NamingUtils.GetNextBestFilename(Enumerable.Empty<string>(), "test_file.osr");
+
+            Assert.AreEqual("test_file.osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameNotTaken()
+        {
+            string[] existingFiles =
+            {
+                "this file exists.zip",
+                "that file exists.too",
+                "three.4",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "test_file.osr");
+
+            Assert.AreEqual("test_file.osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameNotTakenButClose()
+        {
+            string[] existingFiles =
+            {
+                "replay_file(1).osr",
+                "replay_file (not a number).zip",
+                "replay_file (1 <- now THAT is a number right here).lol",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+
+            Assert.AreEqual("replay_file.osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameAlreadyTaken()
+        {
+            string[] existingFiles =
+            {
+                "replay_file.osr",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+
+            Assert.AreEqual("replay_file (1).osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameAlreadyTakenDifferentCase()
+        {
+            string[] existingFiles =
+            {
+                "replay_file.osr",
+                "RePlAy_FiLe (1).OsR",
+                "REPLAY_FILE (2).OSR",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+            Assert.AreEqual("replay_file (3).osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameAlreadyTakenWithBrackets()
+        {
+            string[] existingFiles =
+            {
+                "replay_file.osr",
+                "replay_file (copy).osr",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+            Assert.AreEqual("replay_file (1).osr", nextBestFilename);
+
+            nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file (copy).osr");
+            Assert.AreEqual("replay_file (copy) (1).osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameMultipleAlreadyTaken()
+        {
+            string[] existingFiles =
+            {
+                "replay_file.osr",
+                "replay_file (1).osr",
+                "replay_file (2).osr",
+                "replay_file (3).osr",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+
+            Assert.AreEqual("replay_file (4).osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameMultipleAlreadyTakenWithGaps()
+        {
+            string[] existingFiles =
+            {
+                "replay_file.osr",
+                "replay_file (1).osr",
+                "replay_file (2).osr",
+                "replay_file (4).osr",
+                "replay_file (5).osr",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+
+            Assert.AreEqual("replay_file (3).osr", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameNoExtensions()
+        {
+            string[] existingFiles =
+            {
+                "those",
+                "are definitely",
+                "files",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "surely");
+            Assert.AreEqual("surely", nextBestFilename);
+
+            nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "those");
+            Assert.AreEqual("those (1)", nextBestFilename);
+        }
+
+        [Test]
+        public void TestNextBestFilenameDifferentExtensions()
+        {
+            string[] existingFiles =
+            {
+                "replay_file.osr",
+                "replay_file (1).osr",
+                "replay_file.txt",
+            };
+
+            string nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.osr");
+            Assert.AreEqual("replay_file (2).osr", nextBestFilename);
+
+            nextBestFilename = NamingUtils.GetNextBestFilename(existingFiles, "replay_file.txt");
+            Assert.AreEqual("replay_file (1).txt", nextBestFilename);
+        }
     }
 }

osu.Game/Database/LegacyExporter.cs

@@ -3,9 +3,11 @@
 
 #nullable disable
 
+using System.Collections.Generic;
 using System.IO;
 using osu.Framework.Platform;
 using osu.Game.Extensions;
+using osu.Game.Utils;
 using SharpCompress.Archives.Zip;
 
 namespace osu.Game.Database
@@ -37,8 +39,11 @@ namespace osu.Game.Database
         /// <param name="item">The item to export.</param>
         public void Export(TModel item)
         {
-            string filename = $"{item.GetDisplayString().GetValidFilename()}{FileExtension}";
+            string itemFilename = item.GetDisplayString().GetValidFilename();
 
+            IEnumerable<string> existingExports = exportStorage.GetFiles("", $"{itemFilename}*{FileExtension}");
+
+            string filename = NamingUtils.GetNextBestFilename(existingExports, $"{itemFilename}{FileExtension}");
             using (var stream = exportStorage.CreateFileSafely(filename))
                 ExportModelTo(item, stream);
 

osu.Game/Utils/NamingUtils.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System.Collections.Generic;
+using System.IO;
 using System.Text.RegularExpressions;
 
 namespace osu.Game.Utils
@@ -28,8 +29,53 @@ namespace osu.Game.Utils
         /// </remarks>
         public static string GetNextBestName(IEnumerable<string> existingNames, string desiredName)
         {
-            string pattern = $@"^(?i){Regex.Escape(desiredName)}(?-i)( \((?<copyNumber>[1-9][0-9]*)\))?$";
+            string pattern = $@"^{getBaselineNameDetectingPattern(desiredName)}$";
             var regex = new Regex(pattern, RegexOptions.Compiled);
+
+            int bestNumber = findBestNumber(existingNames, regex);
+
+            return bestNumber == 0
+                ? desiredName
+                : $"{desiredName} ({bestNumber.ToString()})";
+        }
+
+        /// <summary>
+        /// Given a set of <paramref name="existingFilenames"/> and a desired target <paramref name="desiredFilename"/>
+        /// finds a filename closest to <paramref name="desiredFilename"/> that is not in <paramref name="existingFilenames"/>
+        /// </summary>
+        public static string GetNextBestFilename(IEnumerable<string> existingFilenames, string desiredFilename)
+        {
+            string name = Path.GetFileNameWithoutExtension(desiredFilename);
+            string extension = Path.GetExtension(desiredFilename);
+
+            string pattern = $@"^{getBaselineNameDetectingPattern(name)}(?i){Regex.Escape(extension)}(?-i)$";
+            var regex = new Regex(pattern, RegexOptions.Compiled);
+
+            int bestNumber = findBestNumber(existingFilenames, regex);
+
+            return bestNumber == 0
+                ? desiredFilename
+                : $"{name} ({bestNumber.ToString()}){extension}";
+        }
+
+        /// <summary>
+        /// Generates a basic regex pattern that will match all possible conflicting filenames when picking the best available name, given the <paramref name="desiredName"/>.
+        /// The generated pattern can be composed into more complicated regexes for particular uses, such as picking filenames, which need additional file extension handling.
+        /// </summary>
+        /// <remarks>
+        /// The regex shall detect:
+        /// <list type="bullet">
+        /// <item>all strings that are equal to <paramref name="desiredName"/>,</item>
+        /// <item>all strings of the format <c>desiredName (number)</c>, where <c>number</c> is a number written using Arabic numerals.</item>
+        /// </list>
+        /// All comparisons are made in a case-insensitive manner.
+        /// If a number is detected in the matches, it will be output to the <c>copyNumber</c> named group.
+        /// </remarks>
+        private static string getBaselineNameDetectingPattern(string desiredName)
+            => $@"(?i){Regex.Escape(desiredName)}(?-i)( \((?<copyNumber>[1-9][0-9]*)\))?";
+
+        private static int findBestNumber(IEnumerable<string> existingNames, Regex regex)
+        {
             var takenNumbers = new HashSet<int>();
 
             foreach (string name in existingNames)
@@ -53,9 +99,7 @@ namespace osu.Game.Utils
             while (takenNumbers.Contains(bestNumber))
                 bestNumber += 1;
 
-            return bestNumber == 0
+            return bestNumber;
-                ? desiredName
-                : $"{desiredName} ({bestNumber})";
         }
     }
 }