Trigger request failure on receiving a null response for a typed APIRequest

[peppy]

RFC. Seems like about what we'd want. Any kind of issue which results in deserialising not being feasible should be considered a failure for a typed APIRequest as far as I'm concerned.

[bdach]

We cannot do this (at least not without adjustments web-side) because there is at least one case where an APIRequest we have typed to a generic returns a null response and it is not an error.

It is the case that was originally reported on discord. To test this, a brand new account is required.

GET /chat/updates web-side does this. TL;DR: if web determines that all the fields requested by the client it is responding to are essentially blank, it will not produce a 200 OK with the appropiate shape of json but blanked out, but will instead produce a 204 No Content response with, well, no content (as those are not supposed to have a body). There is no error here.

This stops happening on existing accounts because existing accounts have their channel lists populated.

I don't think we can handle this in a generic manner and a carve-out for specific request types is required. A patch that I have that appears to fix the aforementioned particular instance of this follows:

diff --git a/osu.Game/Online/API/APIRequest.cs b/osu.Game/Online/API/APIRequest.cs
index 45ebbcd76d..d812f15120 100644
--- a/osu.Game/Online/API/APIRequest.cs
+++ b/osu.Game/Online/API/APIRequest.cs
@@ -17,7 +17,7 @@ namespace osu.Game.Online.API
     /// An API request with a well-defined response type.
     /// </summary>
     /// <typeparam name="T">Type of the response (used for deserialisation).</typeparam>
-    public abstract class APIRequest<T> : APIRequest where T : class
+    public abstract class APIRequest<T> : APIRequest
     {
         protected override WebRequest CreateWebRequest() => new OsuJsonWebRequest<T>(Uri);
 
diff --git a/osu.Game/Online/API/Requests/GetUpdatesRequest.cs b/osu.Game/Online/API/Requests/GetUpdatesRequest.cs
index 529c579996..b277bb9969 100644
--- a/osu.Game/Online/API/Requests/GetUpdatesRequest.cs
+++ b/osu.Game/Online/API/Requests/GetUpdatesRequest.cs
@@ -1,20 +1,17 @@
 // 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.
 
-#nullable disable
-
-using JetBrains.Annotations;
 using osu.Framework.IO.Network;
 using osu.Game.Online.Chat;
 
 namespace osu.Game.Online.API.Requests
 {
-    public class GetUpdatesRequest : APIRequest<GetUpdatesResponse>
+    public class GetUpdatesRequest : APIRequest<GetUpdatesResponse?>
     {
         private readonly long since;
-        private readonly Channel channel;
+        private readonly Channel? channel;
 
-        public GetUpdatesRequest(long sinceId, [CanBeNull] Channel channel = null)
+        public GetUpdatesRequest(long sinceId, Channel? channel = null)
         {
             this.channel = channel;
             since = sinceId;
diff --git a/osu.Game/Online/Chat/WebSocketChatClient.cs b/osu.Game/Online/Chat/WebSocketChatClient.cs
index a74f0222f2..37774a1f5d 100644
--- a/osu.Game/Online/Chat/WebSocketChatClient.cs
+++ b/osu.Game/Online/Chat/WebSocketChatClient.cs
@@ -80,7 +80,7 @@ public void RequestPresence()
 
             fetchReq.Success += updates =>
             {
-                if (updates.Presence != null)
+                if (updates?.Presence != null)
                 {
                     foreach (var channel in updates.Presence)
                         joinChannel(channel);
diff --git a/osu.Game/Tests/PollingChatClient.cs b/osu.Game/Tests/PollingChatClient.cs
index 75975c716b..eb29b35c1d 100644
--- a/osu.Game/Tests/PollingChatClient.cs
+++ b/osu.Game/Tests/PollingChatClient.cs
@@ -48,7 +48,7 @@ protected APIRequest CreateInitialFetchRequest(long? lastMessageId = null)
 
             fetchReq.Success += updates =>
             {
-                if (updates.Presence != null)
+                if (updates?.Presence != null)
                 {
                     foreach (var channel in updates.Presence)
                         handleChannelJoined(channel);

The dropping of the T : class constraint on APIRequest is a bit scary, and I have not done any extensive testing as to whether it is fine, but I also don't immediately see why it would be an issue, or even get why that constraint existed in the first place. Blame points at 7ecce71 which has very little relevant context.

If you've found more cases of this elsewhere then a local nullability allow might need to be applied to those too.

[bdach]

see above

[bdach]

@ppy/team-web can you give your thoughts here on what we should do? A TL;DR question would be: How common it is for the API to return 204 No Content without a body instead of the usual documented response shape in the body if the documented response would be essentially empty?

[nanaya]

For that specific endpoint, I think it was originally to minimise response size but the structure has since been changed and probably not useful anymore to return 204 in those cases. (and probably shouldn't return 204 in the first place or at least should've been documented)

It seems I remembered it right.

[bdach]

Then I guess removing that 204 return is possibly an option that can unblock this, so long as there is no other endpoint that does anything like that, which again, I'm not sure how to find out without going through every single endpoint that client uses...

edit: ppy/osu-web#11475 is open now

[peppy]

(and merged / deployed)

[bdach]

I guess we could try this then and see if it goes wrong experimentally...