/sync response's account_data includes an m.push_rules event which doesn't include required room.default attribute

[wavexx]

Description

Using a current version matrix-nio on the default matrix.org homeserver I experience the following: matrix-nio/matrix-nio#348

The request: https://matrix.org/_matrix/client/r0/sync?access_token=<snip>&since=s3281165653_757284974_12692804_1597914399_1620552403_3573198_592315752_5209106983_0&full_state=true&timeout=0&filter=%7B%22room%22%3A%7B%22timeline%22%3A%7B%22limit%22%3A30%7D%2C%22state%22%3A%7B%22lazy_load_members%22%3Atrue%7D%7D%7D'

produces the following json:

{
  "next_batch": "s3281181518_757284974_12713739_1597925487_1620566333_3573208_592321297_5209124871_0",
  "account_data": {
    "events": [
      {
        "type": "m.push_rules",
        "content": {
          "global": {
            "room": [
              {
                "actions": [
                  "dont_notify"
                ],
                "rule_id": "!<roomid>:matrix.org",
                "enabled": true
              }
            ],

According to https://matrix.org/docs/api/#get-/_matrix/client/v3/pushrules/ the room.default parameter is required, but is not included in the "room" data.

Steps to reproduce

• After a login request, perform a /sync request like the folling https://matrix.org/_matrix/client/r0/sync?access_token=<snip>&since=s3281165653_757284974_12692804_1597914399_1620552403_3573198_592315752_5209106983_0&full_state=true&timeout=0&filter=%7B%22room%22%3A%7B%22timeline%22%3A%7B%22limit%22%3A30%7D%2C%22state%22%3A%7B%22lazy_load_members%22%3Atrue%7D%7D%7D'
• Note that the "default" parameter is not included in the "room" push_rules

Homeserver

matrix.org

Synapse Version

{"server":{"name":"Synapse","version":"1.67.0 (b=matrix-org-hotfixes,3a5edde170)"}}

Installation Method

No response

Platform

n/a

Relevant log output

n/a

Anything else that would be useful to know?

No response

[DMRobertson]

I've marked this as tolerable because I'm assuming that this doesn't cause end-user pain. Please correct me if that's false.

The relevant source is

synapse/synapse/handlers/sync.py

Lines 415 to 419 in 69fa297

	async def push_rules_for_user(self, user: UserID) -> Dict[str, Dict[str, list]]:
	user_id = user.to_string()
	rules_raw = await self.store.get_push_rules_for_user(user_id)
	rules = format_push_rules_for_user(user, rules_raw)
	return rules

I don't understand push rules well enough to know if it's easy to compute the default attribute at read time (above) or if we need to also write something to the DB before this. @erikjohnston might know more?

[wavexx]

Depends on the end-user pain level ;).

This breaks any client which is currently using the python matrix-nio module, since it does validate the response with a schema. This used to validate correctly until maybe a couple of months ago. I've personally found the issue when using weechat-matrix.

We'll likely relax the schema validation, but it will break old clients.

[DMRobertson]

This used to validate correctly until maybe a couple of months ago.

That's very interesting! Are there any clues which could help us narrow this down to a Synapse version?

[wavexx]

Honestly no. I was hoping this issue to "fix itself" patiently, but it didn't happen ;)
I'm just a casual contributor.

Looking at the schema, this could also be the result of the global.room key not existing or being empty before.

[richvdh]

According to #13884 this was introduced in #13522

[nico-famedly]

This issue breaks our SDK, which uses the openapi spec to generate the pushrule classes (where this is marked as required). This means when using custom push rules, we can't process push rules properly anymore nor can we properly modify them anymore. While we can work around this by patching the openapi spec before generating our code, this means that some of our deployments are currently experiencing this regression and can't use their custom pushrules anymore via our clients. It would be nicer to have it fixed either in the spec or in synapse, so that we don't have to carry a patch.

[richvdh]

It would be nicer to have it fixed either in the spec or in synapse, so that we don't have to carry a patch.

I don't think this was a deliberate change; it should be fixed in synapse rather than the spec.