From 03247651fd25319c9ccce139f719a0aca4d46905 Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 00:25:01 +0530 Subject: [PATCH 01/10] implement table.patch with docs and tests --- db/sql/00_msar.sql | 19 ++++++++++++- db/tables/operations/alter.py | 20 ++++++++++++++ docs/docs/api/rpc.md | 2 ++ mathesar/rpc/tables.py | 29 +++++++++++++++++++- mathesar/tests/rpc/test_endpoints.py | 5 ++++ mathesar/tests/rpc/test_tables.py | 40 ++++++++++++++++++++++++++++ 6 files changed, 113 insertions(+), 2 deletions(-) diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index a18dabb1c4..fbc8dcaf1a 100644 --- a/db/sql/00_msar.sql +++ b/db/sql/00_msar.sql @@ -1121,7 +1121,24 @@ SELECT __msar.comment_on_table( $$ LANGUAGE SQL RETURNS NULL ON NULL INPUT; --- Alter Table: LEFT IN PYTHON (for now) ----------------------------------------------------------- +-- Alter table ------------------------------------------------------------------------------------- +CREATE OR REPLACE FUNCTION +msar.alter_table(tab_id oid, tab_alters jsonb) RETURNS text AS $$ +DECLARE + new_tab_name text; + comment text; + col_alters jsonb; +BEGIN + new_tab_name := tab_alters->>'name'; + comment := tab_alters->>'description'; + col_alters := tab_alters->>'columns'; + PERFORM msar.rename_table(tab_id, new_tab_name); + PERFORM msar.comment_on_table(tab_id, comment); + PERFORM msar.alter_columns(tab_id, col_alters); + RETURN tab_id::regclass::text; +END; +$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; + ---------------------------------------------------------------------------------------------------- ---------------------------------------------------------------------------------------------------- diff --git a/db/tables/operations/alter.py b/db/tables/operations/alter.py index 7ecb160163..d1d554bc29 100644 --- a/db/tables/operations/alter.py +++ b/db/tables/operations/alter.py @@ -50,6 +50,26 @@ def alter_table(table_name, table_oid, schema, engine, update_data): batch_update_columns(table_oid, engine, update_data['columns']) +def alter_table_on_database(table_oid, table_data_dict, conn): + """ + Alter the name, description, or columns of a table, returning name of the altered table. + + Args: + table_oid: The OID of the table to be altered. + table_data_dict: A dict describing the alterations to make. + + table_data_dict should have the form: + { + "name": , + "description": , + "columns": of column_data describing columns to alter. + } + """ + return db_conn.exec_msar_func( + conn, 'alter_table', table_oid, table_data_dict + ).fetchone()[0] + + def update_pk_sequence_to_latest(engine, table, connection=None): """ Update the primary key sequence to the current maximum. diff --git a/docs/docs/api/rpc.md b/docs/docs/api/rpc.md index d749847879..c93d6e7db0 100644 --- a/docs/docs/api/rpc.md +++ b/docs/docs/api/rpc.md @@ -64,7 +64,9 @@ To use an RPC function: - get - add - delete + - patch - TableInfo + - SettableTableInfo --- diff --git a/mathesar/rpc/tables.py b/mathesar/rpc/tables.py index ec586a4129..09a243ecaf 100644 --- a/mathesar/rpc/tables.py +++ b/mathesar/rpc/tables.py @@ -6,7 +6,8 @@ from db.tables.operations.select import get_table_info, get_table from db.tables.operations.drop import drop_table_from_database from db.tables.operations.create import create_table_on_database -from mathesar.rpc.columns import CreateableColumnInfo +from db.tables.operations.alter import alter_table_on_database +from mathesar.rpc.columns import CreateableColumnInfo, SettableColumnInfo from mathesar.rpc.constraints import CreateableConstraintInfo from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions from mathesar.rpc.utils import connect @@ -28,6 +29,12 @@ class TableInfo(TypedDict): description: Optional[str] +class SettableTableInfo(TypedDict): + name: Optional[str] + description: Optional[str] + columns: Optional[list[SettableColumnInfo]] + + @rpc_method(name="tables.list") @http_basic_auth_login_required @handle_rpc_exceptions @@ -125,3 +132,23 @@ def delete( user = kwargs.get(REQUEST_KEY).user with connect(database_id, user) as conn: return drop_table_from_database(table_oid, conn, cascade) + + +@rpc_method(name="tables.patch") +@http_basic_auth_login_required +@handle_rpc_exceptions +def patch(*, table_oid: str, table_data_dict: SettableTableInfo, database_id: int, **kwargs): + """ + Alter details of preexisting tables in a database. + + Args: + table_oid: Identity of the table whose name, description or columns we'll modify. + table_data_dict: A list describing desired table alterations. + database_id: The Django id of the database containing the table. + + Returns: + The name of the altered table. + """ + user = kwargs.get(REQUEST_KEY).user + with connect(database_id, user) as conn: + return alter_table_on_database(table_oid, table_data_dict, conn) diff --git a/mathesar/tests/rpc/test_endpoints.py b/mathesar/tests/rpc/test_endpoints.py index 6542295a72..3644b808b5 100644 --- a/mathesar/tests/rpc/test_endpoints.py +++ b/mathesar/tests/rpc/test_endpoints.py @@ -68,6 +68,11 @@ tables.delete, "tables.delete", [user_is_authenticated] + ), + ( + tables.patch, + "tables.patch", + [user_is_authenticated] ) ] diff --git a/mathesar/tests/rpc/test_tables.py b/mathesar/tests/rpc/test_tables.py index 27b678cc54..76da0bbb1d 100644 --- a/mathesar/tests/rpc/test_tables.py +++ b/mathesar/tests/rpc/test_tables.py @@ -152,3 +152,43 @@ def mock_table_add(table_name, _schema_oid, conn, column_data_list, constraint_d monkeypatch.setattr(tables, 'create_table_on_database', mock_table_add) actual_table_oid = tables.add(table_name='newtable', schema_oid=2200, database_id=11, request=request) assert actual_table_oid == 1964474 + + +def test_tables_patch(rf, monkeypatch): + request = rf.post('/api/rpc/v0', data={}) + request.user = User(username='alice', password='pass1234') + table_oid = 1964474 + database_id = 11 + table_data_dict = { + "name": "newtabname", + "description": "this is a description", + "columns": {} + } + + @contextmanager + def mock_connect(_database_id, user): + if _database_id == database_id and user.username == 'alice': + try: + yield True + finally: + pass + else: + raise AssertionError('incorrect parameters passed') + + def mock_table_patch(_table_oid, _table_data_dict, conn): + if _table_oid != table_oid and _table_data_dict != table_data_dict: + raise AssertionError('incorrect parameters passed') + return 'newtabname' + monkeypatch.setattr(tables, 'connect', mock_connect) + monkeypatch.setattr(tables, 'alter_table_on_database', mock_table_patch) + altered_table_name = tables.patch( + table_oid=1964474, + table_data_dict={ + "name": "newtabname", + "description": "this is a description", + "columns": {} + }, + database_id=11, + request=request + ) + assert altered_table_name == 'newtabname' From 52c45f0a6fedc4955421e208458397b137248950 Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 00:31:14 +0530 Subject: [PATCH 02/10] add return value to function signature --- mathesar/rpc/tables.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mathesar/rpc/tables.py b/mathesar/rpc/tables.py index 09a243ecaf..166acdf680 100644 --- a/mathesar/rpc/tables.py +++ b/mathesar/rpc/tables.py @@ -137,7 +137,9 @@ def delete( @rpc_method(name="tables.patch") @http_basic_auth_login_required @handle_rpc_exceptions -def patch(*, table_oid: str, table_data_dict: SettableTableInfo, database_id: int, **kwargs): +def patch( + *, table_oid: str, table_data_dict: SettableTableInfo, database_id: int, **kwargs +) -> int: """ Alter details of preexisting tables in a database. From 905e5f901e23cf51b498ff0a2221024cd677cf9b Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 00:32:56 +0530 Subject: [PATCH 03/10] alter return from int->str --- mathesar/rpc/tables.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mathesar/rpc/tables.py b/mathesar/rpc/tables.py index 166acdf680..3e3a5e3cf6 100644 --- a/mathesar/rpc/tables.py +++ b/mathesar/rpc/tables.py @@ -139,7 +139,7 @@ def delete( @handle_rpc_exceptions def patch( *, table_oid: str, table_data_dict: SettableTableInfo, database_id: int, **kwargs -) -> int: +) -> str: """ Alter details of preexisting tables in a database. From 5b835fb58700eabdaace2925c27eb3c91459e10a Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 01:13:25 +0530 Subject: [PATCH 04/10] fix comment_on_table sql funtions for null input and add docstring for funtions --- db/sql/00_msar.sql | 27 ++++++++++++++++++++++----- mathesar/rpc/tables.py | 17 ++++++++++++++++- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index fbc8dcaf1a..7cd4fbc0bc 100644 --- a/db/sql/00_msar.sql +++ b/db/sql/00_msar.sql @@ -1089,8 +1089,12 @@ Args: tab_name: The qualified, quoted name of the table whose comment we will change. comment_: The new comment. Any quotes or special characters must be escaped. */ -SELECT __msar.exec_ddl('COMMENT ON TABLE %s IS %s', tab_name, comment_); -$$ LANGUAGE SQL RETURNS NULL ON NULL INPUT; +DECLARE + comment_or_null text := COALESCE(comment_, 'NULL'); +BEGIN +RETURN __msar.exec_ddl('COMMENT ON TABLE %s IS %s', tab_name, comment_or_null); +END; +$$ LANGUAGE plpgsql; CREATE OR REPLACE FUNCTION @@ -1102,7 +1106,7 @@ Args: comment_: The new comment. */ SELECT __msar.comment_on_table(__msar.get_relation_name(tab_id), quote_literal(comment_)); -$$ LANGUAGE SQL RETURNS NULL ON NULL INPUT; +$$ LANGUAGE SQL; CREATE OR REPLACE FUNCTION @@ -1118,12 +1122,25 @@ SELECT __msar.comment_on_table( msar.get_fully_qualified_object_name(sch_name, tab_name), quote_literal(comment_) ); -$$ LANGUAGE SQL RETURNS NULL ON NULL INPUT; +$$ LANGUAGE SQL; -- Alter table ------------------------------------------------------------------------------------- CREATE OR REPLACE FUNCTION -msar.alter_table(tab_id oid, tab_alters jsonb) RETURNS text AS $$ +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. + +Args: + tab_id: The OID of the table whose columns we'll alter. + tab_alters: a JSONB describing the alterations to make. + + The tab_alters should have the form: + { + "name": , + "description": + "columns": , + } +*/ DECLARE new_tab_name text; comment text; diff --git a/mathesar/rpc/tables.py b/mathesar/rpc/tables.py index 3e3a5e3cf6..6a5ceaadac 100644 --- a/mathesar/rpc/tables.py +++ b/mathesar/rpc/tables.py @@ -30,6 +30,21 @@ class TableInfo(TypedDict): class SettableTableInfo(TypedDict): + """ + Information about a table, restricted to settable fields. + + When possible, Passing `null` for a key will clear the underlying + setting. E.g., + + - `description = null` clears the table description. + + Setting any of `name`, `columns` to `null` is a noop. + + Attributes: + name: The new name of the table. + description: The description of the table. + columns: A list describing desired column alterations. + """ name: Optional[str] description: Optional[str] columns: Optional[list[SettableColumnInfo]] @@ -141,7 +156,7 @@ def patch( *, table_oid: str, table_data_dict: SettableTableInfo, database_id: int, **kwargs ) -> str: """ - Alter details of preexisting tables in a database. + Alter details of a preexisting table in a database. Args: table_oid: Identity of the table whose name, description or columns we'll modify. From 07a6bdd7d2a8de7a1935f2e0210c89c8475626d6 Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 18:18:14 +0530 Subject: [PATCH 05/10] use get_relation_name_or_null --- db/sql/00_msar.sql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index 7cd4fbc0bc..bb15f962d3 100644 --- a/db/sql/00_msar.sql +++ b/db/sql/00_msar.sql @@ -1057,7 +1057,7 @@ Args: new_tab_name: unquoted, unqualified table name */ BEGIN - RETURN __msar.rename_table(__msar.get_relation_name(tab_id), quote_ident(new_tab_name)); + RETURN __msar.rename_table(msar.get_relation_name_or_null(tab_id), quote_ident(new_tab_name)); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; @@ -1105,7 +1105,7 @@ Args: tab_id: The OID of the table whose comment we will change. comment_: The new comment. */ -SELECT __msar.comment_on_table(__msar.get_relation_name(tab_id), quote_literal(comment_)); +SELECT __msar.comment_on_table(msar.get_relation_name_or_null(tab_id), quote_literal(comment_)); $$ LANGUAGE SQL; @@ -1152,7 +1152,7 @@ BEGIN PERFORM msar.rename_table(tab_id, new_tab_name); PERFORM msar.comment_on_table(tab_id, comment); PERFORM msar.alter_columns(tab_id, col_alters); - RETURN tab_id::regclass::text; + RETURN msar.get_relation_name_or_null(tab_id); END; $$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; From 2bfaddeda486e54630f58c4ab43e46510a71f629 Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 18:32:42 +0530 Subject: [PATCH 06/10] add mock test for alter_table --- db/tests/tables/operations/test_alter.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/db/tests/tables/operations/test_alter.py b/db/tests/tables/operations/test_alter.py index 993e307364..754a588956 100644 --- a/db/tests/tables/operations/test_alter.py +++ b/db/tests/tables/operations/test_alter.py @@ -29,3 +29,17 @@ def test_comment_on_table(engine_with_schema): assert call_args[2] == schema_name assert call_args[3] == "comment_on_me" assert call_args[4] == "This is a comment" + + +def test_alter_table(): + with patch.object(tab_alter.db_conn, 'exec_msar_func') as mock_exec: + tab_alter.alter_table_on_database( + 12345, + {"name": "newname", "description": "this is a comment", "columns": {}}, + "conn" + ) + call_args = mock_exec.call_args_list[0][0] + assert call_args[0] == "conn" + assert call_args[1] == "alter_table" + assert call_args[2] == 12345 + assert call_args[3] == {"name": "newname", "description": "this is a comment", "columns": {}} From 43dc1f0e6e7d871e1b21b1b9ec7a3d170dda167e Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Wed, 12 Jun 2024 18:36:43 +0530 Subject: [PATCH 07/10] use -> instead of ->> for columns --- db/sql/00_msar.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index bb15f962d3..a6ee233ad2 100644 --- a/db/sql/00_msar.sql +++ b/db/sql/00_msar.sql @@ -1148,7 +1148,7 @@ DECLARE BEGIN new_tab_name := tab_alters->>'name'; comment := tab_alters->>'description'; - col_alters := tab_alters->>'columns'; + col_alters := tab_alters->'columns'; PERFORM msar.rename_table(tab_id, new_tab_name); PERFORM msar.comment_on_table(tab_id, comment); PERFORM msar.alter_columns(tab_id, col_alters); From c6f994cd1adeb58fd514ac0f28ac51553ebdb71d Mon Sep 17 00:00:00 2001 From: Sean Colsen Date: Wed, 12 Jun 2024 14:13:03 -0400 Subject: [PATCH 08/10] Add schemas.add RPC function --- conftest.py | 4 +- db/schemas/operations/alter.py | 6 +- db/schemas/operations/create.py | 55 ++++++++++---- db/sql/00_msar.sql | 86 +++++++++++++--------- db/sql/test_00_msar.sql | 33 +++++++-- db/tables/operations/infer_types.py | 4 +- db/tests/schemas/operations/test_create.py | 16 ++-- db/types/install.py | 8 +- docs/docs/api/rpc.md | 1 + mathesar/rpc/schemas.py | 41 ++++++++--- mathesar/tests/imports/test_csv.py | 4 +- mathesar/tests/imports/test_json.py | 4 +- mathesar/tests/rpc/test_endpoints.py | 5 ++ mathesar/utils/schemas.py | 4 +- 14 files changed, 178 insertions(+), 93 deletions(-) diff --git a/conftest.py b/conftest.py index 6089145050..05cd0978ba 100644 --- a/conftest.py +++ b/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, {}) diff --git a/db/schemas/operations/alter.py b/db/schemas/operations/alter.py index e6b345720f..919997eff0 100644 --- a/db/schemas/operations/alter.py +++ b/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: diff --git a/db/schemas/operations/create.py b/db/schemas/operations/create.py index a079ec2cd3..8be771c362 100644 --- a/db/schemas/operations/create.py +++ b/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] diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index a18dabb1c4..31689896a7 100644 --- a/db/sql/00_msar.sql +++ b/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. @@ -612,7 +615,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; @@ -885,8 +888,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; @@ -902,8 +905,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_)); @@ -916,7 +919,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_)); @@ -932,43 +935,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; ---------------------------------------------------------------------------------------------------- diff --git a/db/sql/test_00_msar.sql b/db/sql/test_00_msar.sql index cb11d87e8e..db10bb73e5 100644 --- a/db/sql/test_00_msar.sql +++ b/db/sql/test_00_msar.sql @@ -1183,17 +1183,38 @@ $$ LANGUAGE plpgsql; -- msar.schema_ddl -------------------------------------------------------------------------------- -CREATE OR REPLACE FUNCTION test_create_schema() RETURNS SETOF TEXT AS $$ +CREATE OR REPLACE FUNCTION test_create_schema_without_description() RETURNS SETOF TEXT AS $$ +DECLARE sch_oid oid; BEGIN - PERFORM msar.create_schema( - sch_name => 'create_schema'::text, - if_not_exists => false - ); - RETURN NEXT has_schema('create_schema'); + SELECT msar.create_schema('foo bar') INTO sch_oid; + RETURN NEXT has_schema('foo bar'); + RETURN NEXT is(sch_oid, msar.get_schema_oid('foo bar')); + RETURN NEXT is(obj_description(sch_oid), NULL); +END; +$$ LANGUAGE plpgsql; + + +CREATE OR REPLACE FUNCTION test_create_schema_with_description() RETURNS SETOF TEXT AS $$ +DECLARE sch_oid oid; +BEGIN + SELECT msar.create_schema('foo bar', 'yay') INTO sch_oid; + RETURN NEXT has_schema('foo bar'); + RETURN NEXT is(sch_oid, msar.get_schema_oid('foo bar')); + RETURN NEXT is(obj_description(sch_oid), 'yay'); END; $$ LANGUAGE plpgsql; +CREATE OR REPLACE FUNCTION test_create_schema_that_already_exists() RETURNS SETOF TEXT AS $t$ +DECLARE sch_oid oid; +BEGIN + SELECT msar.create_schema('foo bar') INTO sch_oid; + RETURN NEXT throws_ok($$SELECT msar.create_schema('foo bar')$$, '42P06'); + RETURN NEXT is(msar.create_schema_if_not_exists('foo bar'), sch_oid); +END; +$t$ LANGUAGE plpgsql; + + CREATE OR REPLACE FUNCTION __setup_drop_schema() RETURNS SETOF TEXT AS $$ BEGIN CREATE SCHEMA drop_test_schema; diff --git a/db/tables/operations/infer_types.py b/db/tables/operations/infer_types.py index e7d3c522a3..11c571103c 100644 --- a/db/tables/operations/infer_types.py +++ b/db/tables/operations/infer_types.py @@ -5,7 +5,7 @@ from db import constants from db.columns.base import MathesarColumn from db.columns.operations.infer_types import infer_column_type -from db.schemas.operations.create import create_schema +from db.schemas.operations.create import create_schema_if_not_exists_via_sql_alchemy from db.tables.operations.create import CreateTableAs from db.tables.operations.select import reflect_table from db.types.operations.convert import get_db_type_enum_from_class @@ -43,7 +43,7 @@ def infer_table_column_types(schema, table_name, engine, metadata=None, columns_ table = reflect_table(table_name, schema, engine, metadata=metadata) temp_name = TEMP_TABLE % (int(time())) - create_schema(TEMP_SCHEMA, engine, if_not_exists=True) + create_schema_if_not_exists_via_sql_alchemy(TEMP_SCHEMA, engine) with engine.begin() as conn: while engine.dialect.has_table(conn, temp_name, schema=TEMP_SCHEMA): temp_name = TEMP_TABLE.format(int(time())) diff --git a/db/tests/schemas/operations/test_create.py b/db/tests/schemas/operations/test_create.py index 8c6d796f80..5ca649fc21 100644 --- a/db/tests/schemas/operations/test_create.py +++ b/db/tests/schemas/operations/test_create.py @@ -1,22 +1,16 @@ -import pytest from unittest.mock import patch -import db.schemas.operations.create as sch_create +from db.schemas.operations.create import create_schema_via_sql_alchemy -@pytest.mark.parametrize( - "if_not_exists", [(True), (False), (None)] -) -def test_create_schema(engine_with_schema, if_not_exists): +def test_create_schema_via_sql_alchemy(engine_with_schema): engine = engine_with_schema - with patch.object(sch_create, 'execute_msar_func_with_engine') as mock_exec: - sch_create.create_schema( + with patch.object(create_schema_via_sql_alchemy, 'execute_msar_func_with_engine') as mock_exec: + create_schema_via_sql_alchemy( schema_name='new_schema', engine=engine, comment=None, - if_not_exists=if_not_exists ) call_args = mock_exec.call_args_list[0][0] assert call_args[0] == engine - assert call_args[1] == "create_schema" + assert call_args[1] == "create_schema_via_sql_alchemy" assert call_args[2] == "new_schema" - assert call_args[3] == if_not_exists or False diff --git a/db/types/install.py b/db/types/install.py index 5651af1eed..7c1dfd92d6 100644 --- a/db/types/install.py +++ b/db/types/install.py @@ -1,12 +1,12 @@ from db.types.custom import email, money, multicurrency, uri, json_array, json_object from db.constants import TYPES_SCHEMA -from db.schemas.operations.create import create_schema +from db.schemas.operations.create import create_schema_if_not_exists_via_sql_alchemy from db.types.operations.cast import install_all_casts import psycopg -def create_type_schema(engine): - create_schema(TYPES_SCHEMA, engine, if_not_exists=True) +def create_type_schema(engine) -> None: + create_schema_if_not_exists_via_sql_alchemy(TYPES_SCHEMA, engine) def install_mathesar_on_database(engine): @@ -24,4 +24,6 @@ def install_mathesar_on_database(engine): def uninstall_mathesar_from_database(engine): conn_str = str(engine.url) with psycopg.connect(conn_str) as conn: + # TODO: Clean up this code so that it references all the schemas in our + # `INTERNAL_SCHEMAS` constant. conn.execute(f"DROP SCHEMA IF EXISTS __msar, msar, {TYPES_SCHEMA} CASCADE") diff --git a/docs/docs/api/rpc.md b/docs/docs/api/rpc.md index d749847879..911805ae19 100644 --- a/docs/docs/api/rpc.md +++ b/docs/docs/api/rpc.md @@ -52,6 +52,7 @@ To use an RPC function: options: members: - list_ + - add - delete - SchemaInfo diff --git a/mathesar/rpc/schemas.py b/mathesar/rpc/schemas.py index caf7f3fc43..890f5c5747 100644 --- a/mathesar/rpc/schemas.py +++ b/mathesar/rpc/schemas.py @@ -7,6 +7,7 @@ from modernrpc.auth.basic import http_basic_auth_login_required from db.constants import INTERNAL_SCHEMAS +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 mathesar.rpc.exceptions.handlers import handle_rpc_exceptions @@ -22,13 +23,40 @@ class SchemaInfo(TypedDict): name: The name of the schema description: A description of the schema table_count: The number of tables in the schema - exploration_count: The number of explorations in the schema """ oid: int name: str description: Optional[str] table_count: int - exploration_count: int + + +@rpc_method(name="schemas.add") +@http_basic_auth_login_required +@handle_rpc_exceptions +def add( + *, + name: str, + database_id: int, + description: Optional[str] = None, + **kwargs, +) -> int: + """ + Add a schema + + Args: + name: The name of the schema to add. + database_id: The Django id of the database containing the schema. + description: A description of the schema + + Returns: + The integer OID of the schema created + """ + with connect(database_id, kwargs.get(REQUEST_KEY).user) as conn: + return create_schema( + schema_name=name, + conn=conn, + description=description + ) @rpc_method(name="schemas.list") @@ -42,18 +70,13 @@ def list_(*, database_id: int, **kwargs) -> list[SchemaInfo]: database_id: The Django id of the database containing the table. Returns: - A list of schema details + A list of SchemaInfo objects """ user = kwargs.get(REQUEST_KEY).user with connect(database_id, user) as conn: schemas = get_schemas(conn) - user_defined_schemas = [s for s in schemas if s['name'] not in INTERNAL_SCHEMAS] - - # TODO_FOR_BETA: join exploration count from internal DB here after we've - # 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] + return [s for s in schemas if s['name'] not in INTERNAL_SCHEMAS] @rpc_method(name="schemas.delete") diff --git a/mathesar/tests/imports/test_csv.py b/mathesar/tests/imports/test_csv.py index a7b860df8d..ea88f53607 100644 --- a/mathesar/tests/imports/test_csv.py +++ b/mathesar/tests/imports/test_csv.py @@ -7,7 +7,7 @@ from mathesar.errors import InvalidTableError from mathesar.imports.base import create_table_from_data_file from mathesar.imports.csv import get_sv_dialect, get_sv_reader -from db.schemas.operations.create import create_schema +from db.schemas.operations.create import create_schema_via_sql_alchemy from db.schemas.utils import get_schema_oid_from_name from db.constants import COLUMN_NAME_TEMPLATE from psycopg.errors import DuplicateTable @@ -45,7 +45,7 @@ def col_headers_empty_data_file(col_headers_empty_csv_filepath): @pytest.fixture() def schema(engine, test_db_model): - create_schema(TEST_SCHEMA, engine) + create_schema_via_sql_alchemy(TEST_SCHEMA, engine) schema_oid = get_schema_oid_from_name(TEST_SCHEMA, engine) yield Schema.current_objects.create(oid=schema_oid, database=test_db_model) with engine.begin() as conn: diff --git a/mathesar/tests/imports/test_json.py b/mathesar/tests/imports/test_json.py index b099e48680..a35b47dc74 100644 --- a/mathesar/tests/imports/test_json.py +++ b/mathesar/tests/imports/test_json.py @@ -5,7 +5,7 @@ from mathesar.models.base import DataFile, Schema from mathesar.imports.base import create_table_from_data_file -from db.schemas.operations.create import create_schema +from db.schemas.operations.create import create_schema_via_sql_alchemy from db.schemas.utils import get_schema_oid_from_name from psycopg.errors import DuplicateTable @@ -21,7 +21,7 @@ def data_file(patents_json_filepath): @pytest.fixture() def schema(engine, test_db_model): - create_schema(TEST_SCHEMA, engine) + create_schema_via_sql_alchemy(TEST_SCHEMA, engine) schema_oid = get_schema_oid_from_name(TEST_SCHEMA, engine) yield Schema.current_objects.create(oid=schema_oid, database=test_db_model) with engine.begin() as conn: diff --git a/mathesar/tests/rpc/test_endpoints.py b/mathesar/tests/rpc/test_endpoints.py index 6542295a72..c0854ad2fd 100644 --- a/mathesar/tests/rpc/test_endpoints.py +++ b/mathesar/tests/rpc/test_endpoints.py @@ -39,6 +39,11 @@ "connections.add_from_scratch", [user_is_superuser] ), + ( + schemas.add, + "schemas.add", + [user_is_authenticated] + ), ( schemas.list_, "schemas.list", diff --git a/mathesar/utils/schemas.py b/mathesar/utils/schemas.py index 1d12c0060b..0552745704 100644 --- a/mathesar/utils/schemas.py +++ b/mathesar/utils/schemas.py @@ -1,7 +1,7 @@ from django.core.exceptions import ObjectDoesNotExist from rest_framework.exceptions import ValidationError -from db.schemas.operations.create import create_schema +from db.schemas.operations.create import create_schema_via_sql_alchemy from db.schemas.utils import get_schema_oid_from_name, get_mathesar_schemas from mathesar.database.base import create_mathesar_engine from mathesar.models.base import Schema, Database @@ -19,7 +19,7 @@ def create_schema_and_object(name, connection_id, comment=None): all_schemas = get_mathesar_schemas(engine) if name in all_schemas: raise ValidationError({"name": f"Schema name {name} is not unique"}) - create_schema(name, engine, comment=comment) + create_schema_via_sql_alchemy(name, engine, comment) schema_oid = get_schema_oid_from_name(name, engine) schema = Schema.objects.create(oid=schema_oid, database=database_model) From 05eb1769f6a997bd0f060fd11907fb7818891a7d Mon Sep 17 00:00:00 2001 From: Sean Colsen Date: Wed, 12 Jun 2024 14:30:03 -0400 Subject: [PATCH 09/10] Fix failing test --- db/tests/schemas/operations/test_create.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/db/tests/schemas/operations/test_create.py b/db/tests/schemas/operations/test_create.py index 5ca649fc21..9452b95e1a 100644 --- a/db/tests/schemas/operations/test_create.py +++ b/db/tests/schemas/operations/test_create.py @@ -1,16 +1,16 @@ from unittest.mock import patch -from db.schemas.operations.create import create_schema_via_sql_alchemy +import db.schemas.operations.create as sch_create def test_create_schema_via_sql_alchemy(engine_with_schema): engine = engine_with_schema - with patch.object(create_schema_via_sql_alchemy, 'execute_msar_func_with_engine') as mock_exec: - create_schema_via_sql_alchemy( + with patch.object(sch_create, 'execute_msar_func_with_engine') as mock_exec: + sch_create.create_schema_via_sql_alchemy( schema_name='new_schema', engine=engine, - comment=None, + description=None, ) call_args = mock_exec.call_args_list[0][0] assert call_args[0] == engine - assert call_args[1] == "create_schema_via_sql_alchemy" + assert call_args[1] == "create_schema" assert call_args[2] == "new_schema" From c4ee70c9745e758a3655d70dd996d5476fe0ca06 Mon Sep 17 00:00:00 2001 From: Anish Umale Date: Fri, 14 Jun 2024 02:56:30 +0530 Subject: [PATCH 10/10] fix inaccurate docstring --- db/sql/00_msar.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/sql/00_msar.sql b/db/sql/00_msar.sql index a6ee233ad2..c6693b00a6 100644 --- a/db/sql/00_msar.sql +++ b/db/sql/00_msar.sql @@ -1128,7 +1128,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.