Merge pull request #3610 from mathesar-foundation/schemas_delete

Implement `schemas.delete` RPC method

conftest.py

@@ -13,7 +13,7 @@
 from db.engine import add_custom_types_to_ischema_names, create_engine as sa_create_engine
 from db.types import install
 from db.sql import install as sql_install
-from db.schemas.operations.drop import drop_schema as drop_sa_schema
+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.utils import get_schema_oid_from_name, get_schema_name_from_oid
 
@@ -225,7 +225,7 @@ def _create_schema(schema_name, engine, schema_mustnt_exist=True):
                 # 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)
+                    drop_sa_schema(engine, schema_name, cascade=True)
                     logger.debug(f'dropping {schema_name}')
         except OperationalError as e:
             logger.debug(f'ignoring operational error: {e}')

db/schemas/operations/drop.py

@@ -1,20 +1,33 @@
-from db.connection import execute_msar_func_with_engine
+from db.connection import execute_msar_func_with_engine, exec_msar_func
 
 
-def drop_schema(schema_name, engine, cascade=False, if_exists=False):
+def drop_schema_via_name(engine, name, cascade=False):
     """
-    Drop a schema.
+    Drop a schema by its name.
+
+    If no schema exists with the given name, an exception will be raised.
+
+    Deprecated:
+        Use drop_schema_via_oid instead. This function is deprecated because we
+        are phasing out name-based operations in favor of OID-based operations
+        and we are phasing out SQLAlchemy in favor of psycopg.
 
     Args:
-        schema_name: Name of the schema to drop.
-        engine: SQLAlchemy engine object for connecting.
-        cascade: Whether to drop the dependent objects.
-        if_exists: Whether to ignore an error if the schema doesn't
-                   exist.
+        engine: SQLAlchemy engine object for connecting. name: Name of the
+        schema to drop. cascade: Whether to drop the dependent objects.
+    """
+    execute_msar_func_with_engine(engine, 'drop_schema', name, cascade).fetchone()
+
 
-    Returns:
-        Returns a string giving the command that was run.
+def drop_schema_via_oid(conn, id, cascade=False):
+    """
+    Drop a schema by its OID.
+
+    If no schema exists with the given oid, an exception will be raised.
+
+    Args:
+        conn: a psycopg connection
+        id: the OID of the schema to drop.
+        cascade: Whether to drop the dependent objects.
     """
-    return execute_msar_func_with_engine(
-        engine, 'drop_schema', schema_name, cascade, if_exists
-    ).fetchone()[0]
+    exec_msar_func(conn, 'drop_schema', id, cascade).fetchone()

db/sql/00_msar.sql

@@ -957,61 +957,43 @@ $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
 ----------------------------------------------------------------------------------------------------
 
 
--- Drop schema -------------------------------------------------------------------------------------
-
 CREATE OR REPLACE FUNCTION
-__msar.drop_schema(sch_name text, cascade_ boolean, if_exists boolean) RETURNS TEXT AS $$/*
-Drop a schema, returning the command executed.
+msar.drop_schema(sch_name text, cascade_ boolean) RETURNS void AS $$/*
+Drop a schema
+
+If no schema exists with the given name, an exception will be raised.
 
 Args:
-  sch_name: A properly quoted name of the schema to be dropped
-  cascade_: Whether to drop dependent objects.
-  if_exists: Whether to ignore an error if the schema doesn't exist
+  sch_name: An unqoted name of the schema to be dropped
+  cascade_: When true, dependent objects will be dropped automatically
 */
 DECLARE
-  cmd_template text;
+  cascade_sql text = CASE cascade_ WHEN TRUE THEN ' CASCADE' ELSE '' END;
 BEGIN
-  IF if_exists
-  THEN
-    cmd_template := 'DROP SCHEMA IF EXISTS %s';
-  ELSE
-    cmd_template := 'DROP SCHEMA %s';
-  END IF;
-  IF cascade_
-  THEN
-    cmd_template = cmd_template || ' CASCADE';
-  END IF;
-  RETURN __msar.exec_ddl(cmd_template, sch_name);
+  EXECUTE 'DROP SCHEMA ' || quote_ident(sch_name) || cascade_sql;
 END;
 $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
 
 
 CREATE OR REPLACE FUNCTION
-msar.drop_schema(sch_id oid, cascade_ boolean, if_exists boolean) RETURNS TEXT AS $$/*
-Drop a schema, returning the command executed.
-
-Args:
-  sch_id: The OID of the schema to drop
-  cascade_: Whether to drop dependent objects.
-  if_exists: Whether to ignore an error if the schema doesn't exist
-*/
-BEGIN
-  RETURN __msar.drop_schema(__msar.get_schema_name(sch_id), cascade_, if_exists);
-END;
-$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
+msar.drop_schema(sch_id oid, cascade_ boolean) RETURNS void AS $$/*
+Drop a schema
 
-
-CREATE OR REPLACE FUNCTION
-msar.drop_schema(sch_name text, cascade_ boolean, if_exists boolean) RETURNS TEXT AS $$/*
-Drop a schema, returning the command executed.
+If no schema exists with the given oid, an exception will be raised.
 
 Args:
-  sch_name: An unqoted name of the schema to be dropped
-  cascade_: Whether to drop dependent objects.
-  if_exists: Whether to ignore an error if the schema doesn't exist
+  sch_id: The OID of the schema to drop
+  cascade_: When true, dependent objects will be dropped automatically
 */
+DECLARE
+  sch_name text;
 BEGIN
-  RETURN __msar.drop_schema(quote_ident(sch_name), cascade_, if_exists);
+  SELECT nspname INTO sch_name FROM pg_namespace WHERE oid = sch_id;
+  IF sch_name IS NULL THEN
+    RAISE EXCEPTION 'No schema with OID % exists.', sch_id
+    USING ERRCODE = '3F000'; -- invalid_schema_name
+  END IF;
+  PERFORM msar.drop_schema(sch_name, cascade_);
 END;
 $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT;
 

db/sql/test_00_msar.sql

@@ -1201,63 +1201,51 @@ END;
 $$ LANGUAGE plpgsql;
 
 
-CREATE OR REPLACE FUNCTION test_drop_schema_if_exists_false() RETURNS SETOF TEXT AS $$
+CREATE OR REPLACE FUNCTION test_drop_schema_using_name() RETURNS SETOF TEXT AS $$
 BEGIN
   PERFORM __setup_drop_schema();
   PERFORM msar.drop_schema(
     sch_name => 'drop_test_schema', 
-    cascade_ => false, 
-    if_exists => false
+    cascade_ => false
   );
   RETURN NEXT hasnt_schema('drop_test_schema');
   RETURN NEXT throws_ok(
-    format(
-      'SELECT msar.drop_schema(
-        sch_name => ''%s'',
-        cascade_ => false,
-        if_exists => false
-      );', 
-      'drop_non_existing_schema'
-    ),
-    '3F000',
-    'schema "drop_non_existing_schema" does not exist'
+    $d$
+      SELECT msar.drop_schema(
+        sch_name => 'drop_non_existing_schema',
+        cascade_ => false
+      )
+    $d$, 
+    '3F000'
   );
 END;
 $$ LANGUAGE plpgsql;
 
 
-CREATE OR REPLACE FUNCTION test_drop_schema_if_exists_true() RETURNS SETOF TEXT AS $$
+CREATE OR REPLACE FUNCTION test_drop_schema_using_oid() RETURNS SETOF TEXT AS $$
 BEGIN
   PERFORM __setup_drop_schema();
   PERFORM msar.drop_schema(
-    sch_name => 'drop_test_schema',
-    cascade_ => false,
-    if_exists => true
+    sch_id => 'drop_test_schema'::regnamespace::oid,
+    cascade_ => false
   );
   RETURN NEXT hasnt_schema('drop_test_schema');
-  RETURN NEXT lives_ok(
-    format(
-      'SELECT msar.drop_schema(
-        sch_name => ''%s'',
-        cascade_ => false,
-        if_exists => true
-      );', 
-      'drop_non_existing_schema'
-    )
-  );
 END;
 $$ LANGUAGE plpgsql;
 
 
-CREATE OR REPLACE FUNCTION test_drop_schema_using_oid() RETURNS SETOF TEXT AS $$
+CREATE OR REPLACE FUNCTION test_drop_schema_using_invalid_oid() RETURNS SETOF TEXT AS $$
 BEGIN
   PERFORM __setup_drop_schema();
-  PERFORM msar.drop_schema(
-    sch_id => 'drop_test_schema'::regnamespace::oid,
-    cascade_ => false,
-    if_exists => false
+  RETURN NEXT throws_ok(
+    $d$
+      SELECT msar.drop_schema(
+        sch_id => 0,
+        cascade_ => false
+      )
+    $d$,
+    '3F000'
   );
-  RETURN NEXT hasnt_schema('drop_test_schema');
 END;
 $$ LANGUAGE plpgsql;
 
@@ -1278,8 +1266,7 @@ BEGIN
   PERFORM __setup_schema_with_dependent_obj();
   PERFORM msar.drop_schema(
     sch_name => 'schema1',
-    cascade_ => true,
-    if_exists => false
+    cascade_ => true
   );
   RETURN NEXT hasnt_schema('schema1');
 END;
@@ -1290,16 +1277,13 @@ CREATE OR REPLACE FUNCTION test_drop_schema_restricted() RETURNS SETOF TEXT AS $
 BEGIN
   PERFORM __setup_schema_with_dependent_obj();
   RETURN NEXT throws_ok(
-    format(
-      'SELECT msar.drop_schema(
-        sch_name => ''%s'',
-        cascade_ => false,
-        if_exists => false
-      );',
-      'schema1'
-    ),
-    '2BP01',
-    'cannot drop schema schema1 because other objects depend on it'
+    $d$
+      SELECT msar.drop_schema(
+        sch_name => 'schema1',
+        cascade_ => false
+      )
+    $d$,
+    '2BP01'
   );
 END;
 $$ LANGUAGE plpgsql;
@@ -2407,10 +2391,13 @@ $$ LANGUAGE plpgsql;
 CREATE OR REPLACE FUNCTION test_get_valid_target_type_strings() RETURNS SETOF TEXT AS $$
 BEGIN
   PERFORM __setup_cast_functions();
-  RETURN NEXT is(msar.get_valid_target_type_strings('text'), '["numeric", "text"]'::jsonb);
-  RETURN NEXT is(
-    msar.get_valid_target_type_strings('text'::regtype::oid), '["numeric", "text"]'::jsonb
-  );
+
+  RETURN NEXT ok(msar.get_valid_target_type_strings('text') @> '["numeric", "text"]');
+  RETURN NEXT is(jsonb_array_length(msar.get_valid_target_type_strings('text')), 2);
+
+  RETURN NEXT ok(msar.get_valid_target_type_strings('text'::regtype::oid) @> '["numeric", "text"]');
+  RETURN NEXT is(jsonb_array_length(msar.get_valid_target_type_strings('text'::regtype::oid)), 2);
+
   RETURN NEXT is(msar.get_valid_target_type_strings('interval'), NULL);
 END;
 $$ LANGUAGE plpgsql;

db/tests/schemas/operations/test_drop.py

@@ -3,16 +3,13 @@
 import db.schemas.operations.drop as sch_drop
 
 
-@pytest.mark.parametrize(
-    "cascade, if_exists", [(True, True), (False, True), (True, False), (False, False)]
-)
-def test_drop_schema(engine_with_schema, cascade, if_exists):
+@pytest.mark.parametrize("cascade", [True, False])
+def test_drop_schema(engine_with_schema, cascade):
     engine = engine_with_schema
     with patch.object(sch_drop, 'execute_msar_func_with_engine') as mock_exec:
-        sch_drop.drop_schema('drop_test_schema', engine, cascade, if_exists)
+        sch_drop.drop_schema_via_name(engine, 'drop_test_schema', cascade)
     call_args = mock_exec.call_args_list[0][0]
     assert call_args[0] == engine
     assert call_args[1] == "drop_schema"
     assert call_args[2] == "drop_test_schema"
     assert call_args[3] == cascade
-    assert call_args[4] == if_exists

docs/docs/api/rpc.md

@@ -52,6 +52,7 @@ To use an RPC function:
     options:
       members:
       - list_
+      - delete
       - SchemaInfo
 
 ---

mathesar/models/base.py

@@ -28,7 +28,7 @@
 from db.records.operations.select import get_column_cast_records, get_count, get_record
 from db.records.operations.select import get_records
 from db.records.operations.update import update_record
-from db.schemas.operations.drop import drop_schema
+from db.schemas.operations.drop import drop_schema_via_name
 from db.schemas.operations.select import get_schema_description
 from db.schemas import utils as schema_utils
 from db.tables import utils as table_utils
@@ -241,9 +241,8 @@ def update_sa_schema(self, update_params):
         return result
 
     def delete_sa_schema(self):
-        result = drop_schema(self.name, self._sa_engine, cascade=True)
+        drop_schema_via_name(self._sa_engine, self.name, cascade=True)
         reset_reflection(db_name=self.database.name)
-        return result
 
     def clear_name_cache(self):
         cache_key = f"{self.database.name}_schema_name_{self.oid}"

mathesar/rpc/schemas.py

@@ -8,6 +8,7 @@
 
 from db.constants import INTERNAL_SCHEMAS
 from db.schemas.operations.select import get_schemas
+from db.schemas.operations.drop import drop_schema_via_oid
 from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions
 from mathesar.rpc.utils import connect
 
@@ -53,3 +54,18 @@ def list_(*, database_id: int, **kwargs) -> list[SchemaInfo]:
     # refactored the models so that each exploration is associated with a schema
     # (by oid) in a specific database.
     return [{**s, "exploration_count": 0} for s in user_defined_schemas]
+
+
+@rpc_method(name="schemas.delete")
+@http_basic_auth_login_required
+@handle_rpc_exceptions
+def delete(*, schema_id: int, database_id: int, **kwargs) -> None:
+    """
+    Delete a schema, given its OID.
+
+    Args:
+        schema_id: The OID of the schema to delete.
+        database_id: The Django id of the database containing the schema.
+    """
+    with connect(database_id, kwargs.get(REQUEST_KEY).user) as conn:
+        drop_schema_via_oid(conn, schema_id)

mathesar/tests/rpc/test_endpoints.py

@@ -39,6 +39,11 @@
         "schemas.list",
         [user_is_authenticated]
     ),
+    (
+        schemas.delete,
+        "schemas.delete",
+        [user_is_authenticated]
+    ),
     (
         tables.list_,
         "tables.list",