From 86588778b170b363b2e7156a9ffa08ba32e15f67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 10 Sep 2019 22:06:10 +0200 Subject: [PATCH] Implement fallback decoder registration After the preparatory introduction of LineBufferedReader, it is now possible to introduce registration of fallback decoders that won't drop input supplied in the first line of the file. A fallback decoder is used when the magic in the first line of the file does not match any of the other known decoders. In such a case, the fallback decoder is constructed and provided a LineBufferedReader instance. The process of matching magic only peeks the first non-empty line, so it is available for re-reading in Decode() using ReadLine(). There can be only one fallback decoder per type; a second attempt of registering a fallback will result in an exception to avoid bugs. To address the issue of parsing failing on badly or non-headered files, set the legacy decoders for Beatmaps and Storyboards as the fallbacks. Due to non-trivial logic, several new, passing unit tests with possible edge cases also included. --- .../Formats/LegacyBeatmapDecoderTest.cs | 101 ++++++++++++++++++ .../Beatmaps/IO/ImportBeatmapTest.cs | 2 +- osu.Game.Tests/Resources/corrupted-header.osu | 5 + .../empty-line-instead-of-header.osu | 5 + .../Resources/empty-lines-at-start.osu | 8 ++ osu.Game.Tests/Resources/missing-header.osu | 4 + .../Resources/no-empty-line-after-header.osu | 4 + osu.Game.Tests/osu.Game.Tests.csproj | 7 ++ osu.Game/Beatmaps/Formats/Decoder.cs | 41 +++++-- .../Beatmaps/Formats/LegacyBeatmapDecoder.cs | 1 + .../Formats/LegacyStoryboardDecoder.cs | 1 + osu.Game/IO/LineBufferedReader.cs | 2 + 12 files changed, 172 insertions(+), 9 deletions(-) create mode 100644 osu.Game.Tests/Resources/corrupted-header.osu create mode 100644 osu.Game.Tests/Resources/empty-line-instead-of-header.osu create mode 100644 osu.Game.Tests/Resources/empty-lines-at-start.osu create mode 100644 osu.Game.Tests/Resources/missing-header.osu create mode 100644 osu.Game.Tests/Resources/no-empty-line-after-header.osu diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs index 6a77c6ca96..f6c0dbbecf 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.IO; using NUnit.Framework; using osuTK; using osuTK.Graphics; @@ -490,5 +491,105 @@ namespace osu.Game.Tests.Beatmaps.Formats Assert.DoesNotThrow(() => decoder.Decode(badStream)); } } + + [Test] + public void TestFallbackDecoderForCorruptedHeader() + { + Decoder decoder = null; + Beatmap beatmap = null; + + using (var resStream = TestResources.OpenResource("corrupted-header.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + Assert.DoesNotThrow(() => decoder = Decoder.GetDecoder(stream)); + Assert.IsInstanceOf(decoder); + Assert.DoesNotThrow(() => beatmap = decoder.Decode(stream)); + Assert.IsNotNull(beatmap); + Assert.AreEqual("Beatmap with corrupted header", beatmap.Metadata.Title); + Assert.AreEqual("Evil Hacker", beatmap.Metadata.AuthorString); + } + } + + [Test] + public void TestFallbackDecoderForMissingHeader() + { + Decoder decoder = null; + Beatmap beatmap = null; + + using (var resStream = TestResources.OpenResource("missing-header.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + Assert.DoesNotThrow(() => decoder = Decoder.GetDecoder(stream)); + Assert.IsInstanceOf(decoder); + Assert.DoesNotThrow(() => beatmap = decoder.Decode(stream)); + Assert.IsNotNull(beatmap); + Assert.AreEqual("Beatmap with no header", beatmap.Metadata.Title); + Assert.AreEqual("Incredibly Evil Hacker", beatmap.Metadata.AuthorString); + } + } + + [Test] + public void TestDecodeFileWithEmptyLinesAtStart() + { + Decoder decoder = null; + Beatmap beatmap = null; + + using (var resStream = TestResources.OpenResource("empty-lines-at-start.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + Assert.DoesNotThrow(() => decoder = Decoder.GetDecoder(stream)); + Assert.IsInstanceOf(decoder); + Assert.DoesNotThrow(() => beatmap = decoder.Decode(stream)); + Assert.IsNotNull(beatmap); + Assert.AreEqual("Empty lines at start", beatmap.Metadata.Title); + Assert.AreEqual("Edge Case Hunter", beatmap.Metadata.AuthorString); + } + } + + [Test] + public void TestDecodeFileWithEmptyLinesAndNoHeader() + { + Decoder decoder = null; + Beatmap beatmap = null; + + using (var resStream = TestResources.OpenResource("empty-line-instead-of-header.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + Assert.DoesNotThrow(() => decoder = Decoder.GetDecoder(stream)); + Assert.IsInstanceOf(decoder); + Assert.DoesNotThrow(() => beatmap = decoder.Decode(stream)); + Assert.IsNotNull(beatmap); + Assert.AreEqual("The dog ate the file header", beatmap.Metadata.Title); + Assert.AreEqual("Why does this keep happening", beatmap.Metadata.AuthorString); + } + } + + [Test] + public void TestDecodeFileWithContentImmediatelyAfterHeader() + { + Decoder decoder = null; + Beatmap beatmap = null; + + using (var resStream = TestResources.OpenResource("no-empty-line-after-header.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + Assert.DoesNotThrow(() => decoder = Decoder.GetDecoder(stream)); + Assert.IsInstanceOf(decoder); + Assert.DoesNotThrow(() => beatmap = decoder.Decode(stream)); + Assert.IsNotNull(beatmap); + Assert.AreEqual("No empty line delimiting header from contents", beatmap.Metadata.Title); + Assert.AreEqual("Edge Case Hunter", beatmap.Metadata.AuthorString); + } + } + + [Test] + public void TestDecodeEmptyFile() + { + using (var resStream = new MemoryStream()) + using (var stream = new LineBufferedReader(resStream)) + { + Assert.Throws(() => Decoder.GetDecoder(stream)); + } + } } } diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index ad0ed00989..0686073292 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -126,7 +126,7 @@ namespace osu.Game.Tests.Beatmaps.IO var breakTemp = TestResources.GetTestBeatmapForImport(); - MemoryStream brokenOsu = new MemoryStream(new byte[] { 1, 3, 3, 7 }); + MemoryStream brokenOsu = new MemoryStream(); MemoryStream brokenOsz = new MemoryStream(File.ReadAllBytes(breakTemp)); File.Delete(breakTemp); diff --git a/osu.Game.Tests/Resources/corrupted-header.osu b/osu.Game.Tests/Resources/corrupted-header.osu new file mode 100644 index 0000000000..92701a4a7d --- /dev/null +++ b/osu.Game.Tests/Resources/corrupted-header.osu @@ -0,0 +1,5 @@ +ow computerosu file format v14 + +[Metadata] +Title: Beatmap with corrupted header +Creator: Evil Hacker diff --git a/osu.Game.Tests/Resources/empty-line-instead-of-header.osu b/osu.Game.Tests/Resources/empty-line-instead-of-header.osu new file mode 100644 index 0000000000..91ecf8d84a --- /dev/null +++ b/osu.Game.Tests/Resources/empty-line-instead-of-header.osu @@ -0,0 +1,5 @@ + + +[Metadata] +Title: The dog ate the file header +Creator: Why does this keep happening \ No newline at end of file diff --git a/osu.Game.Tests/Resources/empty-lines-at-start.osu b/osu.Game.Tests/Resources/empty-lines-at-start.osu new file mode 100644 index 0000000000..cb3b1761a2 --- /dev/null +++ b/osu.Game.Tests/Resources/empty-lines-at-start.osu @@ -0,0 +1,8 @@ + + + +osu file format v14 + +[Metadata] +Title: Empty lines at start +Creator: Edge Case Hunter \ No newline at end of file diff --git a/osu.Game.Tests/Resources/missing-header.osu b/osu.Game.Tests/Resources/missing-header.osu new file mode 100644 index 0000000000..95fac0d79b --- /dev/null +++ b/osu.Game.Tests/Resources/missing-header.osu @@ -0,0 +1,4 @@ +[Metadata] + +Title: Beatmap with no header +Creator: Incredibly Evil Hacker diff --git a/osu.Game.Tests/Resources/no-empty-line-after-header.osu b/osu.Game.Tests/Resources/no-empty-line-after-header.osu new file mode 100644 index 0000000000..9db2b7c01c --- /dev/null +++ b/osu.Game.Tests/Resources/no-empty-line-after-header.osu @@ -0,0 +1,4 @@ +osu file format v14 +[Metadata] +Title: No empty line delimiting header from contents +Creator: Edge Case Hunter \ No newline at end of file diff --git a/osu.Game.Tests/osu.Game.Tests.csproj b/osu.Game.Tests/osu.Game.Tests.csproj index 84f67c9319..9dc3e85e1b 100644 --- a/osu.Game.Tests/osu.Game.Tests.csproj +++ b/osu.Game.Tests/osu.Game.Tests.csproj @@ -1,5 +1,12 @@  + + + + + + + diff --git a/osu.Game/Beatmaps/Formats/Decoder.cs b/osu.Game/Beatmaps/Formats/Decoder.cs index 0f1b617809..4a539cc33f 100644 --- a/osu.Game/Beatmaps/Formats/Decoder.cs +++ b/osu.Game/Beatmaps/Formats/Decoder.cs @@ -28,6 +28,7 @@ namespace osu.Game.Beatmaps.Formats public abstract class Decoder { private static readonly Dictionary>> decoders = new Dictionary>>(); + private static readonly Dictionary> fallback_decoders = new Dictionary>(); static Decoder() { @@ -49,21 +50,31 @@ namespace osu.Game.Beatmaps.Formats if (!decoders.TryGetValue(typeof(T), out var typedDecoders)) throw new IOException(@"Unknown decoder type"); - string line; + // start off with the first line of the file + string line = stream.PeekLine()?.Trim(); - do + while (line != null && line.Length == 0) { - line = stream.ReadLine()?.Trim(); - } while (line != null && line.Length == 0); + // consume the previously peeked empty line and advance to the next one + stream.ReadLine(); + line = stream.PeekLine()?.Trim(); + } if (line == null) - throw new IOException(@"Unknown file format (null)"); + throw new IOException("Unknown file format (null)"); var decoder = typedDecoders.Select(d => line.StartsWith(d.Key, StringComparison.InvariantCulture) ? d.Value : null).FirstOrDefault(); - if (decoder == null) - throw new IOException($@"Unknown file format ({line})"); - return (Decoder)decoder.Invoke(line); + // it's important the magic does NOT get consumed here, since sometimes it's part of the structure + // (see JsonBeatmapDecoder - the magic string is the opening brace) + // decoder implementations should therefore not die on receiving their own magic + if (decoder != null) + return (Decoder)decoder.Invoke(line); + + if (!fallback_decoders.TryGetValue(typeof(T), out var fallbackDecoder)) + throw new IOException($"Unknown file format ({line})"); + + return (Decoder)fallbackDecoder.Invoke(); } /// @@ -78,5 +89,19 @@ namespace osu.Game.Beatmaps.Formats typedDecoders[magic] = constructor; } + + /// + /// Registers a fallback decoder instantiation function. + /// The fallback will be returned if the first line of the decoded stream does not match any known magic. + /// + /// Type of object being decoded. + /// A function that constructs the fallback. + protected static void SetFallbackDecoder(Func constructor) + { + if (fallback_decoders.ContainsKey(typeof(T))) + throw new InvalidOperationException($"A fallback decoder was already added for type {typeof(T)}."); + + fallback_decoders[typeof(T)] = constructor; + } } } diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs index 6e34fd8e90..786b7611b5 100644 --- a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs @@ -26,6 +26,7 @@ namespace osu.Game.Beatmaps.Formats public static void Register() { AddDecoder(@"osu file format v", m => new LegacyBeatmapDecoder(Parsing.ParseInt(m.Split('v').Last()))); + SetFallbackDecoder(() => new LegacyBeatmapDecoder()); } /// diff --git a/osu.Game/Beatmaps/Formats/LegacyStoryboardDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyStoryboardDecoder.cs index a8f8dbf69d..5dbd67d304 100644 --- a/osu.Game/Beatmaps/Formats/LegacyStoryboardDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyStoryboardDecoder.cs @@ -34,6 +34,7 @@ namespace osu.Game.Beatmaps.Formats // note that this isn't completely correct AddDecoder(@"osu file format v", m => new LegacyStoryboardDecoder()); AddDecoder(@"[Events]", m => new LegacyStoryboardDecoder()); + SetFallbackDecoder(() => new LegacyStoryboardDecoder()); } protected override void ParseStreamInto(LineBufferedReader stream, Storyboard storyboard) diff --git a/osu.Game/IO/LineBufferedReader.cs b/osu.Game/IO/LineBufferedReader.cs index 66dee1181f..aab761afd8 100644 --- a/osu.Game/IO/LineBufferedReader.cs +++ b/osu.Game/IO/LineBufferedReader.cs @@ -25,6 +25,7 @@ namespace osu.Game.IO /// /// Reads the next line from the stream without consuming it. + /// Subsequent calls to without a will return the same string. /// public string PeekLine() { @@ -39,6 +40,7 @@ namespace osu.Game.IO /// /// Reads the next line from the stream and consumes it. + /// If a line was peeked, that same line will then be consumed and returned. /// public string ReadLine() => lineBuffer.Count > 0 ? lineBuffer.Dequeue() : streamReader.ReadLine();