Merge pull request #51 from jagerman/pgsql-testing

Add PostgreSQL to test suite

Author: jagerman

.drone.jsonnet

@@ -27,7 +27,10 @@ local debian_pipeline(name,
                       image,
                       arch='amd64',
                       deps=default_deps,
+                      before_pytest=[],
+                      pytest_opts='',
                       extra_cmds=[],
+                      services=[],
                       allow_fail=false) = {
   kind: 'pipeline',
   type: 'docker',
@@ -51,13 +54,29 @@ local debian_pipeline(name,
                   'eatmydata ' + apt_get_quiet + ' update',
                   'eatmydata ' + apt_get_quiet + ' dist-upgrade -y',
                   'eatmydata ' + apt_get_quiet + ' install --no-install-recommends -y ' + std.join(' ', deps),
-                  'PYTHONPATH=. python3 -mpytest -vv --color=yes --sql-tracing',
+                ] + before_pytest + [
+                  'PYTHONPATH=. python3 -mpytest -vv --color=yes --sql-tracing ' + pytest_opts,
                 ]
                 + extra_cmds,
     },
   ],
+  services: services,
 };
 
+local debian_pg_pipeline(name, image, pg_tag='bullseye') = debian_pipeline(
+  name,
+  image,
+  deps=default_deps + ['python3-psycopg2', 'postgresql-client'],
+  services=[
+    { name: 'pg', image: 'postgres:bullseye', environment: { POSTGRES_USER: 'ci', POSTGRES_PASSWORD: 'ci' } },
+  ],
+  before_pytest=[
+    'for i in $(seq 0 30); do if pg_isready -d ci -h pg -U ci -t 1; then break; fi; if [ "$i" = 30 ]; then echo "Timeout waiting for postgresql" >&2; exit 1; fi; sleep 1; done',
+  ],
+  pytest_opts='--pgsql "postgresql://ci:ci@pg/ci"'
+);
+
+
 [
   {
     name: 'Lint checks',
@@ -92,6 +111,9 @@ local debian_pipeline(name,
   debian_pipeline('Ubuntu latest (amd64)', docker_base + 'ubuntu-rolling'),
   debian_pipeline('Ubuntu LTS (amd64)', docker_base + 'ubuntu-lts'),
 
+  debian_pg_pipeline('PostgreSQL 14/sid', docker_base + 'debian-sid', pg_tag='14-bullseye'),
+  debian_pg_pipeline('PostgreSQL 12/focal', docker_base + 'ubuntu-focal', pg_tag='12-bullseye'),
+
   // ARM builds (ARM64 and armhf)
   debian_pipeline('Debian sid (ARM64)', docker_base + 'debian-sid', arch='arm64'),
   debian_pipeline('Debian stable (armhf)', docker_base + 'debian-stable/arm32v7', arch='arm64'),

sogs/config.py

@@ -11,7 +11,6 @@
 
 # Default config settings; most of these are configurable via config.ini (see it for details).
 DB_URL = 'sqlite:///sogs.db'
-DB_SCHEMA_FILE = 'schema.sql'  # Not configurable, just a constant
 KEY_FILE = 'key_x25519'
 URL_BASE = 'http://example.net'
 HTTP_SHOW_INDEX = True

sogs/db.py

@@ -117,17 +117,25 @@ def database_init():
         metadata.clear()
         metadata.reflect(bind=engine, views=True)
 
+        if 'messages' not in metadata.tables:
+            msg = (
+                "Critical error: SQL schema creation failed; "
+                f"tables: {', '.join(metadata.tables.keys())}"
+            )
+            logging.critical(msg)
+            raise RuntimeError(msg)
+
     changes = False
 
     # Database migrations/updates/etc.
     for migrate in (
         migrate_v01x,
         add_new_tables,
         add_new_columns,
-        update_message_views,
         create_message_details_deleter,
         check_for_hacks,
         seqno_etc_updates,
+        update_message_views,
         user_perm_future_updates,
     ):
         with transaction(conn):
@@ -220,7 +228,7 @@ def add_new_tables(conn):
 
 
 def update_message_views(conn):
-    if engine.name != "sqlite":
+    if engine.name == "sqlite":
         if any(x not in metadata.tables['message_metadata'].c for x in ('whisper_to', 'filtered')):
             logging.warning("DB migration: replacing message_metadata/message_details views")
             conn.execute("DROP VIEW IF EXISTS message_metadata")
@@ -232,19 +240,30 @@ def update_message_views(conn):
     FROM messages
         JOIN users uposter ON messages."user" = uposter.id
         LEFT JOIN users uwhisper ON messages.whisper = uwhisper.id
+"""
+            )
+            conn.execute(
+                """
+CREATE TRIGGER message_details_deleter INSTEAD OF DELETE ON message_details
+FOR EACH ROW WHEN OLD.data IS NOT NULL
+BEGIN
+    UPDATE messages SET data = NULL, data_size = NULL, signature = NULL
+        WHERE id = OLD.id;
+END
 """
             )
             conn.execute(
                 """
 CREATE VIEW message_metadata AS
-SELECT id, room, "user", session_id, posted, edited, updated, filtered, whisper_to,
+SELECT id, room, "user", session_id, posted, edited, seqno, filtered, whisper_to,
         length(data) AS data_unpadded, data_size, length(signature) as signature_length
     FROM message_details
 """
             )
 
             return True
-        # else: don't worry about this for postgresql because initial pg support had the fix
+
+    # else: don't worry about this for postgresql because initial pg support had the fix
 
     return False
 
@@ -334,7 +353,7 @@ def seqno_etc_updates(conn):
             """
 CREATE TABLE pinned_messages (
     room INTEGER NOT NULL REFERENCES rooms(id) ON DELETE CASCADE,
-    message INTEGER NOT NULL REFERENCES rooms(id) ON DELETE CASCADE,
+    message INTEGER NOT NULL REFERENCES messages(id) ON DELETE CASCADE,
     pinned_by INTEGER NOT NULL REFERENCES users(id),
     pinned_at FLOAT NOT NULL DEFAULT ((julianday('now') - 2440587.5)*86400.0), /* unix epoch when pinned */
     PRIMARY KEY(room, message)
@@ -417,7 +436,7 @@ def seqno_etc_updates(conn):
             """
 CREATE TABLE pinned_messages (
     room BIGINT NOT NULL REFERENCES rooms ON DELETE CASCADE,
-    message BIGINT NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    message BIGINT NOT NULL REFERENCES messages ON DELETE CASCADE,
     pinned_by BIGINT NOT NULL REFERENCES users,
     pinned_at FLOAT NOT NULL DEFAULT (extract(epoch from now())),
     PRIMARY KEY(room, message)
@@ -531,7 +550,7 @@ def user_perm_future_updates(conn):
         conn.execute(
             """
 CREATE TABLE user_ban_futures (
-    room INTEGER NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    room INTEGER REFERENCES rooms ON DELETE CASCADE,
     "user" INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
     at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
     banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */
@@ -568,24 +587,17 @@ def create_admin_user(dbconn):
     )
 
 
-if config.DB_URL.startswith('postgresql'):
-    # rooms.token is a 'citext' (case-insensitive text), which sqlalchemy doesn't recognize out of
-    # the box.  Map it to a plain TEXT which is good enough for what we need (if we actually needed
-    # to generate this wouldn't suffice: we'd have to use something like the sqlalchemy-citext
-    # module).
-    from sqlalchemy.dialects.postgresql.base import ischema_names
-
-    if 'citext' not in ischema_names:
-        ischema_names['citext'] = ischema_names['text']
-
-
 engine, engine_initial_pid, metadata = None, None, None
 
 
 def _init_engine(*args, **kwargs):
     """
     Initializes or reinitializes db.engine.  (Only the test suite should be calling this externally
     to reinitialize).
+
+    Arguments:
+    sogs_preinit - a callable to invoke after setting up `engine` but before calling
+    `database_init()`.
     """
     global engine, engine_initial_pid, metadata, have_returning
 
@@ -597,9 +609,17 @@ def _init_engine(*args, **kwargs):
             return
         args = (config.DB_URL,)
 
-    # Disable *sqlalchemy*-level autocommit, which works so badly that it got completely removed in
-    # 2.0.  (We put the actual sqlite into driver-level autocommit mode below).
-    engine = sqlalchemy.create_engine(*args, **kwargs).execution_options(autocommit=False)
+    preinit = kwargs.pop('sogs_preinit', None)
+
+    exec_opts_args = {}
+    if args[0].startswith('postgresql'):
+        exec_opts_args['isolation_level'] = 'READ COMMITTED'
+    else:
+        # SQLite's Python code is seriously broken, so we have to force off autocommit mode and turn
+        # on driver-level autocommit (which we do below).
+        exec_opts_args['autocommit'] = False
+
+    engine = sqlalchemy.create_engine(*args, **kwargs).execution_options(**exec_opts_args)
     engine_initial_pid = os.getpid()
     metadata = sqlalchemy.MetaData()
 
@@ -619,6 +639,10 @@ def sqlite_fix_connect(dbapi_connection, connection_record):
             # disable pysqlite's emitting of the BEGIN statement entirely.
             # also stops it from emitting COMMIT before any DDL.
             dbapi_connection.isolation_level = None
+            # Enforce foreign keys.  It is very sad that this is not default.
+            cursor = dbapi_connection.cursor()
+            cursor.execute("PRAGMA foreign_keys=ON")
+            cursor.close()
 
         @sqlalchemy.event.listens_for(engine, "begin")
         def do_begin(conn):
@@ -628,6 +652,18 @@ def do_begin(conn):
     else:
         have_returning = True
 
+        # rooms.token is a 'citext' (case-insensitive text), which sqlalchemy doesn't recognize out
+        # of the box.  Map it to a plain TEXT which is good enough for what we need (if we actually
+        # needed to generate this wouldn't suffice: we'd have to use something like the
+        # sqlalchemy-citext module).
+        from sqlalchemy.dialects.postgresql.base import ischema_names
+
+        if 'citext' not in ischema_names:
+            ischema_names['citext'] = ischema_names['text']
+
+    if preinit:
+        preinit()
+
     database_init()
 
 

sogs/model/room.py

@@ -525,7 +525,7 @@ def get_messages_for(
             # - anything directed to us specifically
             # - anything we sent (i.e. outbound whispers)
             # - non-whispers
-            "whisper_mods OR whisper = :user OR user = :user OR whisper IS NULL"
+            'whisper_mods OR whisper = :user OR "user" = :user OR whisper IS NULL'
             if mod
             # For a regular user we want to see:
             # - anything with whisper_to sent to us

sogs/schema.pgsql

@@ -17,7 +17,6 @@ CREATE TABLE rooms (
     upload BOOLEAN NOT NULL DEFAULT TRUE, /* Whether file uploads are allowed by default */
     CHECK(token SIMILAR TO '[a-zA-Z0-9_-]+')
 );
-CREATE INDEX rooms_token ON rooms(token);
 
 -- Trigger to expire an old room image attachment when the room image is changed
 CREATE OR REPLACE FUNCTION trigger_room_image_expiry()
@@ -90,8 +89,8 @@ EXECUTE PROCEDURE trigger_messages_insert_history();
 
 CREATE TABLE pinned_messages (
     room BIGINT NOT NULL REFERENCES rooms ON DELETE CASCADE,
-    message BIGINT NOT NULL REFERENCES rooms ON DELETE CASCADE,
-    pinned_by BIGINT NOT NULL REFERENCES users,
+    message BIGINT NOT NULL REFERENCES messages ON DELETE CASCADE,
+    pinned_by BIGINT NOT NULL, /* foreign key to users(id) */
     pinned_at FLOAT NOT NULL DEFAULT (extract(epoch from now())),
     PRIMARY KEY(room, message)
 );
@@ -154,6 +153,7 @@ CREATE INDEX users_last_active ON users(last_active);
 ALTER TABLE messages ADD CONSTRAINT messages_user_fk FOREIGN KEY ("user") REFERENCES users;
 ALTER TABLE messages ADD CONSTRAINT messages_whisper_fk FOREIGN KEY (whisper) REFERENCES users;
 ALTER TABLE files ADD CONSTRAINT files_uploader_fk FOREIGN KEY (uploader) REFERENCES users;
+ALTER TABLE pinned_messages ADD CONSTRAINT pinned_messages_pinned_by FOREIGN KEY (pinned_by) REFERENCES users;
 
 -- Create a trigger to maintain the implication "admin implies moderator"
 CREATE OR REPLACE FUNCTION trigger_user_admins_are_mods()
@@ -194,7 +194,7 @@ EXECUTE PROCEDURE trigger_message_details_deleter();
 -- View of `messages` that is useful for manually inspecting table contents by only returning the
 -- length (rather than raw bytes) for data/signature.
 CREATE VIEW message_metadata AS
-SELECT id, room, "user", session_id, posted, edited, seqno, whisper_to,
+SELECT id, room, "user", session_id, posted, edited, seqno, filtered, whisper_to,
         length(data) AS data_unpadded, data_size, length(signature) as signature_length
     FROM message_details;
 
@@ -376,7 +376,7 @@ CREATE INDEX user_permissions_future_room_user ON user_permission_futures(room,
 -- their user_permissions.banned to TRUE then add a row here with banned = FALSE to schedule the
 -- unban.  (You can also schedule a future *ban* here, but the utility of that is less clear).
 CREATE TABLE user_ban_futures (
-    room INTEGER NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    room INTEGER REFERENCES rooms ON DELETE CASCADE,
     "user" INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
     at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
     banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */

sogs/schema.sqlite

@@ -81,7 +81,7 @@ END;
 
 CREATE TABLE pinned_messages (
     room INTEGER NOT NULL REFERENCES rooms(id) ON DELETE CASCADE,
-    message INTEGER NOT NULL REFERENCES rooms(id) ON DELETE CASCADE,
+    message INTEGER NOT NULL REFERENCES messages(id) ON DELETE CASCADE,
     pinned_by INTEGER NOT NULL REFERENCES users(id),
     pinned_at FLOAT NOT NULL DEFAULT ((julianday('now') - 2440587.5)*86400.0), /* unix epoch when pinned */
     PRIMARY KEY(room, message)
@@ -165,7 +165,7 @@ END;
 -- View of `messages` that is useful for manually inspecting table contents by only returning the
 -- length (rather than raw bytes) for data/signature.
 CREATE VIEW message_metadata AS
-SELECT id, room, user, session_id, posted, edited, seqno, whisper_to,
+SELECT id, room, user, session_id, posted, edited, seqno, filtered, whisper_to,
         length(data) AS data_unpadded, data_size, length(signature) as signature_length
     FROM message_details;