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.

Sign up for GitHub

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

Fix merging of default push rules #4135

Merged
merged 5 commits into from
Mar 28, 2024
Merged

Fix merging of default push rules #4135

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ed082f
Fix merging of default push rules
t3chguy Mar 28, 2024
32ddf28
Iterate
t3chguy Mar 28, 2024
0f25429
Iterate
t3chguy Mar 28, 2024
06e0838
Iterate
t3chguy Mar 28, 2024
467b49a
Add test
t3chguy Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 133 additions & 5 deletions spec/unit/pushprocessor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
import * as utils from "../test-utils/test-utils";
import { IActionsObject, PushProcessor } from "../../src/pushprocessor";
import { ConditionKind, EventType, IContent, MatrixClient, MatrixEvent, PushRuleActionName, RuleId } from "../../src";
import {
ConditionKind,
EventType,
IContent,
IPushRule,
MatrixClient,
MatrixEvent,
PushRuleActionName,
RuleId,
TweakName,
} from "../../src";
import { mockClientMethodsUser } from "../test-utils/client";

describe("NotificationService", function () {
Expand All @@ -12,21 +22,21 @@ describe("NotificationService", function () {

let pushProcessor: PushProcessor;

const msc3914RoomCallRule = {
const msc3914RoomCallRule: IPushRule = {
rule_id: ".org.matrix.msc3914.rule.room.call",
default: true,
enabled: true,
conditions: [
{
kind: "event_match",
kind: ConditionKind.EventMatch,
Copy link
Member

Choose a reason for hiding this comment

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

ftr I am generally deeply ambivalent about using symbolic constants in test code: using the literal gives us a chance to check that the constant is doing what we expect.

(This brought to you on the back of a Synapse bug where we had a test which used what we thought was a constant but turned out not to be.)

YMMV though, not a particular request to change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

ftr I am generally deeply ambivalent about using symbolic constants in test code: using the literal gives us a chance to check that the constant is doing what we expect.

Its preferable to @ts-ignore.

image

Copy link
Member

Choose a reason for hiding this comment

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

I mean I'd argue your problem here is forcing msc3914RoomCallRule to be an IPushRule, but whatever

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if I remove that type cast there, then I'd get the failure in the call to rewritePushRules

The only reason it was fine before this PR is because we passed matrixClient.pushRules! to rewritePushRules and had
image to make TS happy with the types of pushRules within the mock matrixClient

Copy link
Member Author

Choose a reason for hiding this comment

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

image

key: "type",
pattern: "org.matrix.msc3401.call",
},
{
kind: "call_started",
kind: ConditionKind.CallStarted,
},
],
actions: ["notify", { set_tweak: "sound", value: "default" }],
actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Sound, value: "default" }],
};

let matrixClient: MatrixClient;
Expand Down Expand Up @@ -188,6 +198,108 @@ describe("NotificationService", function () {
});

it("should add default rules in the correct order", () => {
matrixClient.pushRules = PushProcessor.rewriteDefaultRules({
richvdh marked this conversation as resolved.
Show resolved Hide resolved
device: {},
global: {
content: [],
override: [
{
rule_id: ".m.rule.master",
default: true,
enabled: false,
conditions: [],
actions: [],
},
{
actions: [
PushRuleActionName.Notify,
{
set_tweak: TweakName.Sound,
value: "default",
},
{
set_tweak: TweakName.Highlight,
},
],
enabled: true,
pattern: "coffee",
rule_id: "coffee",
default: false,
},
{
actions: [
PushRuleActionName.Notify,
{
set_tweak: TweakName.Sound,
value: "default",
},
{
set_tweak: TweakName.Highlight,
},
],
conditions: [
{
kind: ConditionKind.ContainsDisplayName,
},
],
enabled: true,
default: true,
rule_id: ".m.rule.contains_display_name",
},
{
actions: [
PushRuleActionName.Notify,
{
set_tweak: TweakName.Sound,
value: "default",
},
],
conditions: [
{
is: "2",
kind: ConditionKind.RoomMemberCount,
},
],
enabled: true,
rule_id: ".m.rule.room_one_to_one",
default: true,
},
],
room: [],
sender: [],
underride: [
{
actions: [
PushRuleActionName.Notify,
{
set_tweak: TweakName.Highlight,
value: false,
},
],
conditions: [],
enabled: true,
rule_id: "user-defined",
default: false,
},
msc3914RoomCallRule,
{
actions: [
PushRuleActionName.Notify,
{
set_tweak: TweakName.Highlight,
value: false,
},
],
conditions: [],
enabled: true,
rule_id: ".m.rule.fallback",
default: true,
},
],
},
});
pushProcessor = new PushProcessor(matrixClient);

// By the time we get here, we expect the PushProcessor to have merged the new .m.rule.is_room_mention rule into the existing list of rules.
// Check that has happened, and that it is in the right place.
const containsDisplayNameRuleIdx = matrixClient.pushRules?.global.override?.findIndex(
Expand All @@ -205,6 +317,22 @@ describe("NotificationService", function () {

expect(containsDisplayNameRuleIdx).toBeLessThan(isRoomMentionRuleIdx!);
expect(isRoomMentionRuleIdx).toBeLessThan(mReactionRuleIdx!);

expect(matrixClient.pushRules?.global.override?.map((r) => r.rule_id)).toEqual([
".m.rule.master",
"coffee",
".m.rule.contains_display_name",
".m.rule.room_one_to_one",
".m.rule.is_room_mention",
".m.rule.reaction",
".org.matrix.msc3786.rule.room.server_acl",
]);
expect(matrixClient.pushRules?.global.underride?.map((r) => r.rule_id)).toEqual([
"user-defined",
".org.matrix.msc3914.rule.room.call",
// Assert that unknown default rules are maintained
".m.rule.fallback",
]);
});

// User IDs
Expand Down
45 changes: 31 additions & 14 deletions src/pushprocessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ import {
ICallStartedPrefixCondition,
IContainsDisplayNameCondition,
IEventMatchCondition,
IEventPropertyIsCondition,
IEventPropertyContainsCondition,
IEventPropertyIsCondition,
IPushRule,
IPushRules,
IRoomMemberCountCondition,
Expand All @@ -49,6 +49,10 @@ const RULEKINDS_IN_ORDER = [
PushRuleKind.Underride,
];

const UserDefinedRules = Symbol("UserDefinedRules");
t3chguy marked this conversation as resolved.
Show resolved Hide resolved

type OrderedRules = Array<string | typeof UserDefinedRules>;
t3chguy marked this conversation as resolved.
Show resolved Hide resolved

// The default override rules to apply to the push rules that arrive from the server.
// We do this for two reasons:
// 1. Synapse is unlikely to send us the push rule in an incremental sync - see
Expand Down Expand Up @@ -115,8 +119,9 @@ const DEFAULT_OVERRIDE_RULES: Record<string, IPushRule> = {
},
};

const EXPECTED_DEFAULT_OVERRIDE_RULE_IDS = [
const EXPECTED_DEFAULT_OVERRIDE_RULE_IDS: OrderedRules = [
RuleId.Master,
UserDefinedRules,
RuleId.SuppressNotices,
RuleId.InviteToSelf,
RuleId.MemberEvent,
Expand Down Expand Up @@ -151,7 +156,8 @@ const DEFAULT_UNDERRIDE_RULES: Record<string, IPushRule> = {
},
};

const EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS = [
const EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS: OrderedRules = [
UserDefinedRules,
RuleId.IncomingCall,
RuleId.EncryptedDM,
RuleId.DM,
Expand All @@ -162,24 +168,31 @@ const EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS = [
/**
* Make sure that each of the rules listed in `defaultRuleIds` is listed in the given set of push rules.
*
* @param kind - the kind of push rule set being merged.
* @param incomingRules - the existing set of known push rules for the user.
* @param defaultRules - a lookup table for the default definitions of push rules.
* @param defaultRuleIds - the IDs of the expected default push rules, in order.
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
*
* @returns A copy of `incomingRules`, with any missing default rules inserted in the right place.
*/
function mergeRulesWithDefaults(
kind: PushRuleKind,
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
incomingRules: IPushRule[],
defaultRules: Record<string, IPushRule>,
defaultRuleIds: string[],
defaultRuleIds: OrderedRules,
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
): IPushRule[] {
// Calculate the index after the last default rule in `incomingRules`
// to allow us to split the incomingRules into defaults and custom
let firstCustomRuleIndex = incomingRules.findIndex((r) => !r.default);
if (firstCustomRuleIndex < 0) firstCustomRuleIndex = incomingRules.length;

function insertDefaultPushRule(ruleId: string): void {
if (ruleId in defaultRules) {
// Find the indices of the edges of the user-defined rules in the incoming rules
const incomingRulesEnabled = incomingRules.map((rule) => rule.enabled);
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
const userDefinedRulesRange: [number, number] = [
incomingRulesEnabled.indexOf(false),
incomingRulesEnabled.lastIndexOf(false),
];
Copy link
Member

Choose a reason for hiding this comment

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

we're actually doing quite a lot of iterating and copying of incomingRules. I'd instead start just by partitioning them:

// Split the incomingRules into defaults and custom
const incomingDefaultRules = incomingRules.filter((rule) => rule.default);
const incomingCustomRules = incomingRules.filter((rule) => !rule.default);

... and I think everything else can then be more easily written in terms of those, instead of incomingRules.slice.

Not a blocker, just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Splitting them as per your example would mean you lose track of the location of the user-defined rules. Right now only .m.rule.master precedes them but the server could send other default rules as they are added in that position. This codepath is only hit once for each incoming m.push_rules event down /sync, those should be quite rare

Copy link
Member

Choose a reason for hiding this comment

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

I don't really follow. We insert the custom rules into newRules just before copying RuleId.SuppressNotices either way?

This codepath is only hit once for each incoming m.push_rules event down /sync, those should be quite rare

Yeah, I'm arguing this rather more from the PoV of clarity than performance. Just saying: it's not like doing the indexOfs here rather than copying them is saving you much.

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 don't really follow. We insert the custom rules into newRules just before copying RuleId.SuppressNotices either way?

incomingRules is in the order of:

some default rules (currently just master)
user defined rules
other default rules

Your proposed code would merge the default rules into one list and the default orders wouldn't account for any rules in the first bucket other than master so the order of any additional rules specified by the server there in later spec versions go into the wrong spot

// Split the incomingRules into defaults and custom
const incomingDefaultRules = incomingRules.filter((rule) => rule.default);
const incomingCustomRules = incomingRules.filter((rule) => !rule.default);

Copy link
Member

Choose a reason for hiding this comment

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

Your proposed code would merge the default rules into one list and the default orders wouldn't account for any rules in the first bucket other than master so the order of any additional rules specified by the server there in later spec versions go into the wrong spot

I don't think so. In such a scenario, the new rule would end up as the second entry in incomingDefaultRules (after master, and before suppress_notices). Assuming we change the loop below to iterate over incomingDefaultRules instead of the current pair of slices of incomingRules:

  • We'll see master; it will match the expectation, and we will copy it over.
  • We'll see the hypothesized new rule; it is unrecognized, and so we will copy it over.
  • We'll see suppress_notices. Its index in orderedRuleIds is higher than the expectation, so first we copy over the custom rules. Then we carry on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making the proposed change the test fails as follows:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this was due to missing ".org.matrix.msc3914.rule.room.call" in the expected defaults in the last PR


function insertDefaultPushRule(ruleId: OrderedRules[number]): void {
if (ruleId === UserDefinedRules) {
// Re-insert any user-defined rules that were in `incomingRules`
newRules.push(...incomingRules.slice(...userDefinedRulesRange));
} else if (ruleId in defaultRules) {
logger.warn(`Adding default global push rule ${ruleId}`);
newRules.push(defaultRules[ruleId]);
} else {
Expand All @@ -189,7 +202,11 @@ function mergeRulesWithDefaults(

let nextExpectedRuleIdIndex = 0;
const newRules: IPushRule[] = [];
for (const rule of incomingRules.slice(0, firstCustomRuleIndex)) {
// Process the default rules by merging them with defaults
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
for (const rule of [
...incomingRules.slice(0, userDefinedRulesRange[0]),
...incomingRules.slice(userDefinedRulesRange[1]),
]) {
const ruleIndex = defaultRuleIds.indexOf(rule.rule_id);
if (ruleIndex === -1) {
// an unrecognised rule; copy it over
Expand All @@ -212,8 +229,6 @@ function mergeRulesWithDefaults(
insertDefaultPushRule(ruleId);
}

// Finally any non-default rules that were in `incomingRules`
newRules.push(...incomingRules.slice(firstCustomRuleIndex));
return newRules;
}

Expand Down Expand Up @@ -281,12 +296,14 @@ export class PushProcessor {

// Merge the client-level defaults with the ones from the server
newRules.global.override = mergeRulesWithDefaults(
PushRuleKind.Override,
newRules.global.override,
DEFAULT_OVERRIDE_RULES,
EXPECTED_DEFAULT_OVERRIDE_RULE_IDS,
);

newRules.global.underride = mergeRulesWithDefaults(
PushRuleKind.Underride,
newRules.global.underride,
DEFAULT_UNDERRIDE_RULES,
EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS,
Expand Down
Loading