Extension scoping doesn't correctly handle the case where only one field is provided

[DMRobertson]

matrix-org/matrix-spec-proposals@30e31c8 was my attempt to clarify the intended semantics of the extension scoping config.

The code as written today:

sliding-sync/sync3/extensions/extensions.go

Lines 99 to 127 in bca1b68

	// RoomInScope determines whether a given room ought to be processed by this extension,
	// according to the "core" extension scoping logic. Extensions are free to suppress
	// updates for a room based on additional criteria.
	func (r *Core) RoomInScope(roomID string, extCtx Context) bool {
	// If the extension hasn't had its scope configured, process everything.
	if r.Lists == nil && r.Rooms == nil {
	return true
	}
	// If this extension has been explicitly subscribed to this room, process the update.
	for _, roomInScope := range r.Rooms {
	if roomInScope == roomID {
	return true
	}
	}
	// If the room belongs to one of the lists that this extension should process, process the update.
	visibleInLists := extCtx.RoomIDsToLists[roomID]
	for _, visibleInList := range visibleInLists {
	for _, shouldProcessList := range r.Lists {
	if visibleInList == shouldProcessList {
	return true
	}
	}
	}
	// Otherwise ignore the update.
	return false
	}

If you scope to {"lists": ["a"]}, the extension will only process rooms visible in a and ⚠️ ignore those in your room subscriptions. It should process all rooms visible in a and all those in your room subscriptions.

Similarly if you scope to {"rooms": ["!foo:example"]}, the extension will only process that room and will ⚠️ ignore those visible in your sliding windows. It should indead process that room and all those in your sliding windows.

cc @bnjbvr

[bnjbvr]

Thanks for the clarification.

Would it make sense to have another special-case selector, that means "all the X that are handled in the request", so it's not cumbersome to express I want read receipts only for this one list, but read receipts for all the rooms that are in room_subscriptions? ("*" comes to mind, but it's not really "every single room/list", it's only those that belong to the request)

edit: I've been told that it's not required: setting a value to lists doesn't interfere with the settings in rooms, they're independent from each other 👍

[DMRobertson]

Hmm. I have found a problem. I have a test where:

• top level request defines two sliding windows A and B
• first sync requests typing notifs in list A and all room subs ({"lists": ["A"], "rooms": null})
• second sync requests typing notifs in some room R and all lists ({"lists": null, "rooms": ["R"]})
• the muxedReq treats "null" as "no change" and processes as-if the config was {"lists": ["A"], "rooms": ["R"]}

the latter is unfortunate.

Enumerating the cases the client can want to send, for e.g. lists:

1. "Make the extension apply to lists L1, L2, ..." -> send [L1, L2, ...].
2. "Use previous value" -> send null or omit the field
3. "Make the extension ignore all lists" -> send []
4. "Make the extension consider all lists" -> ???

Suggestion: I think I'd probably change the API to be that you either

• provide an explicit list of list keys to cover cases 1 and 3
• provide a special "all_lists": true field and no list keys for case 4
• provide neither for case 2 (which is the typical case)

and say that all_rooms: true wins if you provide both fields. Then all of that would have to be duplicated for lists too.

[bnjbvr]

"Use previous value" -> send null or omit the field

Instead of treating null as use the previous value, could we do the following:

• key is undefined, aka not present in the request JSON => use the previous value
• key is null => stop using it

That's pretty common in the Rust SDK's usage, for what it's worth: we treat null as a way to indicate we want to explicitly unset, vs don't set the field (aka it's undefined) to keep the previous value. Is this protocol common with the proxy server?

[DMRobertson]

The proxy, and to my understanding the protocol broadly treats undefined and null as one-and-the-same; it'd be fiddly to change that now.

Instead, the MSC was updated again to interpret any array starting with * to mean "all lists/room subscriptions as specifed in the main request" (i.e. point 4 in my previous comment). #149 implements this.