diff --git a/db/schemas/operations/alter.py b/db/schemas/operations/alter.py index 919997eff0..994cdb1fae 100644 --- a/db/schemas/operations/alter.py +++ b/db/schemas/operations/alter.py @@ -1,46 +1,31 @@ -from db.connection import execute_msar_func_with_engine +import json -SUPPORTED_SCHEMA_ALTER_ARGS = {'name', 'description'} +from db.connection import execute_msar_func_with_engine, exec_msar_func -def rename_schema(schema_name, engine, rename_to): +def patch_schema_via_sql_alchemy(schema_name, engine, patch): """ - Rename an existing schema. + Patch a schema using a SQLAlchemy engine. Args: schema_name: Name of the schema to change. engine: SQLAlchemy engine object for connecting. - rename_to: New schema name. - - Returns: - Returns a string giving the command that was run. + patch: A dict mapping the following fields to new values: + - 'name' (optional): New name for the schema. + - 'description' (optional): New description for the schema. """ - if rename_to == schema_name: - return - return execute_msar_func_with_engine( - engine, 'rename_schema', schema_name, rename_to - ).fetchone()[0] + execute_msar_func_with_engine(engine, "patch_schema", schema_name, json.dumps(patch)) -def comment_on_schema(schema_name, engine, comment): +def patch_schema(schema_oid, conn, patch): """ - Change description of a schema. + Patch a schema using a psycopg connection. Args: - schema_name: The name of the schema whose comment we will change. - comment: The new comment. - engine: SQLAlchemy engine object for connecting. - - Returns: - Returns a string giving the command that was run. + schema_oid: The OID of the schema to change. + conn: a psycopg connection + patch: A dict mapping the following fields to new values: + - 'name' (optional): New name for the schema. + - 'description' (optional): New description for the schema. """ - return execute_msar_func_with_engine( - engine, 'comment_on_schema', schema_name, comment - ).fetchone()[0] - - -def alter_schema(name, engine, update_data): - if "description" in update_data: - comment_on_schema(name, engine, update_data["description"]) - if "name" in update_data: - rename_schema(name, engine, update_data["name"]) + exec_msar_func(conn, "patch_schema", schema_oid, json.dumps(patch)) diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index 52e69e92e9..f242b84797 100644 --- a/db/sql/00_msar.sql +++ b/db/sql/00_msar.sql @@ -162,18 +162,26 @@ 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 QUOTED name for a given schema. +CREATE OR REPLACE FUNCTION msar.get_schema_name(sch_id oid) RETURNS TEXT AS $$/* +Return the UNQUOTED name for a given schema. -The schema *must* be in the pg_namespace table to use this function. +Raises an exception if the schema is not found. Args: sch_id: The OID of the schema. */ +DECLARE sch_name text; BEGIN - RETURN sch_id::regnamespace::text; + 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; + + RETURN sch_name; END; -$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; +$$ LANGUAGE plpgsql; CREATE OR REPLACE FUNCTION @@ -834,95 +842,71 @@ $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; ---------------------------------------------------------------------------------------------------- ---------------------------------------------------------------------------------------------------- - --- Rename schema ----------------------------------------------------------------------------------- - -CREATE OR REPLACE FUNCTION -__msar.rename_schema(old_sch_name text, new_sch_name text) RETURNS TEXT AS $$/* -Change a schema's name, returning the command executed. +DROP FUNCTION IF EXISTS msar.rename_schema(oid, text); +CREATE OR REPLACE FUNCTION msar.rename_schema(sch_id oid, new_sch_name text) RETURNS void AS $$/* +Change a schema's name Args: - old_sch_name: A properly quoted original schema name - new_sch_name: A properly quoted new schema name + sch_id: The OID of the schema to rename + new_sch_name: A new for the schema, UNQUOTED */ DECLARE - cmd_template text; -BEGIN - cmd_template := 'ALTER SCHEMA %s RENAME TO %s'; - RETURN __msar.exec_ddl(cmd_template, old_sch_name, new_sch_name); -END; -$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; - - -CREATE OR REPLACE FUNCTION -msar.rename_schema(old_sch_name text, new_sch_name text) RETURNS TEXT AS $$/* -Change a schema's name, returning the command executed. - -Args: - old_sch_name: An unquoted original schema name - new_sch_name: An unquoted new schema name -*/ -BEGIN - RETURN __msar.rename_schema(quote_ident(old_sch_name), quote_ident(new_sch_name)); -END; -$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; - - -CREATE OR REPLACE FUNCTION msar.rename_schema(sch_id oid, new_sch_name text) RETURNS TEXT AS $$/* -Change a schema's name, returning the command executed. - -Args: - sch_id: The OID of the original schema - new_sch_name: An unquoted new schema name -*/ + old_sch_name text := msar.get_schema_name(sch_id); BEGIN - RETURN __msar.rename_schema(__msar.get_schema_name(sch_id), quote_ident(new_sch_name)); + IF old_sch_name = new_sch_name THEN + -- Return early if the names are the same. This avoids an error from Postgres. + RETURN; + END IF; + EXECUTE format('ALTER SCHEMA %I RENAME TO %I', old_sch_name, new_sch_name); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; --- Comment on schema ------------------------------------------------------------------------------- +CREATE OR REPLACE FUNCTION msar.set_schema_description( + sch_id oid, + description text +) RETURNS void AS $$/* +Set the PostgreSQL description (aka COMMENT) of a schema. -CREATE OR REPLACE FUNCTION -__msar.comment_on_schema(sch_name text, comment_ text) RETURNS TEXT AS $$/* -Change the description of a schema, returning command executed. +Descriptions are removed by passing an empty string. Passing a NULL description will cause +this function to return NULL withou doing anything. Args: - sch_name: The QUOTED name of the schema whose comment we will change. - comment_: The new comment, QUOTED + sch_id: The OID of the schema. + description: The new description, UNQUOTED */ -DECLARE - cmd_template text; BEGIN - cmd_template := 'COMMENT ON SCHEMA %s IS %s'; - RETURN __msar.exec_ddl(cmd_template, sch_name, comment_); + EXECUTE format('COMMENT ON SCHEMA %I IS %L', msar.get_schema_name(sch_id), description); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; -CREATE OR REPLACE FUNCTION -msar.comment_on_schema(sch_name text, comment_ text) RETURNS TEXT AS $$/* -Change the description of a schema, returning command executed. +CREATE OR REPLACE FUNCTION msar.patch_schema(sch_id oid, patch jsonb) RETURNS void AS $$/* +Modify a schema according to the given patch. Args: - sch_name: The UNQUOTED name of the schema whose comment we will change. - comment_: The new comment, UNQUOTED + sch_id: The OID of the schema. + patch: A JSONB object with the following keys: + - name: (optional) The new name of the schema + - description: (optional) The new description of the schema. To remove a description, pass an + empty string. Passing a NULL description will have no effect on the description. */ BEGIN - RETURN __msar.comment_on_schema(quote_ident(sch_name), quote_literal(comment_)); + PERFORM msar.rename_schema(sch_id, patch->>'name'); + PERFORM msar.set_schema_description(sch_id, patch->>'description'); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; -CREATE OR REPLACE FUNCTION msar.comment_on_schema(sch_id oid, comment_ text) RETURNS TEXT AS $$/* -Change the description of a schema, returning command executed. +CREATE OR REPLACE FUNCTION msar.patch_schema(sch_name text, patch jsonb) RETURNS void AS $$/* +Modify a schema according to the given patch. Args: - sch_id: The OID of the schema. - comment_: The new comment, UNQUOTED + sch_id: The name of the schema. + patch: A JSONB object as specified by msar.patch_schema(sch_id oid, patch jsonb) */ BEGIN - RETURN __msar.comment_on_schema(__msar.get_schema_name(sch_id), quote_literal(comment_)); + PERFORM msar.patch_schema(msar.get_schema_oid(sch_name), patch); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; @@ -979,7 +963,7 @@ DECLARE schema_oid oid; BEGIN EXECUTE 'CREATE SCHEMA ' || quote_ident(sch_name); schema_oid := msar.get_schema_oid(sch_name); - PERFORM msar.comment_on_schema(schema_oid, description); + PERFORM msar.set_schema_description(schema_oid, description); RETURN schema_oid; END; $$ LANGUAGE plpgsql; @@ -1022,15 +1006,8 @@ Args: sch_id: The OID of the schema to drop cascade_: When true, dependent objects will be dropped automatically */ -DECLARE - sch_name text; BEGIN - 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_); + PERFORM msar.drop_schema(msar.get_schema_name(sch_id), cascade_); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; @@ -2339,7 +2316,7 @@ DECLARE column_defs __msar.col_def[]; constraint_defs __msar.con_def[]; BEGIN - fq_table_name := format('%s.%s', __msar.get_schema_name(sch_oid), quote_ident(tab_name)); + fq_table_name := format('%I.%I', msar.get_schema_name(sch_oid), tab_name); column_defs := msar.process_col_def_jsonb(0, col_defs, false, true); constraint_defs := msar.process_con_def_jsonb(0, con_defs); PERFORM __msar.add_table(fq_table_name, column_defs, constraint_defs); diff --git a/db/sql/test_00_msar.sql b/db/sql/test_00_msar.sql index db10bb73e5..9bce00d3cf 100644 --- a/db/sql/test_00_msar.sql +++ b/db/sql/test_00_msar.sql @@ -1310,47 +1310,40 @@ END; $$ LANGUAGE plpgsql; -CREATE OR REPLACE FUNCTION __setup_alter_schema() RETURNS SETOF TEXT AS $$ +CREATE OR REPLACE FUNCTION test_patch_schema() RETURNS SETOF TEXT AS $$ +DECLARE sch_oid oid; BEGIN - CREATE SCHEMA alter_me; -END; -$$ LANGUAGE plpgsql; - + CREATE SCHEMA foo; + SELECT msar.get_schema_oid('foo') INTO sch_oid; -CREATE OR REPLACE FUNCTION test_rename_schema() RETURNS SETOF TEXT AS $$ -BEGIN - PERFORM __setup_alter_schema(); - PERFORM msar.rename_schema( - old_sch_name => 'alter_me', - new_sch_name => 'altered' - ); - RETURN NEXT hasnt_schema('alter_me'); + PERFORM msar.patch_schema('foo', '{"name": "altered"}'); + RETURN NEXT hasnt_schema('foo'); RETURN NEXT has_schema('altered'); -END; -$$ LANGUAGE plpgsql; + RETURN NEXT is(obj_description(sch_oid), NULL); + RETURN NEXT is(msar.get_schema_name(sch_oid), 'altered'); + PERFORM msar.patch_schema(sch_oid, '{"description": "yay"}'); + RETURN NEXT is(obj_description(sch_oid), 'yay'); -CREATE OR REPLACE FUNCTION test_rename_schema_using_oid() RETURNS SETOF TEXT AS $$ -BEGIN - PERFORM __setup_alter_schema(); - PERFORM msar.rename_schema( - sch_id => 'alter_me'::regnamespace::oid, - new_sch_name => 'altered' - ); - RETURN NEXT hasnt_schema('alter_me'); - RETURN NEXT has_schema('altered'); -END; -$$ LANGUAGE plpgsql; + -- Edge case: setting the description to null doesn't actually remove it. This behavior is + -- debatable. I did it this way because it was easier to implement. + PERFORM msar.patch_schema(sch_oid, '{"description": null}'); + RETURN NEXT is(obj_description(sch_oid), 'yay'); + -- Description is removed when an empty string is passed. + PERFORM msar.patch_schema(sch_oid, '{"description": ""}'); + RETURN NEXT is(obj_description(sch_oid), NULL); -CREATE OR REPLACE FUNCTION test_comment_on_schema() RETURNS SETOF TEXT AS $$ -BEGIN - PERFORM __setup_alter_schema(); - PERFORM msar.comment_on_schema( - sch_name => 'alter_me', - comment_ => 'test comment' - ); - RETURN NEXT is(obj_description('alter_me'::regnamespace::oid), 'test comment'); + PERFORM msar.patch_schema(sch_oid, '{"name": "NEW", "description": "WOW"}'); + RETURN NEXT has_schema('NEW'); + RETURN NEXT is(msar.get_schema_name(sch_oid), 'NEW'); + RETURN NEXT is(obj_description(sch_oid), 'WOW'); + + -- Patching should be idempotent + PERFORM msar.patch_schema(sch_oid, '{"name": "NEW", "description": "WOW"}'); + RETURN NEXT has_schema('NEW'); + RETURN NEXT is(msar.get_schema_name(sch_oid), 'NEW'); + RETURN NEXT is(obj_description(sch_oid), 'WOW'); END; $$ LANGUAGE plpgsql; diff --git a/db/tests/schemas/operations/test_alter.py b/db/tests/schemas/operations/test_alter.py deleted file mode 100644 index 3cc3e7c995..0000000000 --- a/db/tests/schemas/operations/test_alter.py +++ /dev/null @@ -1,28 +0,0 @@ -from unittest.mock import patch -import db.schemas.operations.alter as sch_alter - - -def test_rename_schema(engine_with_schema): - engine = engine_with_schema - with patch.object(sch_alter, 'execute_msar_func_with_engine') as mock_exec: - sch_alter.rename_schema('rename_me', engine, rename_to='renamed') - call_args = mock_exec.call_args_list[0][0] - assert call_args[0] == engine - assert call_args[1] == "rename_schema" - assert call_args[2] == "rename_me" - assert call_args[3] == "renamed" - - -def test_comment_on_schema(engine_with_schema): - engine = engine_with_schema - with patch.object(sch_alter, 'execute_msar_func_with_engine') as mock_exec: - sch_alter.comment_on_schema( - schema_name='comment_on_me', - engine=engine, - comment='This is a comment' - ) - call_args = mock_exec.call_args_list[0][0] - assert call_args[0] == engine - assert call_args[1] == "comment_on_schema" - assert call_args[2] == "comment_on_me" - assert call_args[3] == "This is a comment" diff --git a/docs/docs/api/rpc.md b/docs/docs/api/rpc.md index 1f2e5e0e30..2086e7d825 100644 --- a/docs/docs/api/rpc.md +++ b/docs/docs/api/rpc.md @@ -54,7 +54,9 @@ To use an RPC function: - list_ - add - delete + - patch - SchemaInfo + - SchemaPatch --- diff --git a/mathesar/rpc/schemas.py b/mathesar/rpc/schemas.py index 890f5c5747..66bd1e4b97 100644 --- a/mathesar/rpc/schemas.py +++ b/mathesar/rpc/schemas.py @@ -10,6 +10,7 @@ from db.schemas.operations.create import create_schema from db.schemas.operations.select import get_schemas from db.schemas.operations.drop import drop_schema_via_oid +from db.schemas.operations.alter import patch_schema from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions from mathesar.rpc.utils import connect @@ -30,6 +31,16 @@ class SchemaInfo(TypedDict): table_count: int +class SchemaPatch(TypedDict): + """ + Attributes: + name: The name of the schema + description: A description of the schema + """ + name: Optional[str] + description: Optional[str] + + @rpc_method(name="schemas.add") @http_basic_auth_login_required @handle_rpc_exceptions @@ -82,13 +93,30 @@ def list_(*, database_id: int, **kwargs) -> list[SchemaInfo]: @rpc_method(name="schemas.delete") @http_basic_auth_login_required @handle_rpc_exceptions -def delete(*, schema_id: int, database_id: int, **kwargs) -> None: +def delete(*, schema_oid: int, database_id: int, **kwargs) -> None: """ Delete a schema, given its OID. Args: - schema_id: The OID of the schema to delete. + schema_oid: 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) + drop_schema_via_oid(conn, schema_oid) + + +@rpc_method(name="schemas.patch") +@http_basic_auth_login_required +@handle_rpc_exceptions +def patch(*, schema_oid: int, database_id: int, patch: SchemaPatch, **kwargs) -> None: + """ + Patch a schema, given its OID. + + Args: + schema_oid: The OID of the schema to delete. + database_id: The Django id of the database containing the schema. + patch: A SchemaPatch object containing the fields to update. + """ + with connect(database_id, kwargs.get(REQUEST_KEY).user) as conn: + patch_schema(schema_oid, conn, patch) + diff --git a/mathesar/tests/rpc/test_endpoints.py b/mathesar/tests/rpc/test_endpoints.py index c290cac843..d51ac78fa9 100644 --- a/mathesar/tests/rpc/test_endpoints.py +++ b/mathesar/tests/rpc/test_endpoints.py @@ -59,6 +59,11 @@ "schemas.delete", [user_is_authenticated] ), + ( + schemas.patch, + "schemas.patch", + [user_is_authenticated] + ), ( tables.list_, "tables.list", diff --git a/mathesar/utils/models.py b/mathesar/utils/models.py index 6072f95bd5..85904a0847 100644 --- a/mathesar/utils/models.py +++ b/mathesar/utils/models.py @@ -7,7 +7,7 @@ from rest_framework.exceptions import ValidationError from db.tables.operations.alter import alter_table, SUPPORTED_TABLE_ALTER_ARGS -from db.schemas.operations.alter import alter_schema, SUPPORTED_SCHEMA_ALTER_ARGS +from db.schemas.operations.alter import patch_schema_via_sql_alchemy from db.columns.exceptions import InvalidTypeError from mathesar.api.exceptions.error_codes import ErrorCodes @@ -47,12 +47,12 @@ def update_sa_schema(schema, validated_data): ErrorCodes.UnsupportedAlter.value, message=f'Updating {arg} for schema is not supported.' ) - for arg in set(validated_data) - SUPPORTED_SCHEMA_ALTER_ARGS] + for arg in set(validated_data) - {'name', 'description'}] if errors: raise base_api_exceptions.GenericAPIException(errors, status_code=status.HTTP_400_BAD_REQUEST) if errors: raise ValidationError(errors) - alter_schema(schema.name, schema._sa_engine, validated_data) + patch_schema_via_sql_alchemy(schema.name, schema._sa_engine, validated_data) def ensure_cached_engine_ready(engine):