Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite Event.DeserializationStrategy #923

Merged
merged 28 commits into from
Mar 16, 2024
Merged

Rewrite Event.DeserializationStrategy #923

merged 28 commits into from
Mar 16, 2024

Conversation

lukellmann
Copy link
Member

@lukellmann lukellmann commented Mar 5, 2024

When deserializing Events, Event.DeserializationStrategy assumed that the d field was the last field to be observed. If it wasn't the last field, the deserialization could fail in two ways:

  • the t and s fields were ignored if they came after the d field
  • an exception was thrown if the op field came after the d field

To fix these possible failure cases, the deserialization logic has been changed to work in two steps:

  1. decode all fields regardless of order, treating the d field as a plain JsonElement
  2. after all fields have been decoded, construct an Event from the JsonElement, depending on the values of the op, t and s fields

The supertype of Event.DeserializationStrategy has also been changed from DeserializationStrategy<Event?> to DeserializationStrategy<Event> - deserialize no longer returns null in some cases of illegal event payloads, but throws exceptions instead.

Fixes #922

lukellmann and others added 2 commits March 15, 2024 18:18
Co-authored-by: Michael Rittmeister <michael@rittmeister.in>
@lukellmann lukellmann marked this pull request as ready for review March 16, 2024 01:43
@lukellmann lukellmann merged commit d1651a7 into main Mar 16, 2024
8 of 9 checks passed
@lukellmann lukellmann deleted the unknown-d-field-fix branch March 16, 2024 01:55
lukellmann added a commit that referenced this pull request Mar 16, 2024
To keep the diff reasonable, I left this out of #923.
lukellmann added a commit that referenced this pull request Mar 16, 2024
Similar to #923 but for the voice gateway.
lukellmann added a commit that referenced this pull request Mar 16, 2024
Some tests added in #923 seem to time out on Kotlin/JS. To avoid
problems like this in the future, this commit disables test timeouts on
Kotlin/JS completely.
DRSchlaubi pushed a commit that referenced this pull request Mar 16, 2024
Some tests added in #923 seem to time out on Kotlin/JS. To avoid
problems like this in the future, this commit disables test timeouts on
Kotlin/JS completely.
DRSchlaubi pushed a commit that referenced this pull request Mar 16, 2024
To keep the diff reasonable, I left this out of #923.
DRSchlaubi pushed a commit that referenced this pull request Mar 19, 2024
* Rewrite VoiceEvent.DeserializationStrategy

Similar to #923 but for the voice gateway.

* Log and ignore all unexpected opcodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERROR dev.kord.gateway.DefaultGateway
2 participants