From 14c95f45848c90457425fc85b302517d82db3968 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 26 Jun 2023 17:28:08 +0900 Subject: [PATCH 1/7] Apply NRT to `FilterCriteria` --- .../SongSelect/TestSceneFilterControl.cs | 2 +- osu.Game/Screens/Select/FilterCriteria.cs | 18 +++++++----------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs index 64e2447cca..f020cd02f7 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs @@ -210,7 +210,7 @@ namespace osu.Game.Tests.Visual.SongSelect InputManager.Click(MouseButton.Left); }); - AddAssert("collection filter still selected", () => control.CreateCriteria().CollectionBeatmapMD5Hashes.Any()); + AddAssert("collection filter still selected", () => control.CreateCriteria().CollectionBeatmapMD5Hashes?.Any() ?? false); AddAssert("filter request not fired", () => !received); } diff --git a/osu.Game/Screens/Select/FilterCriteria.cs b/osu.Game/Screens/Select/FilterCriteria.cs index 320bfb1b45..2be8c71e21 100644 --- a/osu.Game/Screens/Select/FilterCriteria.cs +++ b/osu.Game/Screens/Select/FilterCriteria.cs @@ -1,12 +1,10 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +#nullable enable +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Collections.Generic; using System.Linq; -using JetBrains.Annotations; using osu.Game.Beatmaps; using osu.Game.Collections; using osu.Game.Rulesets; @@ -20,7 +18,7 @@ namespace osu.Game.Screens.Select public GroupMode Group; public SortMode Sort; - public BeatmapSetInfo SelectedBeatmapSet; + public BeatmapSetInfo? SelectedBeatmapSet; public OptionalRange StarDifficulty; public OptionalRange ApproachRate; @@ -42,10 +40,10 @@ namespace osu.Game.Screens.Select public string[] SearchTerms = Array.Empty(); - public RulesetInfo Ruleset; + public RulesetInfo? Ruleset; public bool AllowConvertedBeatmaps; - private string searchText; + private string searchText = string.Empty; /// /// as a number (if it can be parsed as one). @@ -70,11 +68,9 @@ namespace osu.Game.Screens.Select /// /// Hashes from the to filter to. /// - [CanBeNull] - public IEnumerable CollectionBeatmapMD5Hashes { get; set; } + public IEnumerable? CollectionBeatmapMD5Hashes { get; set; } - [CanBeNull] - public IRulesetFilterCriteria RulesetCriteria { get; set; } + public IRulesetFilterCriteria? RulesetCriteria { get; set; } public struct OptionalRange : IEquatable> where T : struct From 1960cd08397ed18c67695391b70e0ce415e92fc4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 26 Jun 2023 18:26:39 +0900 Subject: [PATCH 2/7] Add test coverage of exact matching of search terms --- .../NonVisual/Filtering/FilterMatchingTest.cs | 24 +++++++++++++++++ .../Filtering/FilterQueryParserTest.cs | 27 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs b/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs index 33204d33a7..8bd9407599 100644 --- a/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs +++ b/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs @@ -159,6 +159,30 @@ namespace osu.Game.Tests.NonVisual.Filtering Assert.AreEqual(filtered, carouselItem.Filtered.Value); } + [Test] + [TestCase("\"artist\"", false)] + [TestCase("\"arti\"", true)] + [TestCase("\"artist title author\"", true)] + [TestCase("\"artist\" \"title\" \"author\"", false)] + [TestCase("\"an artist\"", true)] + [TestCase("\"tags too\"", false)] + [TestCase("\"tags to\"", true)] + [TestCase("\"version\"", false)] + [TestCase("\"an auteur\"", true)] + public void TestCriteriaMatchingExactTerms(string terms, bool filtered) + { + var exampleBeatmapInfo = getExampleBeatmap(); + var criteria = new FilterCriteria + { + Ruleset = new RulesetInfo { OnlineID = 6 }, + AllowConvertedBeatmaps = true, + SearchText = terms + }; + var carouselItem = new CarouselBeatmap(exampleBeatmapInfo); + carouselItem.Filter(criteria); + Assert.AreEqual(filtered, carouselItem.Filtered.Value); + } + [Test] [TestCase("", false)] [TestCase("The", false)] diff --git a/osu.Game.Tests/NonVisual/Filtering/FilterQueryParserTest.cs b/osu.Game.Tests/NonVisual/Filtering/FilterQueryParserTest.cs index da32edb8fb..46f91f9a67 100644 --- a/osu.Game.Tests/NonVisual/Filtering/FilterQueryParserTest.cs +++ b/osu.Game.Tests/NonVisual/Filtering/FilterQueryParserTest.cs @@ -23,6 +23,31 @@ namespace osu.Game.Tests.NonVisual.Filtering Assert.AreEqual(4, filterCriteria.SearchTerms.Length); } + [Test] + public void TestApplyQueriesBareWordsWithExactMatch() + { + const string query = "looking for \"a beatmap\" like \"this\""; + var filterCriteria = new FilterCriteria(); + FilterQueryParser.ApplyQueries(filterCriteria, query); + Assert.AreEqual("looking for \"a beatmap\" like \"this\"", filterCriteria.SearchText); + Assert.AreEqual(5, filterCriteria.SearchTerms.Length); + + Assert.That(filterCriteria.SearchTerms[0].SearchTerm, Is.EqualTo("a beatmap")); + Assert.That(filterCriteria.SearchTerms[0].Exact, Is.True); + + Assert.That(filterCriteria.SearchTerms[1].SearchTerm, Is.EqualTo("this")); + Assert.That(filterCriteria.SearchTerms[1].Exact, Is.True); + + Assert.That(filterCriteria.SearchTerms[2].SearchTerm, Is.EqualTo("looking")); + Assert.That(filterCriteria.SearchTerms[2].Exact, Is.False); + + Assert.That(filterCriteria.SearchTerms[3].SearchTerm, Is.EqualTo("for")); + Assert.That(filterCriteria.SearchTerms[3].Exact, Is.False); + + Assert.That(filterCriteria.SearchTerms[4].SearchTerm, Is.EqualTo("like")); + Assert.That(filterCriteria.SearchTerms[4].Exact, Is.False); + } + /* * The following tests have been written a bit strangely (they don't check exact * bound equality with what the filter says). @@ -235,6 +260,7 @@ namespace osu.Game.Tests.NonVisual.Filtering Assert.AreEqual("find me songs by please", filterCriteria.SearchText.Trim()); Assert.AreEqual(5, filterCriteria.SearchTerms.Length); Assert.AreEqual("singer", filterCriteria.Artist.SearchTerm); + Assert.That(filterCriteria.Artist.Exact, Is.False); } [Test] @@ -246,6 +272,7 @@ namespace osu.Game.Tests.NonVisual.Filtering Assert.AreEqual("really like yes", filterCriteria.SearchText.Trim()); Assert.AreEqual(3, filterCriteria.SearchTerms.Length); Assert.AreEqual("name with space", filterCriteria.Artist.SearchTerm); + Assert.That(filterCriteria.Artist.Exact, Is.True); } [Test] From a74547c43cf0328d69ec5671635f2a4a4c05356a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 26 Jun 2023 17:30:08 +0900 Subject: [PATCH 3/7] Add exact match support at song select --- .../Select/Carousel/CarouselBeatmap.cs | 10 ++-- osu.Game/Screens/Select/FilterCriteria.cs | 47 +++++++++++++++++-- osu.Game/Screens/Select/FilterQueryParser.cs | 2 +- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs index 7e48bc5cdd..18931c462f 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs @@ -1,7 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System; using System.Linq; using osu.Game.Beatmaps; using osu.Game.Screens.Select.Filter; @@ -65,16 +64,16 @@ namespace osu.Game.Screens.Select.Carousel if (criteria.SearchTerms.Length > 0) { - var terms = BeatmapInfo.GetSearchableTerms(); + var searchableTerms = BeatmapInfo.GetSearchableTerms(); - foreach (string criteriaTerm in criteria.SearchTerms) + foreach (FilterCriteria.OptionalTextFilter criteriaTerm in criteria.SearchTerms) { bool any = false; // ReSharper disable once ForeachCanBeConvertedToQueryUsingAnotherGetEnumerator - foreach (string term in terms) + foreach (string searchTerm in searchableTerms) { - if (!term.Contains(criteriaTerm, StringComparison.InvariantCultureIgnoreCase)) continue; + if (!criteriaTerm.Matches(searchTerm)) continue; any = true; break; @@ -98,7 +97,6 @@ namespace osu.Game.Screens.Select.Carousel if (!match) return false; match &= criteria.CollectionBeatmapMD5Hashes?.Contains(BeatmapInfo.MD5Hash) ?? true; - if (match && criteria.RulesetCriteria != null) match &= criteria.RulesetCriteria.Matches(BeatmapInfo); diff --git a/osu.Game/Screens/Select/FilterCriteria.cs b/osu.Game/Screens/Select/FilterCriteria.cs index 2be8c71e21..3fcc8d5476 100644 --- a/osu.Game/Screens/Select/FilterCriteria.cs +++ b/osu.Game/Screens/Select/FilterCriteria.cs @@ -1,10 +1,14 @@ -#nullable enable +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +#nullable enable // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. using System; using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; using osu.Game.Beatmaps; using osu.Game.Collections; using osu.Game.Rulesets; @@ -38,7 +42,7 @@ namespace osu.Game.Screens.Select IsUpperInclusive = true }; - public string[] SearchTerms = Array.Empty(); + public OptionalTextFilter[] SearchTerms = Array.Empty(); public RulesetInfo? Ruleset; public bool AllowConvertedBeatmaps; @@ -56,11 +60,29 @@ namespace osu.Game.Screens.Select set { searchText = value; - SearchTerms = searchText.Split(' ', StringSplitOptions.RemoveEmptyEntries).ToArray(); + + List terms = new List(); + + string remainingText = value; + + // First handle quoted segments to ensure we keep inline spaces in exact matches. + foreach (Match quotedSegment in Regex.Matches(searchText, "(\"[^\"]+\")")) + { + terms.Add(new OptionalTextFilter { SearchTerm = quotedSegment.Value }); + remainingText = remainingText.Replace(quotedSegment.Value, string.Empty); + } + + // Then handle the rest splitting on any spaces. + terms.AddRange(remainingText.Split(' ', StringSplitOptions.RemoveEmptyEntries).Select(s => new OptionalTextFilter + { + SearchTerm = s + })); + + SearchTerms = terms.ToArray(); SearchNumber = null; - if (SearchTerms.Length == 1 && int.TryParse(SearchTerms[0], out int parsed)) + if (SearchTerms.Length == 1 && int.TryParse(SearchTerms[0].SearchTerm, out int parsed)) SearchNumber = parsed; } } @@ -120,6 +142,8 @@ namespace osu.Game.Screens.Select { public bool HasFilter => !string.IsNullOrEmpty(SearchTerm); + public bool Exact { get; private set; } + public bool Matches(string value) { if (!HasFilter) @@ -129,10 +153,23 @@ namespace osu.Game.Screens.Select if (string.IsNullOrEmpty(value)) return false; + if (Exact) + return Regex.IsMatch(value, $@"(^|\s){searchTerm}($|\s)", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant); + return value.Contains(SearchTerm, StringComparison.InvariantCultureIgnoreCase); } - public string SearchTerm; + private string searchTerm; + + public string SearchTerm + { + get => searchTerm; + set + { + searchTerm = value.Trim('"'); + Exact = searchTerm != value; + } + } public bool Equals(OptionalTextFilter other) => SearchTerm == other.SearchTerm; } diff --git a/osu.Game/Screens/Select/FilterQueryParser.cs b/osu.Game/Screens/Select/FilterQueryParser.cs index c86554ddbc..474a9fdfea 100644 --- a/osu.Game/Screens/Select/FilterQueryParser.cs +++ b/osu.Game/Screens/Select/FilterQueryParser.cs @@ -161,7 +161,7 @@ namespace osu.Game.Screens.Select switch (op) { case Operator.Equal: - textFilter.SearchTerm = value.Trim('"'); + textFilter.SearchTerm = value; return true; default: From 8a7a42b7ec28fc7af008783cff236b3dbde67a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 26 Jun 2023 22:19:52 +0200 Subject: [PATCH 4/7] Remove weird nullable enable and double licence header --- osu.Game/Screens/Select/FilterCriteria.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/osu.Game/Screens/Select/FilterCriteria.cs b/osu.Game/Screens/Select/FilterCriteria.cs index 3fcc8d5476..0024c431f1 100644 --- a/osu.Game/Screens/Select/FilterCriteria.cs +++ b/osu.Game/Screens/Select/FilterCriteria.cs @@ -1,10 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable enable -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. -// See the LICENCE file in the repository root for full licence text. - using System; using System.Collections.Generic; using System.Linq; From 4873aaf7ede1d7558cc551d0744dbec3637543b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 26 Jun 2023 22:27:08 +0200 Subject: [PATCH 5/7] Add failing test case for filters potentially crashing due to invalid regex --- osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs b/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs index 8bd9407599..d8a56ea478 100644 --- a/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs +++ b/osu.Game.Tests/NonVisual/Filtering/FilterMatchingTest.cs @@ -169,6 +169,7 @@ namespace osu.Game.Tests.NonVisual.Filtering [TestCase("\"tags to\"", true)] [TestCase("\"version\"", false)] [TestCase("\"an auteur\"", true)] + [TestCase("\"\\\"", true)] // nasty case, covers properly escaping user input in underlying regex. public void TestCriteriaMatchingExactTerms(string terms, bool filtered) { var exampleBeatmapInfo = getExampleBeatmap(); From 4cb122dad46b3cff9de615b47e7220fd8488297d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 26 Jun 2023 22:27:48 +0200 Subject: [PATCH 6/7] Escape user input before embedding into regex --- osu.Game/Screens/Select/FilterCriteria.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/FilterCriteria.cs b/osu.Game/Screens/Select/FilterCriteria.cs index 0024c431f1..22780acfc3 100644 --- a/osu.Game/Screens/Select/FilterCriteria.cs +++ b/osu.Game/Screens/Select/FilterCriteria.cs @@ -150,7 +150,7 @@ namespace osu.Game.Screens.Select return false; if (Exact) - return Regex.IsMatch(value, $@"(^|\s){searchTerm}($|\s)", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant); + return Regex.IsMatch(value, $@"(^|\s){Regex.Escape(searchTerm)}($|\s)", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant); return value.Contains(SearchTerm, StringComparison.InvariantCultureIgnoreCase); } From e998be0eee006224e3212ed800901aa6237ecce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 26 Jun 2023 22:29:20 +0200 Subject: [PATCH 7/7] Use `== true` rather than `?? false` --- osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs index f020cd02f7..00a0d4a849 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneFilterControl.cs @@ -210,7 +210,7 @@ namespace osu.Game.Tests.Visual.SongSelect InputManager.Click(MouseButton.Left); }); - AddAssert("collection filter still selected", () => control.CreateCriteria().CollectionBeatmapMD5Hashes?.Any() ?? false); + AddAssert("collection filter still selected", () => control.CreateCriteria().CollectionBeatmapMD5Hashes?.Any() == true); AddAssert("filter request not fired", () => !received); }