Merge pull request ppy#190 from smoogipoo/failed-score-no-replay-upload

Don't upload replays for failed scores

osu.Server.Spectator.Tests/ScoreUploaderTests.cs

@@ -7,6 +7,7 @@
 using Moq;
 using osu.Game.Scoring;
 using osu.Server.Spectator.Database;
+using osu.Server.Spectator.Database.Models;
 using osu.Server.Spectator.Hubs;
 using osu.Server.Spectator.Storage;
 using Xunit;
@@ -22,7 +23,14 @@ public class ScoreUploaderTests
         public ScoreUploaderTests()
         {
             mockDatabase = new Mock<IDatabaseAccess>();
-            mockDatabase.Setup(db => db.GetScoreIdFromToken(1)).Returns(Task.FromResult<long?>(2));
+            mockDatabase.Setup(db => db.GetScoreFromToken(1)).Returns(Task.FromResult<SoloScore?>(new SoloScore
+            {
+                ScoreInfo =
+                {
+                    ID = 2,
+                    Passed = true
+                }
+            }));
 
             var databaseFactory = new Mock<IDatabaseFactory>();
             databaseFactory.Setup(factory => factory.GetInstance()).Returns(mockDatabase.Object);
@@ -80,8 +88,17 @@ public async Task ScoreUploadsWithDelayedScoreToken()
             mockStorage.Verify(s => s.WriteAsync(It.IsAny<Score>()), Times.Never);
 
             // Give the score a token.
-            mockDatabase.Setup(db => db.GetScoreIdFromToken(2)).Returns(Task.FromResult<long?>(3));
+            mockDatabase.Setup(db => db.GetScoreFromToken(2)).Returns(Task.FromResult<SoloScore?>(new SoloScore
+            {
+                ScoreInfo =
+                {
+                    ID = 3,
+                    Passed = true
+                }
+            }));
+
             await uploader.Flush();
+
             mockStorage.Verify(s => s.WriteAsync(It.Is<Score>(score => score.ScoreInfo.OnlineID == 3)), Times.Once);
         }
 
@@ -99,7 +116,15 @@ public async Task TimedOutScoreDoesNotUpload()
             mockStorage.Verify(s => s.WriteAsync(It.IsAny<Score>()), Times.Never);
 
             // Give the score a token now. It should still not upload because it has timed out.
-            mockDatabase.Setup(db => db.GetScoreIdFromToken(2)).Returns(Task.FromResult<long?>(3));
+            mockDatabase.Setup(db => db.GetScoreFromToken(2)).Returns(Task.FromResult<SoloScore?>(new SoloScore
+            {
+                ScoreInfo =
+                {
+                    ID = 3,
+                    Passed = true
+                }
+            }));
+
             await uploader.Flush();
             mockStorage.Verify(s => s.WriteAsync(It.IsAny<Score>()), Times.Never);
 

osu.Server.Spectator.Tests/SpectatorHubTest.cs

@@ -120,7 +120,14 @@ public async Task ReplayDataIsSaved(bool savingEnabled)
             hub.Context = mockContext.Object;
             hub.Clients = mockClients.Object;
 
-            mockDatabase.Setup(db => db.GetScoreIdFromToken(1234)).Returns(Task.FromResult<long?>(456));
+            mockDatabase.Setup(db => db.GetScoreFromToken(1234)).Returns(Task.FromResult<SoloScore?>(new SoloScore
+            {
+                ScoreInfo =
+                {
+                    ID = 456,
+                    Passed = true
+                }
+            }));
 
             await hub.BeginPlaySession(1234, state);
             await hub.SendFrameData(new FrameDataBundle(
@@ -264,7 +271,15 @@ public async Task ScoresAreOnlySavedOnRankedBeatmaps(BeatmapOnlineStatus status,
             hub.Context = mockContext.Object;
             hub.Clients = mockClients.Object;
 
-            mockDatabase.Setup(db => db.GetScoreIdFromToken(1234)).Returns(Task.FromResult<long?>(456));
+            mockDatabase.Setup(db => db.GetScoreFromToken(1234)).Returns(Task.FromResult<SoloScore?>(new SoloScore
+            {
+                ScoreInfo =
+                {
+                    ID = 456,
+                    Passed = true
+                }
+            }));
+
             mockDatabase.Setup(db => db.GetBeatmapAsync(beatmap_id)).Returns(Task.FromResult(new database_beatmap
             {
                 approved = status,
@@ -284,5 +299,41 @@ await hub.SendFrameData(new FrameDataBundle(
             else
                 mockScoreStorage.Verify(s => s.WriteAsync(It.Is<Score>(score => score.ScoreInfo.OnlineID == 456)), Times.Never);
         }
+
+        [Fact]
+        public async Task FailedScoresAreNotSaved()
+        {
+            AppSettings.SaveReplays = true;
+
+            Mock<IHubCallerClients<ISpectatorClient>> mockClients = new Mock<IHubCallerClients<ISpectatorClient>>();
+            Mock<ISpectatorClient> mockReceiver = new Mock<ISpectatorClient>();
+            mockClients.Setup(clients => clients.All).Returns(mockReceiver.Object);
+            mockClients.Setup(clients => clients.Group(SpectatorHub.GetGroupId(streamer_id))).Returns(mockReceiver.Object);
+
+            Mock<HubCallerContext> mockContext = new Mock<HubCallerContext>();
+
+            mockContext.Setup(context => context.UserIdentifier).Returns(streamer_id.ToString());
+            hub.Context = mockContext.Object;
+            hub.Clients = mockClients.Object;
+
+            mockDatabase.Setup(db => db.GetScoreFromToken(1234)).Returns(Task.FromResult<SoloScore?>(new SoloScore
+            {
+                ScoreInfo =
+                {
+                    ID = 456,
+                    Passed = false
+                }
+            }));
+
+            await hub.BeginPlaySession(1234, state);
+            await hub.SendFrameData(new FrameDataBundle(
+                new FrameHeader(new ScoreInfo(), new ScoreProcessorStatistics()),
+                new[] { new LegacyReplayFrame(1234, 0, 0, ReplayButtonState.None) }));
+            await hub.EndPlaySession(state);
+
+            await scoreUploader.Flush();
+
+            mockScoreStorage.Verify(s => s.WriteAsync(It.Is<Score>(score => score.ScoreInfo.OnlineID == 456)), Times.Never);
+        }
     }
 }

osu.Server.Spectator/Database/DatabaseAccess.cs

@@ -293,14 +293,15 @@ public async Task MarkScoreHasReplay(Score score)
             });
         }
 
-        public async Task<long?> GetScoreIdFromToken(long token)
+        public async Task<SoloScore?> GetScoreFromToken(long token)
         {
             var connection = await getConnectionAsync();
 
-            return await connection.QuerySingleOrDefaultAsync<long?>("SELECT `score_id` FROM `solo_score_tokens` WHERE `id` = @Id", new
-            {
-                Id = token
-            });
+            return await connection.QuerySingleOrDefaultAsync<SoloScore?>(
+                "SELECT * FROM `solo_scores` WHERE `id` = (SELECT `score_id` FROM `solo_score_tokens` WHERE `id` = @Id)", new
+                {
+                    Id = token
+                });
         }
 
         public async Task<bool> IsScoreProcessedAsync(long scoreId)

osu.Server.Spectator/Database/IDatabaseAccess.cs

@@ -121,11 +121,11 @@ public interface IDatabaseAccess : IDisposable
         Task MarkScoreHasReplay(Score score);
 
         /// <summary>
-        /// Retrieves the score ID for a given score token. Will return null while the score has not yet been submitted.
+        /// Retrieves the <see cref="SoloScore"/> for a given score token. Will return null while the score has not yet been submitted.
         /// </summary>
         /// <param name="token">The score token.</param>
-        /// <returns>The score ID.</returns>
-        Task<long?> GetScoreIdFromToken(long token);
+        /// <returns>The <see cref="SoloScore"/>.</returns>
+        Task<SoloScore?> GetScoreFromToken(long token);
 
         /// <summary>
         /// Returns <see langword="true"/> if the score with the supplied <paramref name="scoreId"/> has been successfully processed.

osu.Server.Spectator/Database/Models/SoloScore.cs

@@ -0,0 +1,61 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using System.ComponentModel.DataAnnotations.Schema;
+using System.Diagnostics.CodeAnalysis;
+using Newtonsoft.Json;
+using osu.Game.Online.API.Requests.Responses;
+
+namespace osu.Server.Spectator.Database.Models
+{
+    [SuppressMessage("ReSharper", "InconsistentNaming")]
+    [Serializable]
+    [Table("solo_scores")]
+    public class SoloScore
+    {
+        public ulong id { get; set; }
+
+        public int user_id { get; set; }
+
+        public int beatmap_id { get; set; }
+
+        public int ruleset_id { get; set; }
+
+        public string data
+        {
+            get => JsonConvert.SerializeObject(ScoreInfo);
+            set
+            {
+                var scoreInfo = JsonConvert.DeserializeObject<SoloScoreInfo>(value);
+
+                if (scoreInfo == null)
+                    return;
+
+                ScoreInfo = scoreInfo;
+
+                ScoreInfo.ID = id;
+                ScoreInfo.BeatmapID = beatmap_id;
+                ScoreInfo.UserID = user_id;
+                ScoreInfo.RulesetID = ruleset_id;
+            }
+        }
+
+        [JsonIgnore]
+        public bool preserve { get; set; }
+
+        [JsonIgnore]
+        public bool has_replay { get; set; }
+
+        [JsonIgnore]
+        public SoloScoreInfo ScoreInfo = new SoloScoreInfo();
+
+        [JsonIgnore]
+        public DateTimeOffset created_at { get; set; }
+
+        [JsonIgnore]
+        public DateTimeOffset updated_at { get; set; }
+
+        public override string ToString() => $"score_id: {id} user_id: {user_id}";
+    }
+}

osu.Server.Spectator/Hubs/ScoreUploader.cs

@@ -7,6 +7,7 @@
 using System.Threading.Tasks;
 using osu.Game.Scoring;
 using osu.Server.Spectator.Database;
+using osu.Server.Spectator.Database.Models;
 using osu.Server.Spectator.Entities;
 using osu.Server.Spectator.Storage;
 
@@ -90,9 +91,9 @@ public async Task Flush()
                         if (!queue.TryDequeue(out var item))
                             continue;
 
-                        long? scoreId = await db.GetScoreIdFromToken(item.Token);
+                        SoloScore? dbScore = await db.GetScoreFromToken(item.Token);
 
-                        if (scoreId == null && !item.Cancellation.IsCancellationRequested)
+                        if (dbScore == null && !item.Cancellation.IsCancellationRequested)
                         {
                             // Score is not ready yet - enqueue for the next attempt.
                             queue.Enqueue(item);
@@ -101,14 +102,26 @@ public async Task Flush()
 
                         try
                         {
-                            if (scoreId != null)
+                            if (dbScore == null)
                             {
-                                item.Score.ScoreInfo.OnlineID = scoreId.Value;
-                                await scoreStorage.WriteAsync(item.Score);
-                                await db.MarkScoreHasReplay(item.Score);
-                            }
-                            else
                                 Console.WriteLine($"Score upload timed out for token: {item.Token}");
+                                return;
+                            }
+
+                            if (!dbScore.ScoreInfo.Passed)
+                                return;
+
+                            var updatedScore = new Score
+                            {
+                                Replay = item.Score.Replay,
+                                ScoreInfo = dbScore.ScoreInfo.ToScoreInfo(item.Score.ScoreInfo.Mods, item.Score.ScoreInfo.BeatmapInfo)
+                            };
+
+                            // This needs to be updated to a valid reference for the score encoder.
+                            updatedScore.ScoreInfo.Ruleset = item.Score.ScoreInfo.Ruleset;
+
+                            await scoreStorage.WriteAsync(updatedScore);
+                            await db.MarkScoreHasReplay(updatedScore);
                         }
                         finally
                         {

osu.Server.Spectator/Hubs/Spectator/ScoreProcessedSubscriber.cs

@@ -11,6 +11,7 @@
 using osu.Framework.Logging;
 using osu.Game.Online.Spectator;
 using osu.Server.Spectator.Database;
+using osu.Server.Spectator.Database.Models;
 using StackExchange.Redis;
 using StatsdClient;
 using Timer = System.Timers.Timer;
@@ -83,28 +84,28 @@ public async Task RegisterForNotificationAsync(string receiverConnectionId, int
             {
                 using var db = databaseFactory.GetInstance();
 
-                long? scoreId = await db.GetScoreIdFromToken(scoreToken);
+                SoloScore? score = await db.GetScoreFromToken(scoreToken);
 
-                if (scoreId == null)
+                if (score == null)
                 {
                     DogStatsd.Increment($"{statsd_prefix}.subscriptions.dropped");
                     return;
                 }
 
-                var subscription = new ScoreProcessedSubscription(receiverConnectionId, userId, scoreId.Value, spectatorHubContext);
+                var subscription = new ScoreProcessedSubscription(receiverConnectionId, userId, (long)score.id, spectatorHubContext);
 
                 // because the score submission flow happens concurrently with the spectator play finished flow,
                 // it is theoretically possible for the score processing to complete before the spectator hub had a chance to register for notifications.
                 // to cover off this possibility, check the database directly once.
-                if (await db.IsScoreProcessedAsync(scoreId.Value))
+                if (await db.IsScoreProcessedAsync((long)score.id))
                 {
                     using (subscription)
                         await subscription.InvokeAsync();
                     DogStatsd.Increment($"{statsd_prefix}.messages.delivered-immediately");
                     return;
                 }
 
-                subscriptions.TryAdd(scoreId.Value, subscription);
+                subscriptions.TryAdd((long)score.id, subscription);
                 DogStatsd.Gauge($"{statsd_prefix}.subscriptions.total", subscriptions.Count);
             }
             catch (Exception ex)