Historical string power level clarification

[neilalexander]

This PR aims to clarify and to define a reasonable standard for handling string power levels in room versions up to and including room version 9.

Related: matrix-org/matrix-spec#853, msc3667.

Preview: https://pr3688--matrix-org-previews.netlify.app

[ShadowJonathan]

One thing i need to ask, and mention, is what the range of this integer will be.

Synapse does not correctly validate a string-passed integer, allowing it to become bigger than 64-bit, or the canonical JSON 2^53 limit.

I personally think this latter bit should not be included, and integers should be validated according to the canonical JSON limit, but it is still worth mentioning. (synapse issue: matrix-org/synapse#11873)

[turt2live]

Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.

This is way out of my depth as a reviewer, so will defer to the server-side folk of the SCT for comment.

[Sorunome]

requiring bigints would significantly complicate server implementations, as in many languages using them isn't as simple as in python.

[deepbluev7]

I would not relax the limit on the integers. Our database adventures found not a single event outside the range. So it should be safe to not also relax the limit on the integer range. If it actually breaks rooms, we could add such language to the spec then?

[richvdh]

I would not relax the limit on the integers.

+1. This whole thing is a balance between not breaking too many existing rooms, whilst not making supporting those existing rooms too onerous for non-Synapse implementations.

Given that we don't know of any rooms which have stringy PLs outside [-2^53, 2^53), requiring non-synapse implementations to support such PLs seems an unnecessary burden.

[turt2live]

@deepbluev7 says:

I suggest we break rooms with PLs outside the range in v6+ and add an appropriate error message to synapse that says to report a bug, if an existing room is broken because of that. And then when someone comes and actually complains about it, we can discuss relaxing the limit as well. We don't need to allow all synapse currently allows, just what is already in the wild. And those are from my search only PLs inside the range, but as strings, in v6+. As long as it is clear how to report, if someone finds a non-conforming room, I think it is fine to break those temporarily.

[richvdh]

(in other words, let's change synapse to reject such PL events if it sees them in the future)

Edit: in room v6+, obviously. room v5 and earlier are kinda stuck with bigint PLs.

[clokep]

Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.

I don't think this is accurate. I just did some testing on Python 2:

>>> int("12345689012345678901234567890")
12345689012345678901234567890L
>>> type(_)
<type 'long'>

On python 3 this creates:

>>> int("12345689012345678901234567890")
12345689012345678901234567890
>>> type(_)
<class 'int'>

The difference is that Python 2 had a separate long class while Python 3 has an int class which expands to any size (essentially long from Python 2). json.dumps seems to work fine (and creates invalid JSON going over the 2 ^ 53 limit... for a number)

tl;dr I think both Python 2 & 3 didn't have a limit.

[turt2live]

Overall this is a sensible inclusion - comments are mostly regarding spec structure rather than the change itself.

[turt2live]

These shouldn't be promoted to headings - the bolding is fine

[turt2live]

We'll define a range when we pull this out of the s2s spec in favour of the room version specs (this section will get airlifted out of this doc).

For now, the following (or a variation therein) should be fine:

Suggested change

	Power level events in room versions up to and including room version 9
	can be optionally represented in string format, in order to maintain
	compatibility with pre-1.0 implementations without breaking existing rooms.
	A homeserver must be prepared to deal with this by parsing the power level
	from the string. In these cases, the following formatting of the power level
	string is allowed:
	In order to maintain backwards compatibility with early implementations,
	power levels can optionally be represented in string format instead of
	integer format. A homeserver must be prepared to deal with this by parsing
	the power level from a string. In these cases, the following formatting of the
	power level string is allowed:

[turt2live]

something missing from this paragraph is a pointer to what value we're talking about though. Examples after the grammar would probably be best, demonstrating a scenario or two (kick power level being "12" and a user having PL "42" or something similar).

[turt2live]

Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.

This is way out of my depth as a reviewer, so will defer to the server-side folk of the SCT for comment.

[turt2live]

Suggested change

	This behaviour is preserved strictly for backward compatibility only. A
	homeserver should take reasonable precautions to prevent users from
	uploading new power level events with string values and must never
	populate the default power levels in a room as string values.
	{{% boxes/warning %}}
	This behaviour is preserved strictly for backward compatibility only. A
	homeserver should take reasonable precautions to prevent users from
	sending new power level events with string values and must never
	populate the default power levels in a room as string values.
	{{% /boxes/warning %}}

[clokep]

I don't know if it is worth mentioning but.... room versions 1 to 5 inclusive can unfortunately have float power-levels that get cast to int as well in Synapse. See MSC2540.

Again from the Python docs:

For floating point numbers, this truncates towards zero.

😢

[richvdh]

@neilalexander are you able to make the requested changes?

It would certainly be great to get this clarified.

[neilalexander]

Replaced by matrix-org/matrix-spec#1082.