Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

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

Open
squahtx opened this issue Aug 12, 2022 · 5 comments
Labels
A-Admin-API A-Database DB stuff like queries, migrations, new/remove columns, indexes, unexpected entries in the db A-SQLite Database issues specific to SQLite O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@squahtx
Copy link
Contributor

squahtx commented Aug 12, 2022

Similar to #13505.
Originally reported by @jplatte in #13509 (comment):

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.

@squahtx squahtx added A-Admin-API T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches labels Aug 12, 2022
@DMRobertson DMRobertson added A-Database DB stuff like queries, migrations, new/remove columns, indexes, unexpected entries in the db A-SQLite Database issues specific to SQLite S-Tolerable Minor significance, cosmetic issues, low or no impact to users. O-Uncommon Most users are unlikely to come across this or unexpected workflow and removed P4 (OBSOLETE: use S- labels.) Okay backlog: will not schedule, will accept patches labels Aug 31, 2022
@dklimpel
Copy link
Contributor

dklimpel commented Sep 8, 2022

IMO, in this case, it has not the reason in SQLite, but the database layout:

CREATE TABLE users (
name text,
password_hash text,
creation_ts bigint,
admin smallint DEFAULT 0 NOT NULL,
upgrade_ts bigint,
is_guest smallint DEFAULT 0 NOT NULL,
appservice_id text,
consent_version text,
consent_server_notice_sent text,
user_type text
);

ALTER TABLE users ADD COLUMN shadow_banned BOOLEAN;

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
Copy link
Contributor

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

@dklimpel
Copy link
Contributor

dklimpel commented Sep 8, 2022

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):

CREATE TABLE group_users ( group_id TEXT NOT NULL, user_id TEXT NOT NULL, is_admin BOOLEAN NOT NULL, is_public BOOLEAN NOT NULL );

and (admin SMALLINT):

CREATE TABLE users( name TEXT, password_hash TEXT, creation_ts BIGINT, admin SMALLINT DEFAULT 0 NOT NULL, upgrade_ts BIGINT, is_guest SMALLINT DEFAULT 0 NOT NULL, appservice_id TEXT, consent_version TEXT, consent_server_notice_sent TEXT, user_type TEXT DEFAULT NULL, UNIQUE(name) );

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
Copy link
Member

clokep commented Sep 8, 2022

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
Copy link
Contributor

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:

  • there is no dedicated boolean type in SQLite, though there are FALSE and TRUE literals as of 3.23.0.
  • booleans are stored as integers by sqlite
  • any column other than INTEGER PRIMARY KEY can contain values of any type ("storage class") whatsoever

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.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Admin-API A-Database DB stuff like queries, migrations, new/remove columns, indexes, unexpected entries in the db A-SQLite Database issues specific to SQLite O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Tolerable Minor significance, cosmetic issues, low or no impact to users. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

4 participants