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

Unclear override semantics for power_level_content_override #492

Open
joepie91 opened this issue Jun 23, 2019 · 4 comments
Open

Unclear override semantics for power_level_content_override #492

joepie91 opened this issue Jun 23, 2019 · 4 comments
Labels
A-Client-Server Issues affecting the CS API clarification An area where the expected behaviour is understood, but the spec could do with being more explicit

Comments

@joepie91
Copy link

From the spec:

power_level_content_override: The power level content to override in the default power level event. This object is applied on top of the generated m.room.power_levels event content prior to it being sent to the room. Defaults to overriding nothing.

It's unclear what the exact override semantics are here. Is the entire generated m.room.power_levels content discarded and replaced with the object specified here (ie. potentially removing keys)? Are they deep-merged? If yes, what are the exact merge semantics?

@turt2live turt2live added clarification An area where the expected behaviour is understood, but the spec could do with being more explicit A-Client-Server Issues affecting the CS API labels Jun 23, 2019
@davigp
Copy link

davigp commented Oct 6, 2021

I found this issue while trying to understand the same, docs are not very helpful. Would appreciate a clarification here 🙂

Seems the root powers are kept, but the events are overridden completely.

@richvdh richvdh transferred this issue from matrix-org/matrix-spec-proposals Mar 1, 2022
@andybalaam
Copy link
Member

Reading the Synapse code, what Synapse does is examine power_level_content_override, and for any key that exists inside that, replaces that entire key in the power levels.

So if I provide:

{
    "power_level_content_override": {
        "events": {
            "my.event.type": 0
        }
    }
}

Then the room's power levels will be the defaults, except that events will have been entirely replaced. For example:

{
    "ban": 50,
    "events": {
        "my.event.type": 0,
    },
    "events_default": 0,
    "historical": 100,
    "invite": 50,
    "kick": 50,
    "redact": 50,
    "state_default": 50,
    "users": {"@sid1:red": 100},
    "users_default": 0
}

@sumnerevans
Copy link
Contributor

I just ran into this confusion when writing a Complement test. I assumed it would deep-merge, and so the test assumes that.

It appears that Dendrite's implementation deep merges (and hence my test that assumes deep-merge passes on it). See the implementation in Dendrite here: https://github.com/matrix-org/dendrite/blob/main/clientapi/routing/createroom.go#L242-L252.

I suggest that we do one of two things:

  1. Explicitly state that it's a deep merge. (Probably doesn't require an MSC as it's just a spec clarification?)
  2. Remove the power_level_content_override option, instead opting for a power_level_content which forces users to explicitly specify every part of the m.room.power_levels content.

Right now, Synapse's behaviour is about the worst possible option because it's totally unclear that the override is only one level deep.

@cloudrac3r
Copy link
Contributor

I'm currently working around this by creating the room with no power level overrides and waiting for it to be created. Then I read the power levels state, do my own deep merge in my application code, then send that back as a new state event. Kind of like this code except I do it immediately after the room is created.

Explicitly state that it's a deep merge. (Probably doesn't require an MSC as it's just a spec clarification?)

As an appservice, I would much prefer to have this behaviour. The server's defaults are exactly what I want, I just want to modify one of the events on top of that. It would be easier if it was a deep merge because I could specify that event directly on room creation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Client-Server Issues affecting the CS API clarification An area where the expected behaviour is understood, but the spec could do with being more explicit
Projects
None yet
Development

No branches or pull requests

6 participants