Delayed event management: split endpoints, no auth #5066
Conversation
Add dedicated endpoints for each of the cancel/restart/send actions for updating a delayed event, and make them unauthenticated. Also keep support for the original endpoint where the update action is in the request body, and make the split-endpoint versions fall back to it if they are unsupported by the homeserver.
as TypeDoc doesn't support that
| * @throws A M_NOT_FOUND error if no matching delayed event could be found. | ||
| */ | ||
| // eslint-disable-next-line | ||
| public async _unstable_sendScheduledDelayedEvent( |
There was a problem hiding this comment.
Note that the reason these new methods are named "*ScheduledDelayedEvent" is because this method would otherwise be named _unstable_sendDelayedEvent, which would clash with the existing method for the PUT /send/{eventType}/{txnId}?delay={ms} endpoint. "Scheduled" was put in this new method's name avoid that clash, and the cancel/restart method names have it too for the sake of consistency.
Alternatively, the existing _unstable_sendDelayedEvent could be renamed to _unstable_scheduleDelayedEvent, so that "schedule" would mean "prepare an event for delayed delivery" and "send" would mean "immediately send a delayed event now instead of waiting for its scheduled delivery time".
There was a problem hiding this comment.
I wonder if the word immediately needs to appear somewhere, the scheduled in this function name doesn't suggest it's an immediate action until I check the docstring.
|
Note that this now pulls in matrix-org/matrix-widget-api#143, so that the split/unauthed endpoints are also invoked when using the widget API (meaning they'll be used by embedded Element Call). |
Half-Shot
left a comment
Half-Shot
left a comment
There was a problem hiding this comment.
Starting to get there, few things that need shaking out
| * @throws A M_NOT_FOUND error if no matching delayed event could be found. | ||
| */ | ||
| // eslint-disable-next-line | ||
| public async _unstable_sendScheduledDelayedEvent( |
There was a problem hiding this comment.
I wonder if the word immediately needs to appear somewhere, the scheduled in this function name doesn't suggest it's an immediate action until I check the docstring.
Co-authored-by: Will Hunt <[email protected]>
Keep supporting it to not break widgets that currently use it. Also add back the test for it.
| * @experimental This currently relies on an unstable MSC (MSC4140). | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| public async _unstable_cancelScheduledDelayedEvent(delayId: string): Promise<EmptyObject> { |
There was a problem hiding this comment.
Is there a reasn for this to return an empty object or is it copying sendToDevice? (not sure why that does either)
There was a problem hiding this comment.
This method is for handling a CS-API endpoint, so returning an empty object is for having a minimal response payload/body. This is done for other endpoints too (including sendToDevice)
There was a problem hiding this comment.
Still not sure why it needs to return anything at all though?
There was a problem hiding this comment.
These methods of the embedded client override ones from MatrixClient, so they have to return {} to fit the return signature of Promise<EmptyObject> from the overridden methods. And the reason the MatrixClient methods have such a signature is to follow the pattern of its other methods doing so when they wrap a CS-API endpoint that the server responds to with an empty JSON object.
I suppose the reason for that pattern is to handle the case of a server adding non-standard keys to a response body that's specced as empty.
There was a problem hiding this comment.
Mmm, I find it very weird that an API would relay through the empty object to the app: letting the app dig data out of a response object that might not be empty seems fairly awful, but you're right, this does seem to be the pattern that the js-sdk uses, by and large, so I guess consistency ftw.
because `return await promise` is at least as fast as `return promise`
Add dedicated endpoints for each of the cancel/restart/send actions for updating a delayed event, and make them unauthenticated.
Also keep support for the original endpoint where the update action is in the request body, and make the split-endpoint versions fall back to it if they are unsupported by the homeserver.
Checklist
public/exportedsymbols have accurate TSDoc documentation.