Add New webhook endpoints #217
Conversation
DRSchlaubi
changed the title
rest/src/main/kotlin/builder/webhook/EditWebhookMessageBuilder.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Whilst I don't like the idea that we are mixing a Message that's supposed to be sent by a webhook with a normal message. This might lead to an error because someone decided to call editWebhook on a normal message and should be looked into. However, this is not something that relates to this PR.
There was a problem hiding this comment.
Implementing these endpoints seem to be a rather big hassle for our current structure. Message behaviours cannot reliably know when they are a webhook message or not (because either Discord won't tell us, or we've made assumptions about knowing enough that have now come back to bite us in the butt).
Because of this lack of information, I think the checks you introduced are brittle and will cause more problems than the might solve.
Looking at this from a user's perspective, no one should be trying to edit a random message and the distance between creating a message and editing it is relatively small in code. I don't think confusing whether a message is a webhook or not would a frequent occurrence. (certainly not without also having a full message object in hand).
Ideally, these checks would be covered by the type system instead. But even if we were to discern a user message from a webhook message with type system, we can't do that for the behaviors.
| channelId: Snowflake, | ||
| messageId: Snowflake, | ||
| kord: Kord, | ||
| webhookId: Snowflake? = null, |
There was a problem hiding this comment.
The presence/absence of a webhook ID isn't always known, it would be a mistake to include it into the behaviour, as well as make any kind of decisions based on the field.
There was a problem hiding this comment.
What else should I do then?
Editing webhook messages require a different endpoint, which also requires the webhook id, hence it's part of the endpoint path. If the message is a webhook message but does not have a webhook id because discord didn't give it to us the edit would fail either way. Ideally we would have a webhookmessage entity but that raises the issue again that we don't always know whether something is a webhook message or not
There was a problem hiding this comment.
What else should I do then?
Make it a mandatory argument when editing a message from a behavior, drop the field from the behavior interface.
Discord has different behavior based on message types, so it might be time to implement Discord's message types as concrete types ourselves. If we do, we can create an overload for webhook messages that doesn't require the id argument. That's something for another PR though.
| callsInPlace(builder, InvocationKind.EXACTLY_ONCE) | ||
| } | ||
|
|
||
| require(webhookId != null) { "Cannot perform edits on non webhook messages use edit() instead" } |
There was a problem hiding this comment.
As noted above, the webhook snowflake not being present does not necessarily indicate whether a message is a webhook message or not. This is a very brittle check and can break in scenarios where a message is a webhook message but the code is not aware of it.
I've gone through multiple libraries and checked how they identify a webhook, it seems to be through the webook_id field. |
I didn't like that too, when I implemented it that way but I didn't want to change everything related to webhooks in this PR. It is something we need to change though |
DRSchlaubi
force-pushed
the
featrure/edit-webhooks
branch
from
4b90323 to
a58d340
Compare
# Conflicts: # core/src/main/kotlin/behavior/MessageBehavior.kt # rest/src/main/kotlin/builder/message/MessageModifyBuilder.kt # rest/src/main/kotlin/json/request/MessageRequests.kt # rest/src/main/kotlin/json/request/WebhookRequests.kt # rest/src/main/kotlin/route/Route.kt # rest/src/main/kotlin/service/WebhookService.kt
Relates to #121
Adds: discord/discord-api-docs#2243