Refactor role handling: Introduce JsonPartialRole and PartialRole for improved deserialization

[jedrek0429]

No description provided.

[KubaZ2]

Thanks, could you explain why this is even needed? There are many cases in which Discord returns some partial objects, and it so far worked that we just used default values for the omitted properties. That way we don't introduce an additional complexity when it is not needed.

[jedrek0429]

Thanks, could you explain why this is even needed? There are many cases in which Discord returns some partial objects, and it so far worked that we just used default values for the omitted properties. That way we don't introduce an additional complexity when it is not needed.

Suppose an invite grants an Administrator role. Discord sends the id, the name, etc. but deliberately does not send permissions. Then if NetCord constructs a normal Role from that payload you can write invite.Roles[0].Permissions and you get:

Permissions.None

That statement is false. Discord did not tell us that the role has no permissions, it told us nothing about its permissions. It's just a semantic problem.

Additionally the existing Role is documented as “Represents a role in a guild” and exposes all of those properties as if they were authoritative. So defaulting leaks through the public API as apparently real data.

There is also precedence: PartialGuildUser, for example, explicitly represents a guild user lacking GuildId and methods depending on it, while GuildUser derives from it. That said, if NetCord intentionally treats omitted fields this way and prefers keeping one model for the "same" conceptual Discord object, then I agree this may be more complexity than the library wants.

As to why this specific class drew my attention, I was just checking recent api changelog entries against NetCord and found this mismatch, and the implementation is very small.

[KubaZ2]

I think your concerns are generally valid, nobody wants to get invalid data. Are you sure there are no more cases in which returned roles are partial in the API? Also I think ideally it should be Role : PartialRole and PartialRole gets all the RestClient aliases generated.

Also PartialGuildUser is a slightly different case. Due to the fact it doesn't have the GuildId available, there is no way for it to have RestClient aliases. That's the main reason it exists.

[jedrek0429]

Also I think ideally it should be Role : PartialRole and PartialRole gets all the RestClient aliases generated.

I think this could become awkward because PartialRole is an Entity whilst Role is a ClientEntity, so PartialRole would need an overload constructor just for Role, and would need to contain the RestClient it doesn't need. That's why I opted for a joint interface implementation instead of class inheritance.

[KubaZ2]

I think this could become awkward because PartialRole is an Entity whilst Role is a ClientEntity, so PartialRole would need an overload constructor just for Role, and would need to contain the RestClient it doesn't need. That's why I opted for a joint interface implementation instead of class inheritance.

Not awkward at all, PartialRole needs RestClient anyway for RestClient aliases. Also ClientEntity is only a nice helper class so that each object that requires a RestClient doesn't need its own property for it written manually.

[github-actions]

The documentation preview is available at https://preview.netcord.dev/413.

[jedrek0429]

@KubaZ2 this is now fully ready