From d97f6a80a645eba4784390c48fd320c95bc71a15 Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Tue, 11 Jun 2024 15:31:23 +0800 Subject: [PATCH 1/7] fix class name spelling --- mathesar/rpc/columns.py | 4 ++-- mathesar/rpc/constraints.py | 2 +- mathesar/rpc/tables.py | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/mathesar/rpc/columns.py b/mathesar/rpc/columns.py index 54ad70896a..537e48dd38 100644 --- a/mathesar/rpc/columns.py +++ b/mathesar/rpc/columns.py @@ -74,9 +74,9 @@ def from_dict(cls, col_default): ) -class CreateableColumnInfo(TypedDict): +class CreatableColumnInfo(TypedDict): """ - Information about adding a new column. + Information needed to add a new column. Only the `name` & `type` keys are required. diff --git a/mathesar/rpc/constraints.py b/mathesar/rpc/constraints.py index b8ebbdabc4..fe726464d0 100644 --- a/mathesar/rpc/constraints.py +++ b/mathesar/rpc/constraints.py @@ -4,5 +4,5 @@ from typing import TypedDict -class CreateableConstraintInfo(TypedDict): +class CreatableConstraintInfo(TypedDict): pass diff --git a/mathesar/rpc/tables.py b/mathesar/rpc/tables.py index ec586a4129..8c6d4575a3 100644 --- a/mathesar/rpc/tables.py +++ b/mathesar/rpc/tables.py @@ -6,8 +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 mathesar.rpc.constraints import CreateableConstraintInfo +from mathesar.rpc.columns import CreatableColumnInfo +from mathesar.rpc.constraints import CreatableConstraintInfo from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions from mathesar.rpc.utils import connect @@ -78,8 +78,8 @@ def add( table_name: str, schema_oid: int, database_id: int, - column_data_list: list[CreateableColumnInfo] = [], - constraint_data_list: list[CreateableConstraintInfo] = [], + column_data_list: list[CreatableColumnInfo] = [], + constraint_data_list: list[CreatableConstraintInfo] = [], comment: str = None, **kwargs ) -> int: From 9510b8c4cc2631772215b4e192b9a59b7e15e7ee Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Tue, 11 Jun 2024 16:40:08 +0800 Subject: [PATCH 2/7] add python wrapper for new column creation --- db/columns/operations/create.py | 78 ++++++++++++++-------- db/tests/columns/operations/test_create.py | 8 +-- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/db/columns/operations/create.py b/db/columns/operations/create.py index 7361276015..da18be26a8 100644 --- a/db/columns/operations/create.py +++ b/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,53 @@ def create_column(engine, table_oid, column_data): return curr.fetchone()[0] +def add_columns_to_table(table_oid, column_data_list, conn): + 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": , + "type": , + "type_options": , + "nullable": , + "default": {"value": } + "description": + } + + Output form: + { + "type": {"name": , "options": }, + "name": , + "not_null": , + "default": , + "description": + } + """ + 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 +93,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, diff --git a/db/tests/columns/operations/test_create.py b/db/tests/columns/operations/test_create.py index b668e71bf6..cc736a97ff 100644 --- a/db/tests/columns/operations/test_create.py +++ b/db/tests/columns/operations/test_create.py @@ -29,7 +29,7 @@ def test_create_column_name(engine_with_schema, in_name, out_name): given a (maybe empty) name param """ engine, schema = engine_with_schema - with patch.object(col_create, "execute_msar_func_with_engine") as mock_exec: + with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: col_create.create_column(engine, 12345, {"name": in_name}) call_args = mock_exec.call_args_list[0][0] assert call_args[0] == engine @@ -47,7 +47,7 @@ def test_create_column_type(engine_with_schema, in_type, out_type): given a (maybe empty) type """ engine, schema = engine_with_schema - with patch.object(col_create, "execute_msar_func_with_engine") as mock_exec: + with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: col_create.create_column(engine, 12345, {"type": in_type}) call_args = mock_exec.call_args_list[0][0] assert call_args[0] == engine @@ -68,7 +68,7 @@ def test_create_column_type_options(engine_with_schema, in_options, out_options) given a (maybe empty) type options dict. """ engine, schema = engine_with_schema - with patch.object(col_create, "execute_msar_func_with_engine") as mock_exec: + with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: col_create.create_column(engine, 12345, {"type_options": in_options}) call_args = mock_exec.call_args_list[0][0] assert call_args[0] == engine @@ -81,7 +81,7 @@ def test_create_column_type_options(engine_with_schema, in_options, out_options) def test_duplicate_column_smoke(engine_with_schema): """This is just a smoke test, since the underlying function is trivial.""" engine, schema = engine_with_schema - with patch.object(col_create, "execute_msar_func_with_engine") as mock_exec: + with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: col_create.duplicate_column( 12345, 4, From 787a07db450a7a0962245242342dacc85b28c3a6 Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Tue, 11 Jun 2024 16:59:43 +0800 Subject: [PATCH 3/7] reuse column creation tests for new function --- db/tests/columns/operations/test_create.py | 35 ++++++++++------------ 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/db/tests/columns/operations/test_create.py b/db/tests/columns/operations/test_create.py index cc736a97ff..a248fd5d49 100644 --- a/db/tests/columns/operations/test_create.py +++ b/db/tests/columns/operations/test_create.py @@ -21,38 +21,36 @@ def test_type_list_completeness(engine): @pytest.mark.parametrize( - "in_name,out_name", [('test1', 'test1'), ('', None), (None, None)] + "in_name,out_name", [("test1", "test1"), ("", None), (None, None)] ) -def test_create_column_name(engine_with_schema, in_name, out_name): +def test_add_columns_name(in_name, out_name): """ Here, we just check that the PostgreSQL function is called properly, when given a (maybe empty) name param """ - engine, schema = engine_with_schema - with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: - col_create.create_column(engine, 12345, {"name": in_name}) + with patch.object(col_create.db_conn, "exec_msar_func") as mock_exec: + col_create.add_columns_to_table(123, [{"name": in_name}], "conn") call_args = mock_exec.call_args_list[0][0] - assert call_args[0] == engine + assert call_args[0] == "conn" assert call_args[1] == "add_columns" - assert call_args[2] == 12345 + assert call_args[2] == 123 assert json.loads(call_args[3])[0]["name"] == out_name @pytest.mark.parametrize( "in_type,out_type", [("numeric", "numeric"), (None, "character varying")] ) -def test_create_column_type(engine_with_schema, in_type, out_type): +def test_add_columns_type(in_type, out_type): """ Here, we just check that the PostgreSQL function is called properly when given a (maybe empty) type """ - engine, schema = engine_with_schema - with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: - col_create.create_column(engine, 12345, {"type": in_type}) + with patch.object(col_create.db_conn, "exec_msar_func") as mock_exec: + col_create.add_columns_to_table(123, [{"type": in_type}], "conn") call_args = mock_exec.call_args_list[0][0] - assert call_args[0] == engine + assert call_args[0] == "conn" assert call_args[1] == "add_columns" - assert call_args[2] == 12345 + assert call_args[2] == 123 actual_col_data = json.loads(call_args[3])[0] assert actual_col_data["name"] is None assert actual_col_data["type"]["name"] == out_type @@ -62,18 +60,17 @@ def test_create_column_type(engine_with_schema, in_type, out_type): @pytest.mark.parametrize( "in_options,out_options", [({"foo": "bar"}, {"foo": "bar"}), (None, None), ({}, {})] ) -def test_create_column_type_options(engine_with_schema, in_options, out_options): +def test_add_columns_type_options(in_options, out_options): """ Here, we just check that the PostgreSQL function is called properly when given a (maybe empty) type options dict. """ - engine, schema = engine_with_schema - with patch.object(col_create.db_conn, "execute_msar_func_with_engine") as mock_exec: - col_create.create_column(engine, 12345, {"type_options": in_options}) + with patch.object(col_create.db_conn, "exec_msar_func") as mock_exec: + col_create.add_columns_to_table(123, [{"type_options": in_options}], "conn") call_args = mock_exec.call_args_list[0][0] - assert call_args[0] == engine + assert call_args[0] == "conn" assert call_args[1] == "add_columns" - assert call_args[2] == 12345 + assert call_args[2] == 123 assert json.loads(call_args[3])[0]["type"]["name"] == "character varying" assert json.loads(call_args[3])[0]["type"]["options"] == out_options From 9907d0417a8acbc050bfa90e30d9b7ade1249056 Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Tue, 11 Jun 2024 17:28:02 +0800 Subject: [PATCH 4/7] wire column adding up to RPC endpoint --- mathesar/rpc/columns.py | 35 +++++++++++++++++++++++++--- mathesar/tests/rpc/test_columns.py | 33 ++++++++++++++++++++++++++ mathesar/tests/rpc/test_endpoints.py | 5 ++++ 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/mathesar/rpc/columns.py b/mathesar/rpc/columns.py index 537e48dd38..a4f7d00ca4 100644 --- a/mathesar/rpc/columns.py +++ b/mathesar/rpc/columns.py @@ -7,6 +7,7 @@ from modernrpc.auth.basic import http_basic_auth_login_required from db.columns.operations.alter import alter_columns_in_table +from db.columns.operations.create import add_columns_to_table from db.columns.operations.drop import drop_columns_from_table from db.columns.operations.select import get_column_info_for_table from mathesar.rpc.exceptions.handlers import handle_rpc_exceptions @@ -78,7 +79,7 @@ class CreatableColumnInfo(TypedDict): """ Information needed to add a new column. - Only the `name` & `type` keys are required. + No keys are required. Attributes: name: The name of the column. @@ -88,8 +89,8 @@ class CreatableColumnInfo(TypedDict): default: The default value. description: The description of the column. """ - name: str - type: str + name: Optional[str] + type: Optional[str] type_options: Optional[TypeOptions] nullable: Optional[bool] default: Optional[ColumnDefault] @@ -215,6 +216,34 @@ def list_(*, table_oid: int, database_id: int, **kwargs) -> ColumnListReturn: ) +@rpc_method(name="columns.add") +@http_basic_auth_login_required +@handle_rpc_exceptions +def add( + *, + column_data_list: list[CreatableColumnInfo], + table_oid: int, + database_id: int, + **kwargs +) -> list[int]: + """ + Alter details of preexisting columns in a table. + + Does not support altering the type or type options of array columns. + + Args: + column_data_list: A list describing desired column alterations. + table_oid: Identity of the table whose columns we'll modify. + database_id: The Django id of the database containing the table. + + Returns: + The number of columns altered. + """ + user = kwargs.get(REQUEST_KEY).user + with connect(database_id, user) as conn: + return add_columns_to_table(table_oid, column_data_list, conn) + + @rpc_method(name="columns.patch") @http_basic_auth_login_required @handle_rpc_exceptions diff --git a/mathesar/tests/rpc/test_columns.py b/mathesar/tests/rpc/test_columns.py index ea9928aca0..1afd813a33 100644 --- a/mathesar/tests/rpc/test_columns.py +++ b/mathesar/tests/rpc/test_columns.py @@ -171,6 +171,39 @@ def mock_column_alter(_table_oid, _column_data_list, conn): assert actual_result == 1 +def test_columns_add(rf, monkeypatch): + request = rf.post('/api/rpc/v0/', data={}) + request.user = User(username='alice', password='pass1234') + table_oid = 23457 + database_id = 2 + column_data_list = [{"id": 3, "name": "newname"}] + + @contextmanager + def mock_connect(_database_id, user): + if _database_id == 2 and user.username == 'alice': + try: + yield True + finally: + pass + else: + raise AssertionError('incorrect parameters passed') + + def mock_column_create(_table_oid, _column_data_list, conn): + if _table_oid != table_oid or _column_data_list != column_data_list: + raise AssertionError('incorrect parameters passed') + return [3, 4] + + monkeypatch.setattr(columns, 'connect', mock_connect) + monkeypatch.setattr(columns, 'add_columns_to_table', mock_column_create) + actual_result = columns.add( + column_data_list=column_data_list, + table_oid=table_oid, + database_id=database_id, + request=request + ) + assert actual_result == [3, 4] + + def test_columns_delete(rf, monkeypatch): request = rf.post('/api/rpc/v0/', data={}) request.user = User(username='alice', password='pass1234') diff --git a/mathesar/tests/rpc/test_endpoints.py b/mathesar/tests/rpc/test_endpoints.py index 6542295a72..6db4a98248 100644 --- a/mathesar/tests/rpc/test_endpoints.py +++ b/mathesar/tests/rpc/test_endpoints.py @@ -29,6 +29,11 @@ "columns.patch", [user_is_authenticated] ), + ( + columns.add, + "columns.add", + [user_is_authenticated] + ), ( connections.add_from_known_connection, "connections.add_from_known_connection", From fb1501eed46f2f8c10ff03262182f50aeb7a00aa Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Tue, 11 Jun 2024 17:33:13 +0800 Subject: [PATCH 5/7] update docs for new column adder RPC function --- docs/docs/api/rpc.md | 1 + mathesar/rpc/columns.py | 10 ++++------ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/docs/api/rpc.md b/docs/docs/api/rpc.md index d749847879..9366a78156 100644 --- a/docs/docs/api/rpc.md +++ b/docs/docs/api/rpc.md @@ -72,6 +72,7 @@ To use an RPC function: options: members: - list_ + - add - patch - delete - ColumnListReturn diff --git a/mathesar/rpc/columns.py b/mathesar/rpc/columns.py index a4f7d00ca4..6a74a6f329 100644 --- a/mathesar/rpc/columns.py +++ b/mathesar/rpc/columns.py @@ -227,17 +227,15 @@ def add( **kwargs ) -> list[int]: """ - Alter details of preexisting columns in a table. - - Does not support altering the type or type options of array columns. + Add columns to a table. Args: - column_data_list: A list describing desired column alterations. - table_oid: Identity of the table whose columns we'll modify. + column_data_list: A list describing desired columns to add. + table_oid: Identity of the table to which we'll add columns. database_id: The Django id of the database containing the table. Returns: - The number of columns altered. + An array of the attnums of the new columns. """ user = kwargs.get(REQUEST_KEY).user with connect(database_id, user) as conn: From 899fb921153e90db0f2356f86fff262abc98acb5 Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Tue, 11 Jun 2024 17:36:11 +0800 Subject: [PATCH 6/7] document db library python function --- db/columns/operations/create.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/db/columns/operations/create.py b/db/columns/operations/create.py index da18be26a8..ddaf29d4b3 100644 --- a/db/columns/operations/create.py +++ b/db/columns/operations/create.py @@ -29,6 +29,17 @@ def create_column(engine, table_oid, column_data): 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 ] From e4ee2bede9c6607f5813ed50cbc6b280fb8d0bb7 Mon Sep 17 00:00:00 2001 From: Brent Moran Date: Thu, 13 Jun 2024 11:30:23 +0800 Subject: [PATCH 7/7] add documentation of column creation defaults --- mathesar/rpc/columns.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mathesar/rpc/columns.py b/mathesar/rpc/columns.py index 6a74a6f329..6f198d8553 100644 --- a/mathesar/rpc/columns.py +++ b/mathesar/rpc/columns.py @@ -229,6 +229,10 @@ def add( """ Add columns to a table. + There are defaults for both the name and type of a column, and so + passing `[{}]` for `column_data_list` would add a single column of + type `CHARACTER VARYING`, with an auto-generated name. + Args: column_data_list: A list describing desired columns to add. table_oid: Identity of the table to which we'll add columns.