Merge pull request #163 from cs50/postgresql

Addresses PostgreSQL rollbacks

Author: dmalan

.github/workflows/main.yml

@@ -30,6 +30,9 @@ jobs:
         pip install mysqlclient psycopg2-binary
     - name: Run tests
       run: python tests/sql.py
+      env:
+        MYSQL_HOST: 127.0.0.1
+        POSTGRESQL_HOST: 127.0.0.1
     - name: Install pypa/build
       run: python -m pip install build --user
     - name: Build a binary wheel and a source tarball

Dockerfile

@@ -0,0 +1,6 @@
+FROM cs50/cli
+
+RUN sudo apt update && sudo apt install --yes libmysqlclient-dev pgloader postgresql
+RUN sudo pip3 install mysqlclient psycopg2-binary
+
+WORKDIR /mnt

README.md

@@ -22,27 +22,25 @@ s = cs50.get_string();
 
 ## Testing
 
-1. Run `cli50` in `python-cs50`.
-1. Run `sudo su -`.
-1. Run `apt update`.
-1. Run `apt install -y libmysqlclient-dev mysql-server postgresql`.
-1. Run `pip3 install mysqlclient psycopg2-binary`.
-1. In `/etc/mysql/mysql.conf.d/mysqld.cnf`, add `skip-grant-tables` under `[mysqld]`.
-1. In `/etc/profile.d/cli.sh`, remove `valgrind` function for now.
-1. Run `service mysql start`.
-1. Run `mysql -e 'CREATE DATABASE IF NOT EXISTS test;'`.
-1. In `/etc/postgresql/12/main/pg_hba.conf`, change:
-   ```
-   local   all             postgres                                peer
-   host    all             all             127.0.0.1/32            md5
-   ```
-   to:
-   ```
-   local   all             postgres                                trust
-   host    all             all             127.0.0.1/32            trust
-   ```
-1. Run `service postgresql start`.
-1. Run `psql -c 'create database test;' -U postgres`.
+1. In one terminal, execute:
+
+    ```
+    cd python-cs50
+    docker compose build
+    docker compose up
+    ```
+
+1. In another terminal, execute:
+
+    ```
+    docker exec -it python-cs50 bash -l
+    ```
+
+    And then execute, e.g.:
+
+    ```
+    python tests/sql.py
+    ```
 
 ### Sample Tests
 

docker-compose.yml

@@ -0,0 +1,34 @@
+services:
+  cli:
+    build: .
+    container_name: python-cs50
+    depends_on:
+      - mysql
+      - postgres 
+    environment:
+      MYSQL_HOST: mysql
+      POSTGRESQL_HOST: postgresql
+    links:
+      - mysql
+      - postgres
+    tty: true
+    volumes:
+      - .:/mnt
+  mysql:
+    environment:
+      MYSQL_DATABASE: test
+      MYSQL_ALLOW_EMPTY_PASSWORD: yes
+    healthcheck:
+      test: ["CMD", "mysqladmin", "-uroot", "ping"]
+    image: cs50/mysql:8
+    ports:
+      - 3306:3306
+  postgres:
+    image: postgres:12
+    environment:
+      POSTGRES_USER: postgres
+      POSTGRES_PASSWORD: postgres
+      POSTGRES_DB: test
+    ports:
+    - 5432:5432
+version: "3.6"

setup.py

@@ -16,5 +16,5 @@
     package_dir={"": "src"},
     packages=["cs50"],
     url="https://github.com/cs50/python-cs50",
-    version="8.0.1"
+    version="8.0.2"
 )

src/cs50/sql.py

@@ -61,19 +61,17 @@ def __init__(self, url, **kwargs):
             if not os.path.isfile(matches.group(1)):
                 raise RuntimeError("not a file: {}".format(matches.group(1)))
 
-        # Create engine, disabling SQLAlchemy's own autocommit mode, raising exception if back end's module not installed
-        self._engine = sqlalchemy.create_engine(url, **kwargs).execution_options(autocommit=False)
+        # Create engine, disabling SQLAlchemy's own autocommit mode raising exception if back end's module not installed;
+        # without isolation_level, PostgreSQL warns with "there is already a transaction in progress" for our own BEGIN and
+        # "there is no transaction in progress" for our own COMMIT
+        self._engine = sqlalchemy.create_engine(url, **kwargs).execution_options(autocommit=False, isolation_level="AUTOCOMMIT")
 
         # Get logger
         self._logger = logging.getLogger("cs50")
 
         # Listener for connections
         def connect(dbapi_connection, connection_record):
 
-            # Disable underlying API's own emitting of BEGIN and COMMIT so we can ourselves
-            # https://docs.sqlalchemy.org/en/13/dialects/sqlite.html#serializable-isolation-savepoints-transactional-ddl
-            dbapi_connection.isolation_level = None
-
             # Enable foreign key constraints
             if type(dbapi_connection) is sqlite3.Connection:  # If back end is sqlite
                 cursor = dbapi_connection.cursor()
@@ -353,12 +351,30 @@ def teardown_appcontext(exception):
 
                 # If INSERT, return primary key value for a newly inserted row (or None if none)
                 elif command == "INSERT":
+
+                    # If PostgreSQL
                     if self._engine.url.get_backend_name() == "postgresql":
-                        try:
-                            result = connection.execute("SELECT LASTVAL()")
-                            ret = result.first()[0]
-                        except sqlalchemy.exc.OperationalError:  # If lastval is not yet defined for this connection
-                            ret = None
+
+                        # Return LASTVAL() or NULL, avoiding
+                        # "(psycopg2.errors.ObjectNotInPrerequisiteState) lastval is not yet defined in this session",
+                        # a la https://stackoverflow.com/a/24186770/5156190;
+                        # cf. https://www.psycopg.org/docs/errors.html re 55000
+                        result = connection.execute("""
+                            CREATE OR REPLACE FUNCTION _LASTVAL()
+                            RETURNS integer LANGUAGE plpgsql
+                            AS $$
+                            BEGIN
+                                BEGIN
+                                    RETURN (SELECT LASTVAL());
+                                EXCEPTION
+                                    WHEN SQLSTATE '55000' THEN RETURN NULL;
+                                END;
+                            END $$;
+                            SELECT _LASTVAL();
+                        """)
+                        ret = result.first()[0]
+
+                    # If not PostgreSQL
                     else:
                         ret = result.lastrowid if result.rowcount == 1 else None
 

tests/sql.py

@@ -1,4 +1,5 @@
 import logging
+import os
 import sys
 import unittest
 import warnings
@@ -155,7 +156,7 @@ def tearDownClass(self):
 class MySQLTests(SQLTests):
     @classmethod
     def setUpClass(self):
-        self.db = SQL("mysql://root@127.0.0.1/test")
+        self.db = SQL(f"mysql://root@{os.getenv('MYSQL_HOST')}/test")
 
     def setUp(self):
         self.db.execute("CREATE TABLE IF NOT EXISTS cs50 (id INTEGER NOT NULL AUTO_INCREMENT, val VARCHAR(16), bin BLOB, PRIMARY KEY (id))")
@@ -165,7 +166,7 @@ def setUp(self):
 class PostgresTests(SQLTests):
     @classmethod
     def setUpClass(self):
-        self.db = SQL("postgresql://postgres:postgres@127.0.0.1/test")
+        self.db = SQL(f"postgresql://postgres:postgres@{os.getenv('POSTGRESQL_HOST')}/test")
 
     def setUp(self):
         self.db.execute("CREATE TABLE IF NOT EXISTS cs50 (id SERIAL PRIMARY KEY, val VARCHAR(16), bin BYTEA)")