Merge branch 'develop' into table_import

conftest.py

@@ -14,7 +14,7 @@
 from db.types import install
 from db.sql import install as sql_install
 from db.schemas.operations.drop import drop_schema_via_name as drop_sa_schema
-from db.schemas.operations.create import create_schema as create_sa_schema
+from db.schemas.operations.create import create_schema_if_not_exists_via_sql_alchemy
 from db.schemas.utils import get_schema_oid_from_name, get_schema_name_from_oid
 
 from fixtures.utils import create_scoped_fixtures
@@ -210,7 +210,7 @@ def _create_schema(schema_name, engine, schema_mustnt_exist=True):
         if schema_mustnt_exist:
             assert schema_name not in created_schemas
         logger.debug(f'creating {schema_name}')
-        create_sa_schema(schema_name, engine, if_not_exists=True)
+        create_schema_if_not_exists_via_sql_alchemy(schema_name, engine)
         schema_oid = get_schema_oid_from_name(schema_name, engine)
         db_name = engine.url.database
         created_schemas_in_this_engine = created_schemas.setdefault(db_name, {})

db/columns/operations/create.py

@@ -5,39 +5,18 @@
 from alembic.operations import Operations
 from psycopg.errors import InvalidTextRepresentation, InvalidParameterValue
 
-from db.columns.defaults import DEFAULT, NAME, NULLABLE, TYPE, DESCRIPTION
+from db import connection as db_conn
+from db.columns.defaults import DEFAULT, NAME, NULLABLE, DESCRIPTION
 from db.columns.exceptions import InvalidDefaultError, InvalidTypeOptionError
-from db.connection import execute_msar_func_with_engine
 from db.tables.operations.select import reflect_table_from_oid
 from db.types.base import PostgresType
 from db.metadata import get_empty_metadata
 
 
 def create_column(engine, table_oid, column_data):
-    column_name = (column_data.get(NAME) or '').strip() or None
-    column_type_id = (
-        column_data.get(
-            # TYPE = 'sa_type'. This is coming straight from the API.
-            # TODO Determine whether we actually need 'sa_type' and 'type'
-            TYPE, column_data.get("type")
-        )
-        or PostgresType.CHARACTER_VARYING.id
-    )
-    column_type_options = column_data.get("type_options", {})
-    column_nullable = column_data.get(NULLABLE, True)
-    default_value = column_data.get(DEFAULT, {}).get('value')
-    column_description = column_data.get(DESCRIPTION)
-    col_create_def = [
-        {
-            "name": column_name,
-            "type": {"name": column_type_id, "options": column_type_options},
-            "not_null": not column_nullable,
-            "default": default_value,
-            "description": column_description,
-        }
-    ]
+    col_create_def = [_transform_column_create_dict(column_data)]
     try:
-        curr = execute_msar_func_with_engine(
+        curr = db_conn.execute_msar_func_with_engine(
             engine, 'add_columns',
             table_oid,
             json.dumps(col_create_def)
@@ -49,6 +28,64 @@ def create_column(engine, table_oid, column_data):
     return curr.fetchone()[0]
 
 
+def add_columns_to_table(table_oid, column_data_list, conn):
+    """
+    Add columns to the given table.
+
+    For a description of the members of column_data_list, see
+    _transform_column_create_dict
+
+    Args:
+        table_oid: The OID of the table whose columns we'll alter.
+        column_data_list: A list of dicts describing columns to add.
+        conn: A psycopg connection.
+    """
+    transformed_column_data = [
+        _transform_column_create_dict(col) for col in column_data_list
+    ]
+    result = db_conn.exec_msar_func(
+        conn, 'add_columns', table_oid, json.dumps(transformed_column_data)
+    ).fetchone()[0]
+    return result
+
+
+# TODO This function wouldn't be needed if we had the same form in the DB
+# as the RPC API function.
+def _transform_column_create_dict(data):
+    """
+    Transform the data dict into the form needed for the DB functions.
+
+    Input data form:
+    {
+        "name": <str>,
+        "type": <str>,
+        "type_options": <dict>,
+        "nullable": <bool>,
+        "default": {"value": <any>}
+        "description": <str>
+    }
+
+    Output form:
+    {
+        "type": {"name": <str>, "options": <dict>},
+        "name": <str>,
+        "not_null": <bool>,
+        "default": <any>,
+        "description": <str>
+    }
+    """
+    return {
+        "name": (data.get(NAME) or '').strip() or None,
+        "type": {
+            "name": data.get("type") or PostgresType.CHARACTER_VARYING.id,
+            "options": data.get("type_options", {})
+        },
+        "not_null": not data.get(NULLABLE, True),
+        "default": data.get(DEFAULT, {}).get('value'),
+        "description": data.get(DESCRIPTION),
+    }
+
+
 def bulk_create_mathesar_column(engine, table_oid, columns, schema):
     # TODO reuse metadata
     table = reflect_table_from_oid(table_oid, engine, metadata=get_empty_metadata())
@@ -67,7 +104,7 @@ def duplicate_column(
         copy_data=True,
         copy_constraints=True
 ):
-    curr = execute_msar_func_with_engine(
+    curr = db_conn.execute_msar_func_with_engine(
         engine,
         'copy_column',
         table_oid,

db/schemas/operations/alter.py

@@ -27,10 +27,8 @@ def comment_on_schema(schema_name, engine, comment):
     Change description of a schema.
 
     Args:
-        schema_name: The name of the schema whose comment we will
-                     change.
-        comment: The new comment. Any quotes or special characters must
-                 be escaped.
+        schema_name: The name of the schema whose comment we will change.
+        comment: The new comment.
         engine: SQLAlchemy engine object for connecting.
 
     Returns:

db/schemas/operations/create.py

@@ -1,26 +1,53 @@
-from db.schemas.operations.alter import comment_on_schema
-from db.connection import execute_msar_func_with_engine
+from db.connection import execute_msar_func_with_engine, exec_msar_func
 
 
-def create_schema(schema_name, engine, comment=None, if_not_exists=False):
+def create_schema_via_sql_alchemy(schema_name, engine, description=None):
     """
-    Creates a schema.
+    Creates a schema using a SQLAlchemy engine.
 
     Args:
         schema_name: Name of the schema to create.
         engine: SQLAlchemy engine object for connecting.
-        comment: The new comment. Any quotes or special characters must
-                 be escaped.
-        if_not_exists: Whether to ignore an error if the schema does
-                       exist.
+        description: A new description to set on the schema.
+
+    If a schema already exists with the given name, this function will raise an error.
 
     Returns:
-        Returns a string giving the command that was run.
+        The integer oid of the newly created schema.
     """
-    result = execute_msar_func_with_engine(
-        engine, 'create_schema', schema_name, if_not_exists
+    return execute_msar_func_with_engine(
+        engine, 'create_schema', schema_name, description
     ).fetchone()[0]
 
-    if comment:
-        comment_on_schema(schema_name, engine, comment)
-    return result
+
+def create_schema_if_not_exists_via_sql_alchemy(schema_name, engine):
+    """
+    Ensure that a schema exists using a SQLAlchemy engine.
+
+    Args:
+        schema_name: Name of the schema to create.
+        engine: SQLAlchemy engine object for connecting.
+
+    Returns:
+        The integer oid of the newly created schema.
+    """
+    return execute_msar_func_with_engine(
+        engine, 'create_schema_if_not_exists', schema_name
+    ).fetchone()[0]
+
+
+def create_schema(schema_name, conn, description=None):
+    """
+    Create a schema using a psycopg connection.
+
+    Args:
+        schema_name: Name of the schema to create.
+        conn: a psycopg connection
+        description: A new description to set on the schema.
+
+    If a schema already exists with the given name, this function will raise an error.
+
+    Returns:
+        The integer oid of the newly created schema.
+    """
+    return exec_msar_func(conn, 'create_schema', schema_name, description).fetchone()[0]

db/sql/00_msar.sql

@@ -143,24 +143,27 @@ $$ LANGUAGE plpgsql;
 
 
 CREATE OR REPLACE FUNCTION msar.schema_exists(schema_name text) RETURNS boolean AS $$/*
-Return true if the given schema exists in the current database, false otherwise.
+Return true if the schema exists, false otherwise.
+
+Args :
+  sch_name: The name of the schema, UNQUOTED.
 */
 SELECT EXISTS (SELECT 1 FROM pg_namespace WHERE nspname=schema_name);
 $$ LANGUAGE SQL RETURNS NULL ON NULL INPUT;
 
 
-CREATE OR REPLACE FUNCTION __msar.get_schema_oid(sch_name text) RETURNS oid AS $$/*
-Return the OID of a schema, if it can be diretly found from a name.
+CREATE OR REPLACE FUNCTION msar.get_schema_oid(sch_name text) RETURNS oid AS $$/*
+Return the OID of a schema, or NULL if the schema does not exist.
 
 Args :
-  sch_name: The name of the schema.
+  sch_name: The name of the schema, UNQUOTED.
 */
-SELECT CASE WHEN msar.schema_exists(sch_name) THEN sch_name::regnamespace::oid END;
+SELECT oid FROM pg_namespace WHERE nspname=sch_name;
 $$ LANGUAGE SQL RETURNS NULL ON NULL INPUT;
 
 
 CREATE OR REPLACE FUNCTION __msar.get_schema_name(sch_id oid) RETURNS TEXT AS $$/*
-Return the name for a given schema, quoted as appropriate.
+Return the QUOTED name for a given schema.
 
 The schema *must* be in the pg_namespace table to use this function.
 
@@ -636,7 +639,7 @@ Args:
 SELECT jsonb_agg(prorettype::regtype::text)
 FROM pg_proc
 WHERE
-  pronamespace=__msar.get_schema_oid('mathesar_types')
+  pronamespace=msar.get_schema_oid('mathesar_types')
   AND proargtypes[0]=typ_id
   AND left(proname, 5) = 'cast_';
 $$ LANGUAGE SQL RETURNS NULL ON NULL INPUT;
@@ -909,8 +912,8 @@ __msar.comment_on_schema(sch_name text, comment_ text) RETURNS TEXT AS $$/*
 Change the description of a schema, returning command executed.
 
 Args:
-  sch_name: The quoted name of the schema whose comment we will change.
-  comment_: The new comment. Any quotes or special characters must be escaped.
+  sch_name: The QUOTED name of the schema whose comment we will change.
+  comment_: The new comment, QUOTED
 */
 DECLARE
   cmd_template text;
@@ -926,8 +929,8 @@ msar.comment_on_schema(sch_name text, comment_ text) RETURNS TEXT AS $$/*
 Change the description of a schema, returning command executed.
 
 Args:
-  sch_name: The quoted name of the schema whose comment we will change.
-  comment_: The new comment.
+  sch_name: The UNQUOTED name of the schema whose comment we will change.
+  comment_: The new comment, UNQUOTED
 */
 BEGIN
   RETURN __msar.comment_on_schema(quote_ident(sch_name), quote_literal(comment_));
@@ -940,7 +943,7 @@ Change the description of a schema, returning command executed.
 
 Args:
   sch_id: The OID of the schema.
-  comment_: The new comment.
+  comment_: The new comment, UNQUOTED
 */
 BEGIN
   RETURN __msar.comment_on_schema(__msar.get_schema_name(sch_id), quote_literal(comment_));
@@ -956,43 +959,54 @@ $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
 ----------------------------------------------------------------------------------------------------
 ----------------------------------------------------------------------------------------------------
 
+-- This gets rid of `msar.create_schema` as defined in Mathesar 0.1.7. We don't want that old
+-- function definition hanging around because it will get invoked when passing NULL as the second
+-- argument like `msar.create_schema('foo', NULL)`.
+DROP FUNCTION IF EXISTS msar.create_schema(text, boolean);
 
--- Create schema -----------------------------------------------------------------------------------
-
-CREATE OR REPLACE FUNCTION
-__msar.create_schema(sch_name text, if_not_exists boolean) RETURNS TEXT AS $$/*
-Create a schema, returning the command executed.
+CREATE OR REPLACE FUNCTION msar.create_schema_if_not_exists(sch_name text) RETURNS oid AS $$/*
+Ensure that a schema exists in the database.
 
 Args:
-  sch_name: A properly quoted name of the schema to be created
-  if_not_exists: Whether to ignore an error if the schema does exist
+  sch_name: the name of the schema to be created, UNQUOTED.
+
+Returns:
+  The integer OID of the schema
 */
-DECLARE
-  cmd_template text;
 BEGIN
-  IF if_not_exists
-  THEN
-    cmd_template := 'CREATE SCHEMA IF NOT EXISTS %s';
-  ELSE
-    cmd_template := 'CREATE SCHEMA %s';
-  END IF;
-  RETURN __msar.exec_ddl(cmd_template, sch_name);
+  EXECUTE 'CREATE SCHEMA IF NOT EXISTS ' || quote_ident(sch_name);
+  RETURN msar.get_schema_oid(sch_name);
 END;
-$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
+$$ LANGUAGE plpgsql;
 
 
-CREATE OR REPLACE FUNCTION
-msar.create_schema(sch_name text, if_not_exists boolean) RETURNS TEXT AS $$/*
-Create a schema, returning the command executed.
+CREATE OR REPLACE FUNCTION msar.create_schema(
+  sch_name text,
+  description text DEFAULT ''
+) RETURNS oid AS $$/*
+Create a schema, possibly with a description.
+
+If a schema with the given name already exists, an exception will be raised.
 
 Args:
-  sch_name: An unquoted name of the schema to be created
-  if_not_exists: Whether to ignore an error if the schema does exist
+  sch_name: the name of the schema to be created, UNQUOTED.
+  description: (optional) A description for the schema, UNQUOTED.
+
+Returns:
+  The integer OID of the schema
+
+Note: This function does not support IF NOT EXISTS because it's simpler that way. I originally tried
+to support descriptions and if_not_exists in the same function, but as I discovered more edge cases
+and inconsistencies, it got too complex, and I didn't think we'd have a good enough use case for it.
 */
+DECLARE schema_oid oid;
 BEGIN
-  RETURN __msar.create_schema(quote_ident(sch_name), if_not_exists);
+  EXECUTE 'CREATE SCHEMA ' || quote_ident(sch_name);
+  schema_oid := msar.get_schema_oid(sch_name);
+  PERFORM msar.comment_on_schema(schema_oid, description);
+  RETURN schema_oid;
 END;
-$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
+$$ LANGUAGE plpgsql;
 
 
 ----------------------------------------------------------------------------------------------------
@@ -1152,7 +1166,7 @@ $$ LANGUAGE SQL;
 -- Alter table -------------------------------------------------------------------------------------
 CREATE OR REPLACE FUNCTION
 msar.alter_table(tab_id oid, tab_alters jsonb) RETURNS text AS $$/*
-Alter columns of the given table in bulk, returning the IDs of the columns so altered.
+Alter the name, description, or columns of a table, returning name of the altered table.
 
 Args:
   tab_id: The OID of the table whose columns we'll alter.