MessageUpdated sometimes returns a null SocketMessage if MessageCacheSize == 0

[jmazouri]

I was seeing an issue where MessageUpdated was occasionally giving me a null arg2 (the SocketMessage), and was able to reproduce it (on the latest version) by sending a message with a URL for which Discord generates an embed automatically - such as https://github.com/RogueException/Discord.Net - but, notably, only when MessageCacheSize is 0; MessageUpdated's arg2 will never be null if the cache is enabled.

This seems to due to the fact that the MESSAGE_UPDATE gateway event, if updating a message to add an embed, doesn't contain author data - and the check on DiscordSocketClient.cs#1221 will only create a "detached" SocketMessage if the author is present. The SocketMessage passed to the event on DiscordSocketClient.cs#1212, after, remains null - as without MessageCacheSize being set, the call to channel.GetCachedMessage will always return null.

According to the documentation (and messages on DAPI), the presence of MessageCacheSize should only affect arg1 of MessageUpdated, but it seems that it indirectly affects both. I'm not sure what the best solution is in this case, but perhaps the "MESSAGE_UPDATE" case could be modified to return a SocketMessage with missing properties (or a less derived type that matches better). This would at least match the functionality described in Discord's API docs:

Unlike creates, message updates may contain only a subset of the full message object payload (but will always contain an id and channel_id).
https://discordapp.com/developers/docs/topics/gateway#message-update

Of course, the solution for now is just "Set MessageCacheSize", but I still think the event should return, at minimum, what the Discord API does (message and channel IDs).

[Joe4evr]

The Cacheable always has the message ID. Channels are always cached, so that's never null and you can get the ID off that.

[jmazouri]

Good point, I'll have to implement a check for that - but I still think it should be made clearer, as the docs and event args imply the SocketMessage will never be null.

[Joe4evr]

That has never been the behavior of Cacheable. And the doc also clearly says:

1. "[...] if it could be pulled from cache."
2. "[...] in cases where the entity cannot be pulled from cache, it is null."

[jmazouri]

The Cacheable is not the issue - it makes sense that the original message may not be cached, and Cacheable exists to better represent cases when it's not. I'm questioning the second arg, which is presented as a raw SocketMessage.

Perhaps the solution, then, is to make the second argument a type similar to Cacheable<SocketMessage, ulong> - though, of course, without the ability to download it.