Type logic refactor

.env.example

@@ -5,3 +5,5 @@ SECRET_KEY=2gr6ud88x=(p855_5nbj_+7^bw-iz&n7ldqv%94mjaecl+b9=4
 DJANGO_DATABASE_KEY=default
 DJANGO_DATABASE_URL=postgres://mathesar:mathesar@mathesar_db:5432/mathesar_django
 MATHESAR_DATABASES=(mathesar_tables|postgresql://mathesar:mathesar@mathesar_db:5432/mathesar)
+# Change this to Dockerfile.integ-tests to be able to run e2e integ tests
+DOCKERFILE=Dockerfile

.github/workflows/run-e2e-integ-tests.yml

@@ -34,23 +34,23 @@ jobs:
 
       - name: Build the stack
         run: docker-compose up --build -d
-      
+
       - name: Sleep for 60 seconds
         run: sleep 60s
         shell: bash
 
       # TODO: This needs to be handled inside the tests
       - name: Run migrations
         run: docker exec mathesar_service python manage.py migrate
-      
+
       - name: Run type installation
         run: docker exec mathesar_service python install.py --skip-confirm
 
       - name: Build front end
         run: docker exec -w /code/mathesar_ui mathesar_service npx vite build
 
       - name: Run integ tests with pytest
-        run: docker exec mathesar_service pytest --browser chromium --browser webkit --browser firefox --video="on" --output="./videos/" mathesar/tests/integration/
+        run: docker exec mathesar_service pytest --browser chromium --browser webkit --browser firefox --video="on" --output="./videos/" mathesar/tests/integration/ -n0
 
       - uses: actions/upload-artifact@v2
         if: ${{ failure() || success() }}

.github/workflows/run-pytest.yml

@@ -32,7 +32,7 @@ jobs:
       - name: Build the stack
         run: docker-compose up --build -d
 
-      - name: create coverage directory
+      - name: Create coverage directory
         run: docker exec mathesar_service mkdir coverage_report
 
       - name: Run tests with pytest

conftest.py

@@ -4,109 +4,197 @@
 environment (e.g., the login info for the Postgres instance for testing)
 """
 import pytest
-from sqlalchemy import text
-from config.settings import DATABASES
+import random
+import string
+
+from django.db import connection as dj_connection
+
+from sqlalchemy_utils import database_exists, create_database, drop_database
+from sqlalchemy.exc import OperationalError
+
 from db.engine import add_custom_types_to_ischema_names, create_engine
 from db.types import install
-from db.schemas.operations.create import create_schema
-from db.schemas.operations.drop import drop_schema
+from db.schemas.operations.drop import drop_schema as drop_sa_schema
+from db.schemas.operations.create import create_schema as create_sa_schema
+from db.schemas.utils import get_schema_oid_from_name, get_schema_name_from_oid
 
 
 @pytest.fixture(scope="session")
-def test_db_name():
-    test_db_name = "mathesar_db_test"
-    superuser_engine = _get_superuser_engine()
-    with superuser_engine.connect() as conn:
-        conn.execution_options(isolation_level="AUTOCOMMIT")
-        conn.execute(text(f"DROP DATABASE IF EXISTS {test_db_name} WITH (FORCE)"))
-        conn.execute(text(f"CREATE DATABASE {test_db_name}"))
-    yield test_db_name
-    with superuser_engine.connect() as conn:
-        conn.execution_options(isolation_level="AUTOCOMMIT")
-        conn.execute(text(f"DROP DATABASE {test_db_name} WITH (FORCE)"))
+def worker_id(worker_id):
+    """
+    Guaranteed to always be a non-empty string.
+
+    Returns 'master' when we're not parallelizing, 'gw0', 'gw1', etc., otherwise.
+    """
+    return worker_id
 
 
 @pytest.fixture(scope="session")
-def engine(test_db_name):
-    engine = _create_engine(test_db_name)
+def get_uid(worker_id):
+    """
+    A factory of session-unique 4 letter strings.
+    """
+    used_uids = set()
+
+    def _get_uid():
+        letters = string.ascii_letters
+        candidate = "".join(random.sample(letters, 4))
+        if worker_id:
+            candidate = worker_id + '_' + candidate
+        if candidate not in used_uids:
+            used_uids.add(candidate)
+            return candidate
+        else:
+            return _get_uid()
+    yield _get_uid
+
+
+@pytest.fixture(scope="function")
+def uid(get_uid):
+    """
+    A session-unique string.
+    """
+    return get_uid()
+
+
+def _get_connection_string(username, password, hostname, database):
+    return f"postgresql://{username}:{password}@{hostname}/{database}"
+
+
+def _create_engine(db_name):
+    dj_connection_settings = dj_connection.settings_dict
+    engine = create_engine(
+        _get_connection_string(
+            username=dj_connection_settings["USER"],
+            password=dj_connection_settings["PASSWORD"],
+            hostname=dj_connection_settings["HOST"],
+            database=db_name,
+        ),
+        future=True,
+        # Setting a fixed timezone makes the timezone aware test cases predictable.
+        connect_args={"options": "-c timezone=utc -c lc_monetary=en_US.UTF-8"}
+    )
     return engine
 
 
-@pytest.fixture(scope="session")
-def engine_with_ischema_names_updated(test_db_name):
+def _create_db():
     """
-    This fixture does not inherit from the fixture `engine`, because it mutates the engine, which
-    would otherwise taint tests depending on `engine`.
+    A factory for Postgres mathesar-installed databases. A fixture made of this method tears down
+    created dbs when leaving scope.
+
+    This method is used to create two fixtures with different scopes, that's why it's not a fixture
+    itself.
     """
-    engine = _create_engine(test_db_name)
-    add_custom_types_to_ischema_names(engine)
-    return engine
+    created_dbs = set()
 
+    def __create_db(db_name):
+        engine = _create_engine(db_name)
+        if database_exists(engine.url):
+            drop_database(engine.url)
+        create_database(engine.url)
+        created_dbs.add(db_name)
+        # Our default testing database has our types and functions preinstalled.
+        install.install_mathesar_on_database(engine)
+        return db_name
+    yield __create_db
+    for db_name in created_dbs:
+        engine = _create_engine(db_name)
+        if database_exists(engine.url):
+            drop_database(engine.url)
 
-@pytest.fixture
-def test_schema_name():
-    return "test_schema"
 
+# This factory will clean up its created dbs after each test function that it's used in.
+# Useful when doing API stuff that would otherwise be bothersome to clean up.
+# TODO padaryti kad duombaze butu ideta ir isimta is settings.DATABASES
+# kaip tai daroma kituose fixtures?
+create_temp_db = pytest.fixture(_create_db, scope="function")
 
-@pytest.fixture
-def engine_with_schema(engine, test_schema_name):
-    schema = test_schema_name
-    _create_schema(engine, schema)
-    yield engine, schema
-    _drop_schema(engine, schema)
 
+# This factory will clean up its created dbs after its module is finished testing.
+create_module_db = pytest.fixture(_create_db, scope="module")
 
-@pytest.fixture
-def engine_with_schema_with_ischema_names_updated(engine_with_ischema_names_updated, test_schema_name):
-    engine = engine_with_ischema_names_updated
-    schema = test_schema_name
-    _create_schema(engine, schema)
-    yield engine, schema
-    _drop_schema(engine, schema)
 
+# This factory will clean up its created dbs after the whole testing session is finished.
+create_session_db = pytest.fixture(_create_db, scope="session")
 
-@pytest.fixture
-def engine_with_mathesar(engine_with_schema_with_ischema_names_updated):
-    engine, schema = engine_with_schema_with_ischema_names_updated
-    install.install_mathesar_on_database(engine)
-    yield engine, schema
-    install.uninstall_mathesar_from_database(engine)
 
+@pytest.fixture(scope="session", autouse=True)
+def test_db_name(worker_id, create_session_db):
+    """
+    A dynamic, yet non-random, db_name is used so that subsequent runs would automatically clean up
+    test databases that we failed to tear down.
+    """
+    default_test_db_name = "mathesar_db_test"
+    db_name = f"{default_test_db_name}_{worker_id}"
+    create_session_db(db_name)
+    yield db_name
 
-def _create_schema(engine, schema):
-    create_schema(schema, engine)
 
+# TODO does testing this make sense?
+@pytest.fixture(scope="session")
+def engine_without_ischema_names_updated(test_db_name):
+    """
+    For testing environments where an engine might not be fully setup.
+    """
+    engine = _create_engine(test_db_name)
+    yield engine
+    engine.dispose()
 
-def _drop_schema(engine, schema):
-    drop_schema(schema, engine, cascade=True, if_exists=False)
 
+@pytest.fixture(scope="session")
+def engine(test_db_name):
+    engine = _create_engine(test_db_name)
+    add_custom_types_to_ischema_names(engine)
+    yield engine
+    engine.dispose()
 
-def _get_superuser_engine():
-    return create_engine(
-        _get_connection_string(
-            username=DATABASES["default"]["USER"],
-            password=DATABASES["default"]["PASSWORD"],
-            hostname=DATABASES["default"]["HOST"],
-            database=DATABASES["default"]["NAME"],
-        ),
-        future=True,
-    )
 
+@pytest.fixture(scope="session")
+def _test_schema_name():
+    return "_test_schema_name"
 
-def _get_connection_string(username, password, hostname, database):
-    return f"postgresql://{username}:{password}@{hostname}/{database}"
 
+@pytest.fixture
+def engine_with_schema_without_updated_ischema_names(
+    engine_without_ischema_names_updated, _test_schema_name, create_db_schema
+):
+    engine = engine_without_ischema_names_updated
+    schema_name = _test_schema_name
+    create_db_schema(schema_name, engine)
+    yield engine, schema_name
 
-def _create_engine(db_name):
-    engine = create_engine(
-        _get_connection_string(
-            DATABASES["default"]["USER"],
-            DATABASES["default"]["PASSWORD"],
-            DATABASES["default"]["HOST"],
-            db_name,
-        ),
-        future=True,
-        # Setting a fixed timezone makes the timezone aware test cases predictable.
-        connect_args={"options": "-c timezone=utc -c lc_monetary=en_US.UTF-8"}
-    )
-    return engine
+
+@pytest.fixture
+def engine_with_schema(engine, _test_schema_name, create_db_schema):
+    schema_name = _test_schema_name
+    create_db_schema(schema_name, engine)
+    yield engine, schema_name
+
+
+@pytest.fixture
+def create_db_schema():
+    """
+    Creates a DB schema factory, making sure to track and clean up new instances
+    """
+    created_schemas = {}
+
+    def _create_schema(schema_name, engine, schema_mustnt_exist=True):
+        if schema_mustnt_exist:
+            assert schema_name not in created_schemas
+        create_sa_schema(schema_name, engine)
+        schema_oid = get_schema_oid_from_name(schema_name, engine)
+        engine_url = engine.url
+        created_schemas_in_this_engine = created_schemas.setdefault(engine_url, {})
+        created_schemas_in_this_engine[schema_name] = schema_oid
+        return schema_name
+    yield _create_schema
+    for engine_url, created_schemas_in_this_engine in created_schemas.items():
+        engine = create_engine(engine_url)
+        try:
+            for _, schema_oid in created_schemas_in_this_engine.items():
+                # Handle schemas being renamed during test
+                schema_name = get_schema_name_from_oid(schema_oid, engine)
+                if schema_name:
+                    drop_sa_schema(schema_name, engine, cascade=True, if_exists=True)
+        except OperationalError:
+            pass

db/columns/base.py

@@ -6,10 +6,7 @@
 )
 from db.tables.operations.select import get_oid_from_table
 from db.types.operations.cast import get_full_cast_map
-from db.types.base import get_db_type_enum_from_class, DatabaseType
-
-from collections.abc import Collection
-from typing import Optional
+from db.types.base import get_db_type_enum_from_class
 
 
 # TODO consider renaming to DbColumn or DatabaseColumn
@@ -112,7 +109,7 @@ def add_engine(self, engine):
         self.engine = engine
 
     @property
-    def valid_target_types(self) -> Optional[Collection[DatabaseType]]:
+    def valid_target_types(self):
         """
         Returns a set of valid types to which the type of the column can be
         altered.

db/columns/operations/alter.py

@@ -11,7 +11,7 @@
 )
 from db.columns.utils import get_mathesar_column_with_engine, get_type_options
 from db.tables.operations.select import get_oid_from_table, reflect_table_from_oid
-from db.types.base import get_db_type_enum_from_class, DatabaseType, get_db_type_enum_from_id
+from db.types.base import get_db_type_enum_from_class, get_db_type_enum_from_id
 from db.types.operations.cast import get_cast_function_name
 from db.utils import execute_statement
 
@@ -60,7 +60,7 @@ def alter_column(engine, table_oid, column_attnum, column_data):
 
 
 def retype_column(
-    table_oid, column_attnum, engine, connection, new_type: DatabaseType = None, type_options={},
+    table_oid, column_attnum, engine, connection, new_type=None, type_options={},
 ):
     table = reflect_table_from_oid(table_oid, engine, connection)
     column_name = get_column_name_from_attnum(table_oid, column_attnum, engine)
@@ -96,7 +96,7 @@ def retype_column(
 
 
 def alter_column_type(
-    table_oid, column_name, engine, connection, target_type: DatabaseType, type_options={}
+    table_oid, column_name, engine, connection, target_type, type_options={}
 ):
     type_options = type_options if type_options is not None else {}
     table = reflect_table_from_oid(table_oid, engine, connection)
@@ -182,7 +182,7 @@ def _check_type_option_equivalence(type_options_1, type_options_2):
 
 
 def _validate_columns_for_batch_update(table, column_data):
-    ALLOWED_KEYS = ['name', 'type', 'type_options']
+    ALLOWED_KEYS = ['attnum', 'name', 'type', 'type_options']
     if len(column_data) != len(table.columns):
         raise ValueError('Number of columns passed in must equal number of columns in table')
     for single_column_data in column_data:
@@ -194,7 +194,8 @@ def _validate_columns_for_batch_update(table, column_data):
 
 def _batch_update_column_types(table_oid, column_data_list, connection, engine):
     for index, column_data in enumerate(column_data_list):
-        if 'type' in column_data:
+        column_attnum = column_data.get('attnum', None)
+        if 'type' in column_data and column_attnum is not None:
             new_type = get_db_type_enum_from_id(column_data['type'])
             type_options = column_data.get('type_options', {})
             if type_options is None:

db/engine.py

@@ -21,6 +21,7 @@ def create_future_engine_with_custom_types(
     return engine
 
 
+# TODO would an engine without ischema names updated ever be used? make it private if not
 def create_future_engine(
         username, password, hostname, database, port, *args, **kwargs
 ):
@@ -31,6 +32,7 @@ def create_future_engine(
     return create_engine(conn_str, *args, **kwargs)
 
 
+# NOTE: used in testing, hence public
 def create_engine(conn_str, *args, **kwargs):
     """
     Wrapper over sqlalchemy.create_engine that stops SA from propagating changes to ischema_names