from_json does an optimization to avoid double deserialization since dd49d6e. It finds op, s and t by scanning whole payload and taking the first match. This is incorrect. It relies on implementation details that Discord doesn't document/guarantee.
The optimization relies on the order of the fields. But this assumption is not part of the documented Discord API. Discord could change the order the fields. Best case this breaks the perfomance optimization because the code scans the whole payload. Worst case, somewhere inside d the other fields also show up and then get incorrectly interpreted as the outer fields, which breaks deserialization. Deserialization could also break if Discord introduces Json whitespace for these keys like going from "op":0 to "op" : 0.
It might be unlikely that Discord does this, but the point is that they could and they wouldn't be breaking any documentation from their side. We shouldn't rely on this.
I mainly wanted to document the problem but I also have an idea for the solution.
The original performance optimization getting rid of serde_value::Value is unnecessary. Assuming Discord does keep the current order, we can keep the same performance while also being correct in case they change it.
In the deserialization code, handle op, s, t in the order they arrive. We expect them to come before d. When they do indeed come before, then continue to parse d. If the order is unexpected and d comes first, then buffer into serde_value::Value and deserialize it properly after the other fields.
Perfomance in the expected case should be roughly the same as now. As a nice side effect, we can impl Deserialize for GatewayEvent directly instead of going through GatewayEventDeserializer. Can keep the current interface so it's not a breaking change.
Note that serde_value::Value is not serde_json::Value. It is agnostic of the Json implementation (serde_json, simd_json). We already depend on serde_value::Value.
Also note that the existing bench uses the wrong order of fields and anyway is not good test for this optimization.
from_jsondoes an optimization to avoid double deserialization since dd49d6e. It findsop,sandtby scanning whole payload and taking the first match. This is incorrect. It relies on implementation details that Discord doesn't document/guarantee.The optimization relies on the order of the fields. But this assumption is not part of the documented Discord API. Discord could change the order the fields. Best case this breaks the perfomance optimization because the code scans the whole payload. Worst case, somewhere inside
dthe other fields also show up and then get incorrectly interpreted as the outer fields, which breaks deserialization. Deserialization could also break if Discord introduces Json whitespace for these keys like going from"op":0to"op" : 0.It might be unlikely that Discord does this, but the point is that they could and they wouldn't be breaking any documentation from their side. We shouldn't rely on this.
I mainly wanted to document the problem but I also have an idea for the solution.
The original performance optimization getting rid of
serde_value::Valueis unnecessary. Assuming Discord does keep the current order, we can keep the same performance while also being correct in case they change it.In the deserialization code, handle
op,s,tin the order they arrive. We expect them to come befored. When they do indeed come before, then continue to parsed. If the order is unexpected anddcomes first, then buffer intoserde_value::Valueand deserialize it properly after the other fields.Perfomance in the expected case should be roughly the same as now. As a nice side effect, we can
impl Deserialize for GatewayEventdirectly instead of going throughGatewayEventDeserializer. Can keep the current interface so it's not a breaking change.Note that
serde_value::Valueis notserde_json::Value. It is agnostic of the Json implementation (serde_json,simd_json). We already depend onserde_value::Value.Also note that the existing bench uses the wrong order of fields and anyway is not good test for this optimization.