sdk: Allow to update notification settings #2135

nimau · 2023-06-22T14:45:28Z

This PR is the second step of the #1959 implementation.

It adds a new notification_settings crate in matrix-sdk to get and define notification settings for a room and enable/disable push rules.

The pub functions will be used by the ffi layer in a future PR.

codecov · 2023-06-22T14:59:20Z

Codecov Report

Patch coverage: 94.41% and project coverage change: +0.13 🎉

Comparison is base (7fd5068) 76.11% compared to head (916b693) 76.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2135      +/-   ##
==========================================
+ Coverage   76.11%   76.25%   +0.13%     
==========================================
  Files         161      164       +3     
  Lines       17202    17330     +128     
==========================================
+ Hits        13094    13215     +121     
- Misses       4108     4115       +7

Impacted Files	Coverage Δ
crates/matrix-sdk/src/error.rs	`62.50% <0.00%> (+1.38%)`	⬆️
crates/matrix-sdk/src/lib.rs	`100.00% <ø> (ø)`
...es/matrix-sdk/src/notification_settings/command.rs	`88.88% <88.88%> (ø)`
crates/matrix-sdk/src/notification_settings/mod.rs	`93.91% <93.91%> (ø)`
crates/matrix-sdk/src/client/mod.rs	`80.47% <100.00%> (+0.20%)`	⬆️
...rix-sdk/src/notification_settings/rule_commands.rs	`100.00% <100.00%> (ø)`
...ates/matrix-sdk/src/notification_settings/rules.rs	`97.43% <100.00%> (+1.11%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

bnjbvr

Nice! There's a potential data race issue in there, and we ought to simplify the insert_rule function. This is close to merge 💪

bnjbvr · 2023-06-26T13:08:45Z

crates/matrix-sdk/src/notification_settings/rules.rs

+        kind: RuleKind,
+        room_id: &RoomId,
+        notify: bool,
+        delete_other_custom_rules: bool,


It's not clear to me why it's this function's responsibility to delete other custom rules: it has nothing to do with the name, and it seems the caller has the possibility of calling another method, delete_rules, to do just that. Could we let the caller handle that, instead? It would make for a smaller API surface, fewer things to test, etc.

(The caller can even do better: 1. first remove all the custom rules, if needs be, 2. insert the new rule afterwards; that would not require the exception in the delete_rules call)

Oh yes, good point. It makes things much simpler, we just have to keep executing the deletions after the insertions (otherwise the mode won' be correct on the next sync)

crates/matrix-sdk/src/notification_settings/mod.rs

bnjbvr · 2023-06-26T13:15:48Z

crates/matrix-sdk/src/notification_settings/mod.rs

+    /// Replace the internal ruleset
+    ///
+    /// # Arguments
+    ///
+    /// * `ruleset` - A `Ruleset` containing account's owner push rules
+    async fn set_ruleset(&self, ruleset: &Ruleset) {
+        *self.rules.write().await = Rules::new(ruleset.clone())
+    }
+
+    /// Get a new `Rules` instance to interact with the ruleset.
+    async fn rules(&self) -> Rules {
+        let rules = &*self.rules.read().await;
+        rules.clone()
+    }


These two methods hide the fact that it's hidden behind a lock, and they may cause confusing issues, looking at how they're used:

get the ruleset

do something with it locally, start requests to the server

replace the internal ruleset

Since only operations 1 and 3 are holding onto the lock, there might be other operations holding onto the lock in the middle. Namely, a new PushRulesEvent may arrive and reset the ruleset (invalidating it in the meanwhile), or another independent request might be interleaved.

Depending on the answer to the above question (are our requests validated by receiving a new PushRulesEvent), maybe we don't even need to write to the local push rules. Otherwise, for operations that do read/request/writes, we need another solution:

taking the write lock throughout the entire request would be a bit sad, but safe

we could get the read-only Rules, then send the request, and after it's done apply a "diff" corresponding to the Command to the local Rules (or, equivalently, rerun the local Rules functions on the self.rules.write().await after the server has responded)

I wonder if the server responds with a partial/updated list of push rules? In that case, we could consider the server's response is the one source of truth

Ok, so as requested, I modified the following:

I removed the 2 functions that were hiding the lock.

I modified the flow used to update the ruleset. Now, every time we want to update the ruleset, we first build a list of commands that reflect the change, then we execute them to update the HS and finally, we update the local ruleset by applying the commands on the current ruleset. Applying a command can fail silently if the current ruleset has changed between the time we build the list of commands and the time we apply them to the ruleset.

bnjbvr · 2023-06-26T13:18:48Z

crates/matrix-sdk/src/notification_settings/mod.rs

+    }
+
+    /// Get a new `Rules` instance to interact with the ruleset.
+    async fn rules(&self) -> Rules {


Could this return a RwLockReadGuard<'_, Rules>, instead? So it's the caller's responsibility to decide whether to clone or not, depending on its needs (I suspect most of the time it's not required to clone).

Yes, good point. I've updated it.

crates/matrix-sdk/src/notification_settings/mod.rs

bnjbvr

We're getting there!

bnjbvr · 2023-06-30T13:05:44Z

crates/matrix-sdk/src/notification_settings/rules.rs

-    pub(crate) fn insert_room_rule(
-        &mut self,
+    /// Build a command to insert a push rule
+    pub(crate) fn build_insert_rule_command(


Ok, so first I thought all the _command methods don't really use self (directly or indirectly), and that suggests a slightly different code structuring here.

But when making a delete command, we shouldn't just let pass through an error where we're trying to remove a rule that doesn't exist. So it could be useful to keep a look at the current rules.

So we can go one of two ways:

Either rename all these methods prepare_{action}_command, but have them do real validation.

Or having a new struct RuleCommands<'a> { commands: Vec<Command>, rules: &'a Ruleset } that has many helper methods insert_rule, etc., and it's independent from the Rules? Then we change the signature of Rules::apply so it takes a RuleCommands parameter. Commands can be tested separately that way.

Ok, I've chosen option 2, the RuleCommands struct simplifies the insertion / deletion of the rules.

crates/matrix-sdk/src/notification_settings/rules.rs

bnjbvr · 2023-06-30T13:11:24Z

crates/matrix-sdk/src/notification_settings/rules.rs

+    ///
+    /// The commands may silently fail because the ruleset may have changed
+    /// between the time the commands were created and the time it is applied.
+    pub(crate) fn apply_commands(&mut self, commands: &[Command]) {


This should take ownership of the Vec<Command>, unless we have a (non-test only) reason to reuse them after applying.

crates/matrix-sdk/src/notification_settings/mod.rs

bnjbvr · 2023-06-30T13:21:45Z

crates/matrix-sdk/src/notification_settings/mod.rs

+        room_id: &RoomId,
+        mode: RoomNotificationMode,
+    ) -> Result<(), NotificationSettingsError> {
+        let rules = self.rules.read().await.clone();


bnjbvr · 2023-06-30T13:25:27Z

crates/matrix-sdk/src/notification_settings/mod.rs

+        let custom_rules = self.get_custom_rules_for_room(room_id).await;
+
+        let rules = self.rules.read().await.clone();


The read lock is implicitly taken in get_custom_rules_for_room, could we take it only once? (inlining the content of get_custom_rules_for_room here)

(also make sure to release it before the execute_commands below, to avoid locking for too long!)

Okay, I've updated the code so that it waits for a lock as little as possible.

bnjbvr · 2023-06-30T13:31:02Z

crates/matrix-sdk/src/notification_settings/mod.rs

+        let mut rules = notification_settings.rules.read().await.clone();
+        _ = insert_room_rule(&mut rules, RuleKind::Room, &room_id, true).unwrap();
+        update_rules(&notification_settings, &rules).await;


Instead of this pattern, could we statically declare the rules manually in a Vec and create a NotificationSettings from that, without using other orthogonal features? Otherwise, we lose a bit of control over the things we test and thus we might be using broken code (in insert_room_rule and others) while not realizing it.

bnjbvr · 2023-06-30T13:31:41Z

crates/matrix-sdk/src/notification_settings/mod.rs

+        let mut rules = notification_settings.rules.read().await.clone();
+        // Set a notifying `Room` rule into the ruleset to be in `AllMessages`
+        _ = insert_room_rule(&mut rules, RuleKind::Room, &room_id, true).unwrap();
+        update_rules(&notification_settings, &rules).await;


ditto everywhere in this file

…notify actions; use clone() where it's more obvious and use explicit variants so we know we have to consider this match if we were to add a new variant later

bnjbvr

Thanks. So as to help with merging this, I'll push my own review comments as commits to this branch, and you can inspect them to see what I've changed. In particular, there are a few review comments inline as XXX comments, which I think deserve clarifications as follow-up PRs (which should either change the code, or at least remove the XXX comments 😁).

sdk: Allow to update notification settings

5930bdb

nimau requested a review from a team as a code owner June 22, 2023 14:45

nimau requested review from bnjbvr and removed request for a team June 22, 2023 14:45

Add an event listener for push rule events

f2f8292

bnjbvr requested changes Jun 26, 2023

View reviewed changes

nimau added 5 commits June 26, 2023 16:43

Fix: simplify insertion and deletion of rules

6d2e04b

Fix: Limit rules cloning

8e945d0

Fix: Unit tests

6fbbb0a

Fix: set_room_notification_mode

1f53988

Fix: potential race condition when updating the local ruleset

6d91fa6

nimau requested a review from bnjbvr June 27, 2023 16:39

bnjbvr requested changes Jun 30, 2023

View reviewed changes

nimau added 3 commits July 3, 2023 15:22

Refactor RuleCommands

efd296c

RuleCommands Unit Tests

4aefb81

Fix: limit lock usage

9ed6a4e

nimau requested a review from bnjbvr July 3, 2023 15:18

bnjbvr added 9 commits July 4, 2023 15:44

nit: use expression assignment by default

dcc759e

nit: pass Ruleset by ownership in RuleCommands' ctor

5005379

a few nits: use to_owned() for &str -> String; use free function for …

176048c

…notify actions; use clone() where it's more obvious and use explicit variants so we know we have to consider this match if we were to add a new variant later

nit: rename RuleCommands::set_enabled to set_enabled_internal

09c5709

nit: test modules don't need to be publicized

b266030

tidy tests

a444544

nit: pass an owned RuleCommands in Rules::apply

1be2fd8

add comments/questions in Rules tests

67d00ef

nit: no need to publicize the tests module

63053f0

bnjbvr reviewed Jul 4, 2023

View reviewed changes

bnjbvr added 2 commits July 4, 2023 17:07

tweak NotificationSettings tests too

f8bd91a

rustfmt

916b693

bnjbvr approved these changes Jul 4, 2023

View reviewed changes

bnjbvr enabled auto-merge (squash) July 4, 2023 15:15

bnjbvr merged commit 3db7210 into main Jul 4, 2023

bnjbvr deleted the nicolas/sdk_notification_settings branch July 4, 2023 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: Allow to update notification settings #2135

sdk: Allow to update notification settings #2135

nimau commented Jun 22, 2023

codecov bot commented Jun 22, 2023 •

edited

Loading

bnjbvr left a comment

bnjbvr Jun 26, 2023

nimau Jun 26, 2023

bnjbvr Jun 26, 2023

nimau Jun 27, 2023

bnjbvr Jun 26, 2023

nimau Jun 26, 2023

bnjbvr left a comment

bnjbvr Jun 30, 2023

nimau Jul 3, 2023

bnjbvr Jun 30, 2023

bnjbvr Jun 30, 2023

bnjbvr Jun 30, 2023

nimau Jul 3, 2023

bnjbvr Jun 30, 2023

nimau Jul 3, 2023

bnjbvr Jun 30, 2023

bnjbvr left a comment

		let custom_rules = self.get_custom_rules_for_room(room_id).await;

		let rules = self.rules.read().await.clone();

sdk: Allow to update notification settings #2135

sdk: Allow to update notification settings #2135

Conversation

nimau commented Jun 22, 2023

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 22, 2023 •

edited

Loading