5.x major change - Rewrite and abstract serialization, deserialization & entity updates
Created by: uwx
Summary
Complete rewrite of the serialization backend, along with updating cached entities.
Fixing this issue also addresses #571, and prevents issues such as #428 (closed) (and possibly others) from cropping up in the future.
Details
ISerializer overview
The ISerializer interface would abstract away all serialization functionality. The library will never have to directly interact with JSON data.
There is no .ToObject/.ToDiscordObject equivalent, instead there should be a custom ITypeConverter for GatewayPayload.Data that emits an adequate subtype.
Deserializaton method, such as ISerializer.Deserialize<SomeType>
Populate method, such as ISerializer.Populate<SomeType>
Serialization method, such as ISerializer.Serialize<SomeType>
DiscordPropertyAttribute overview
An attribute that replaces the current JsonProperty behavior. It would look something like this
[DiscordProperty("property_name", Nullable = false, Converter = typeof(DiscordSomeConverter))]
public DiscordSomeType Property { get; internal set; }
[DiscordProperty("optional_property", Nullable = false, Converter = typeof(DiscordSomeConverter))]
public Optional<DiscordSomeType> OptionalProperty { get; internal set; }The Nullable property is a a replacement for NullValueHandling that obeys the Discord standard more strictly.
Optional<T> supercedes our current uses of optionals, as well as places in the library where we presently use NullValueHandling.Ignore incorrectly in place of an optional property.
Refer to the key:
The Nullable property defaults to false, and the Converter property is optional, and points to an implementation of ITypeConverter. The default converters are provided by the ISerializer implementation and they are as follows:
| C# Type | JSON Type |
|---|---|
| string | string |
| boolean | boolean |
| byte | number |
| sbyte | number |
| short | number |
| ushort | number |
| int | number |
| uint | number |
| long | number |
| ulong | number |
| char | string |
| float | number |
| double | number |
| decimal | number |
| Enum and subtypes | number |
| object (non-primitive) | object or null |
| IReadOnlyDictionary<T> | object of T or null |
| IDictionary<T> | object of T or null |
| Dictionary<T> | object of T or null |
| ConcurrentDictionary<T> | object of T or null |
| IReadOnlyList<T> | array of T or null |
| IList<T> | array of T or null |
| List<T> | array of T or null |
| IEnumberable<T> | array of T or null |
| Nullable<T> | T or null |
| Optional<T> | T or N/A[1] |
[1]: Optional<T> can only be a member of an object (you cannot have an array of Optional<T>), and is serialized as T if the Optional has a value, or not added to the parent object at all if it doesn't have one. It is deserialized as T then wrapped in an Optional<T> if the key is present, or as an empty Optional<T> if the key isn't present.
Alternative: Make Converter an attribute of the type itself instead
ITypeConverter<T> overview
This interface represents a type which handles converting special types between an IJsonToken and the output T.
Read method, such as T ITypeConverter.Read(ISerializer, IJsonToken)
Write method, such as IJsonToken ITypeConverter.Write(ISerializer, T)
IJsonToken overview
Details pending. The general idea is for it to behave similarly to JToken (JSON.NET) or JsonElement (System.Text.Json).
Transport entity changes overview
Transport entities should be abolished entirely. Instead the ISerializer along with custom ITypeConverters should be able to emit a full object every time.
Entity update changes overview
Cache/entity update copy-and-pastes such as the follows:
should be abolished entirely, instead use ISerializer.Populate.
The same generally applies to things like this:
This large snippet contains hand-crafted updates that can be abstracted away entirely into a single line of code.
The most egregious examples of these issues are cache updates for Guilds and Channels.
Superseding ConcurrentDictionary and ReadOnlyConcurrentDictionary using SnowflakeArrayAsDictionaryJsonConverter
Since developing this system of using dictionaries keyed by the snowflake ID I have come to the realization that using a generic solution (ConcurrentDictionary/ReadOnlyConcurrentDictionary) is a lesser alternative to simply having a custom dictionary type, say SnowflakeKeyedDictionary<T> where T : SnowflakeObject. This would make everything much simpler:
- Can implement IReadOnlyDictionary (and/or IReadOnlySet), does not need to implement IDictionary
-
Addmethods can be internal, meaning it doesn't need to be backed by a property that wraps it in ReadOnlyConcurrentDictionary, which can save allocations, as well as making the code far simpler-looking -
Addmethods can simply take the entity itself, and set the key to the ID automatically, making it behave sort of like a Set<T> keyed by the ID, and having less room for error, as you can't mistakenly pass something to it that isn't the actual ID - Converter can be added to the type itself rather than each property
Former issue details
Summary
Ref issues: #165 (closed), #428 (closed), possibly others
The entities returned by the API, whether they be from the socket or REST, are rarely ever complete. As a result, we tend to not handle them well. Entities are updated basically based on guesswork and this persistently leads to obtuse, hard-to-find bugs in the library that are easy to write off as "bad API response" or "cosmic rays". I propose a general-purpose solution for this recurring problem.
Details
The main cause of this problem is what I've dubbed "deserialization spaghetti":
With this design, it's incredibly easy to forget to update every single place in the library whenever one dares touch an entity. And this has happened. Many, many times. The addition of Optional has made this much better, and #165 (closed) will help with this (provided it eradicates or suppresses the Transport entities), but these changes by themselves can't fix the underlying problem - bad code localization.
As a result, instead of blaming it on developer stupidity and fixing issues as they arise, I posit that there should be a major change to how we handle entity data transfer. The reference implementation of the Discord API is effectively the website itself, and through this we can infer several things based on what would be easy, lazy and reasonably effective for serialization and deserialization in JavaScript.
The difficult part is porting what is easy in JavaScript into C#, as it requires a tremendous amount of reflection. Preferably, we want to never have to set those property names anywhere except the member definition itself, and have a standard approach for handling transformation. Then, some day, perhaps the above mountain of code, and much, much more, could be reduced to a single call to a relevant method.
I guess I can dream.
Notes
Part of this extended thought may have been (totally was) inspired (directly stolen) by Emzi's new concepts for Clyde.NET.