Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC3934: Bulk push rules change endpoint #3934

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Nov 15, 2022

@turt2live turt2live changed the title Bulk push rules change endpoint MSC3934: Bulk push rules change endpoint Nov 15, 2022
@turt2live turt2live marked this pull request as ready for review November 15, 2022 23:13
@turt2live turt2live added push proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Nov 15, 2022
Comment on lines +30 to +32
The body is simply a dictionary of rule ID to existing body of `/actions`. If any of the push rules
do not exist, none are changed and the server returns a 400 error to the client. If everything was
updated successfully, a 200 OK response with empty JSON object is returned.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify: for this and the endpoint below, the server treats it as a transaction: if one of the rules fails to update (due to not existing, DB error, etc) then all rules fail to update (a rollback).

error to the client. If everything was updated successfully, a 200 OK response with empty JSON
object is returned.

## Potential issues
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great improvement over what the spec allows today. However, one slight shortcoming is that clients would still need multiple calls if they wanted to update rules of different kinds. I wonder if that's a use case we should seek to support? E.g. I suppose we could move the kind parameter into the body of the request?

Copy link
Contributor

@Johennes Johennes Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along the same train of thought, this proposal also doesn't allow changing actions and enabled simultaneously. In both cases the client would be left with the multi-failure mode we're trying to fix. This almost makes me wonder if we'd need a PUT /_matrix/client/v1/pushrules_bulk?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat concerned about giving clients too much ability to cause performance issues on the server-side with large bulk endpoints (each push rules change triggers a sync loop poke). In theory, the client can isolate the actions and enabled calls safely, and encouraging them to do so would be a good thing imo.

Supporting bulk changes across different kinds is an interesting question though: it implies that one or more push rules might be at the wrong level and need redefining or reconsidering. I'm very interested in this sort of usecase :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm somewhat concerned about giving clients too much ability to cause performance issues on the server-side with large bulk endpoints (each push rules change triggers a sync loop poke).

Good point, yes. Could the /sync triggers be batched up on the server-side so that you only trigger a single time for any bulk request?

the client can isolate the actions and enabled calls safely, and encouraging them to do so would be a good thing imo

The Element iOS app uses an interesting logic here. When you toggle any of the global notification settings on or off, it issues two PUTs. One to /enable to ensure that the rule is actually on and one to /actions to toggle between notify and dont_notify. I'm not sure if other clients behave similarly but in this case there are two requests tied to a single UI control so isolating them would probably be tricky?

Supporting bulk changes across different kinds is an interesting question though: it implies that one or more push rules might be at the wrong level and need redefining or reconsidering. I'm very interested in this sort of usecase :)

Yeah, when thinking through it again, I'm actually failing to come up with a reasonable example. You're probably right that this should normally be taken care of by the priority hierarchy of the different rule kinds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only be a single sync trigger, but that's still a large trigger to happen: we're actively avoiding APIs that allow for arbitrarily large requests/responses, and an endpoint which is "too bulk" would ultimately cause excess data to be transmitted.

As for Element iOS: this sounds like a bug in the client. The client should have a cached copy of the push rules to be able to make decisions on, and the UI control would still be able to represent a failed state: setting actions on a disabled rule prepares it for when it can be enabled, meaning the client can use the enabled/disabled state to determine if something went wrong. A suggested approach would be to set the actions first (where if it fails the client has no-oped the user's request), then enable the rules. If the enable step fails, the UI control can show that easily, allowing the user to try again. With caching, the client can also determine if it needs to change the actions/enabled state of each individual rule it plans to affect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only be a single sync trigger, but that's still a large trigger to happen: we're actively avoiding APIs that allow for arbitrarily large requests/responses, and an endpoint which is "too bulk" would ultimately cause excess data to be transmitted.

Ah, got it. That makes sense. 👍

setting actions on a disabled rule prepares it for when it can be enabled, meaning the client can use the enabled/disabled state to determine if something went wrong

That's a great point! @alfogrillo we should make sure that the PUTs are issued this way around on Element iOS.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that:

  • Element Desktop: runs actions, then enabled on completion
  • Element iOS: runs actions and enabled concurrently 😅

code

@Johennes is it ok for you if I add a new ticket in our backlog for this (it's also worth checking Android)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, should hopefully be an easy fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants