Fix disparate score token types being used as one type #24697
Conversation
It was only generally half-true that the thing was a "score ID"; in solo, it was a score token ID all along, and in multiplayer it was a multiplayer score ID. But that is soon to change, and it will become a "score link ID", which is close enough to a score token to warrant a rename.
This will allow spectator server to distinguish which type of token it is dealing with (either solo, which means it should look up the ID in `solo_score_tokens`, or multiplayer, which means it should look up the ID in `multiplayer_score_links`).
| [Serializable] | ||
| [MessagePackObject] | ||
| public record ScoreToken( | ||
| [property: Key(0)] long ID, | ||
| [property: Key(1)] ScoreTokenType Type); |
There was a problem hiding this comment.
Not sure you'll like this syntax, especially the [property: ] stuff. I did it this way because it's kinda convenient for generating equality members & .ToString() which especially begins to matter on the spectator-server side. But I'll switch to standard class if this offends.
| /// <summary> | ||
| /// Constructs a <see cref="ScoreToken"/> for later use | ||
| /// from the <see cref="APIScoreToken"/> returned by web. | ||
| /// </summary> | ||
| protected abstract ScoreToken RetrieveScoreToken(APIScoreToken token); |
There was a problem hiding this comment.
I generally wanted to avoid creating this method and try to constrain this in CreateTokenRequest(), but the C# type system makes this annoying.
tl;dr: I would ideally do
protected abstract APIRequest<IAPIScoreToken> CreateTokenRequest()with two implementations of IAPIScoreToken for solo and for multi. However the C# type system makes this extra annoying, since you can only have variance on interface generics, so that'd mean I'd have to do IAPIRequest<T> first, then probably something like IAPIScoreTokenRequest<T>, only then can it have IAPIScoreToken in the generic...... I don't think so. Unless you consider that better I guess.
Aside from more safety, also resolves "field never accessed" inspections.
Aside from the numerical ID of the token, it will log the type too now.
This PR adds documentation about the multiplayer score submission flow in the form of a sequence diagram. It was primarily created by taking the existing solo score submission flow sequence diagram and adapting it to the differences specific to the multi flow (different endpoints, different tables, different ID schemes). This diagram does not describe the current state of things. It describes where things _should_ be after a sequence of some already-PRed changes such as: - ppy/osu-web#10437 - ppy/osu#24697 - ppy/osu-server-spectator#185 and some changes that are still being worked on (primarily client-side at this point). The main goal of the document is to inform the changes above. (And to be something that I can easily refer to, because I keep looking up the same information over and over right now.)
|
As I mentioned in discord, sorry about this but I don't think we'll be requiring this (ignoring the wasted effort, that's probably a huge plus). I've been holding off reviewing this while I got my thoughts together and reassessed the whole structure of things, which I've documented over on ppy/osu-infrastructure#24. Won't close just yet, but going to block until we confirm the direction in the linked issue is feasible (I believe @nanaya has already started osu-web implementation of it). |
|
Closing for now to keep things clean. See ppy/osu-infrastructure#24 for rationale. |
More stuff related to #24229. I said I was gonna fix single player replays first, but on second thought I just could not look away from the magnitude of this breakage.
The replay upload flow and the user statistics update flow would rely on the score token sent by client to spectator server in
BeginPlaySession(), in order to unmap the score token to a solo score ID when processing finishes.However, spectator server thus far has not been aware that there are actually two types of token - and to be fair, client gave it no practical way to actually know this. In solo, the token ID comes from the
solo_score_tokenstable, and in multiplayer, the token ID comes from themultiplayer_score_linkstable (after ppy/osu-web#10437). Spectator server has however been treating both types as the solo score type. In particular, this could mean that the replay recording flow would royally confuse things and unmap the token to a score ID wrong due to looking for it in the wrong table, and as such, upload an entirely misattributed replay. (See also: relevant discord conversation.)This PR aims to clean this up by passing a
ScoreTokenstructure to spectator server rather than a plainlong.I'm not adding a dependency on ppy/osu-web#10437 here because the current token handling is wrong anyhow and would have needed to be fixed. That pull only begins to functionally matter for the upcoming
osu-server-spectator-side change.Deployment
This is a straight-up breaking change, as in:
by which I mean that after the upcoming
osu-spectator-server-side change is deployed, old clients will not be able to spectate.I wanted to make this not breaking, but gave up because signalr is just not allowing that to happen because you can't overload hub methods, period. So I just did a straight-up API break. For whatever that's worth, we did that last time, too.
If you think backcompat is a blocker here, then I'll try again, but it'll 99% take at least a method rename.