matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.79k stars 2.13k forks source link

User admin API returns integers instead of booleans on SQLite, or the documentation is incorrect #13519

Open squahtx opened 2 years ago

squahtx commented 2 years ago

Similar to #13505. Originally reported by @jplatte in https://github.com/matrix-org/synapse/pull/13509#issuecomment-1212916888:

Description

The first example on https://matrix-org.github.io/synapse/develop/admin_api/user_admin_api.html shows this:

    "is_guest": 0,
    "admin": 0,
    "deactivated": 0,
    "shadow_banned": 0,

but some of these fields are mentioned as being bool further down.

dklimpel commented 2 years ago

IMO, in this case, it has not the reason in SQLite, but the database layout: https://github.com/matrix-org/synapse/blob/d4d3249ded000219a4f875943632c3d0f928d58d/synapse/storage/schema/main/full_schemas/54/full.sql.postgres#L1091-L1102 https://github.com/matrix-org/synapse/blob/25f43faa70f7cc58493b636c2702ae63395779dc/synapse/storage/schema/main/delta/58/09shadow_ban.sql#L18

For some reason most of them are not boolean but smallint.

As a result, it is not consistent that rooms use booleanand users use int.

DMRobertson commented 2 years ago

I'd guess we chose smallint because it's something small that both sqlite and postgres support, unlike boolean.

dklimpel commented 2 years ago

I'd guess we chose smallint because it's something small that both sqlite and postgres support, unlike boolean.

Thats right, sqlite does not have a boolean. I am confused now if I see that (is_admin BOOLEAN):

https://github.com/matrix-org/synapse/blob/d4d3249ded000219a4f875943632c3d0f928d58d/synapse/storage/schema/main/full_schemas/54/full.sql.sqlite#L153

and (admin SMALLINT): https://github.com/matrix-org/synapse/blob/d4d3249ded000219a4f875943632c3d0f928d58d/synapse/storage/schema/main/full_schemas/54/full.sql.sqlite#L6

And SQLite uses boolean in an example:

CREATE TABLE person(
  person_id       INTEGER PRIMARY KEY,
  team_id         INTEGER REFERENCES team,
  is_team_leader  BOOLEAN,
  -- other fields elided
);

Presumably the inconsistent and non boolean values are then part of the past. Is it a task to convert all these columns to boolean? But ALTER TABLE is not easy in SQLite.

clokep commented 2 years ago

Presumably the inconsistent and non boolean values are then part of the past.

I believe old SQLites didn't support booleans and there might still be some issues using them directly in queries (if you pass them in as parameters than it works OK though).

Not sure how hard it would be to convert, but is likely doable.

DMRobertson commented 2 years ago

I believe old SQLites didn't support booleans and there might still be some issues using them directly in queries (if you pass them in as parameters than it works OK though).

My understanding, reading https://www.sqlite.org/datatype3.html#boolean_datatype and the preceeding section is:

And SQLite uses boolean in an example:

Given the last bullet above, I would guess that a) this is just a hint to the reader and b) BOOLEAN treated as a synonym for INTEGER by SQLite. (But I really am just guessing.)