Update table PATCH endpoint to work with columns #578

kgodey · 2021-08-18T15:04:35Z

Fixes #562

The table detail API now accepts columns as a valid key in a PATCH request. It will updates names and types of columns as well as drop columns. It takes the same options as the table previews endpoint.

Note to reviewers

Please expand the test_table_api.py file, GitHub is hiding it by default as a "large diff".

Technical details

In order to get multiple operations to happen within a single transaction, we're passing connection and table objects around. This code is a little messy, but I didn't want to hold this up since it's blocking our next milestone so I created a new issue to refactor it. I also created a separate issue to look into whether we can support altering the table's name in the same request as altering columns.

Work done in this PR

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

kgodey · 2021-08-19T18:23:41Z

@eito-fis @powellc and anyone else interested in backend code: I'm working on supporting a bunch of different operations in one transaction in this PR. it seems to work, but the way I got it to work is by updating a bunch of functions to take connection_to_use and table_to_use objects and use those instead of opening a new connection or re-reflecting tables. I can see it getting hairy to use very quickly, I'd like to come up with a clean framework for when to open a new connection and when to use an existing connection and create a convention we can follow.

Any suggestions/thoughts would be appreciated

eito-fis · 2021-08-19T19:46:09Z

The sqlalchemy documentation has some suggestion for dealing with the nested transaction pattern, which seem reasonable, if not much different from what we have now.

If we're alright with digging into some of the internals, I think we could also try extending the Engine and Connection classes to handle something like a .begin_or_continue() method (or extend the normal .begin() method)? In particular, we could write a modification of the existing context manager that checks to see if conn._transaction exists and starts a new transaction accordingly. The Engine.begin() method is also very simple, so I don't think it would be much work to extend or make a copy of.

Wrt to the table, I don't have a great solution. Maybe just a function that takes a table and OID, either of which could be None, and returns a table? That way we could just replace all the reflection calls with a new function call and have minimal change.

kgodey · 2021-08-20T11:06:13Z

Thanks @eito-fis! Those are all good ideas, I'll think about it and figure out an approach.

…rovements

kgodey · 2021-08-21T15:45:23Z

Please see the following issues for follow up work related to this issue:

Refactor db code to work with existing connections #592
PATCH requests to the Table API should support changing the table's name and columns at the same time #593

kgodey · 2021-08-23T11:55:02Z

@eito-fis @powellc This is blocking @pavish so we're prioritizing merging it. Please review this even if it is merged, I'll make any code review changes in a new PR.

pavish

Merging this PR, as it is blocking work on the frontend. Review changes, if any, will be taken up as a separate PR.

eito-fis

Looks great! Just a couple small comments.

eito-fis · 2021-08-23T18:49:06Z

db/columns.py

+    return False
+
+
+def retype_column_in_connection(table, connection, engine, column_index, new_type, type_options={}):


Why do we create a new function here instead of using the connection_to_use=None, table_to_use=None pattern used elsewhere?

I was trying out different patterns to figure out which one might work better, I'll refactor this to be more consistent with the other functions.

eito-fis · 2021-08-23T18:52:48Z

db/types/alteration.py

-        columns.set_column_default(table_oid, column_index, new_default, engine)
+        default_stmt = select(text(cast_stmt))
+        new_default = str(execute_statement(engine, default_stmt, connection_to_use).first()[0])
+        columns.set_column_default(table_oid, column_index, new_default, engine, connection_to_use)


Should this also pass table_to_use?

Nice catch, thanks!

eito-fis · 2021-08-23T19:05:33Z

db/tests/test_columns.py

+        assert updated_table.columns[index].name == column_data[index - 2]['name']
+
+
+def test_batch_update_column_all_operations(engine_email_type):


Really like these tests 👍

eito-fis · 2021-08-23T19:08:21Z

mathesar/tests/views/api/test_table_api.py

+def _check_columns(actual_column_list, expected_column_list):
+    # Columns will return an extra type_options key in actual_dict
+    # so we need to check equality only for the keys in expect_dict
+    for index, column_dict in enumerate(expected_column_list):


Small nitpick, it might be nicer to iterate over a zip of the two lists.

Agreed, I'll make the change. Thanks!

Table PATCH now updates columns, but not in one transaction.

65f8090

kgodey mentioned this pull request Aug 19, 2021

WIP Add boolean type altering to API #387 #585

Closed

7 tasks

kgodey added 5 commits August 19, 2021 08:55

Merge branch 'master' into table_patch

d5c6873

Fixed issue where columns were required to create tables.

9b6f189

Modified a bunch of functions to work with an existing connection.

ff9b587

Fix broken tests.

914a1a7

Support dropping columns.

5b0e0f0

kgodey mentioned this pull request Aug 19, 2021

Duplicate tables are present with the same oid in mathesar_table #559

Closed

Handle case where both name and columns are passed in.

1593a8c

kgodey added 13 commits August 20, 2021 14:33

Merge branch 'master' into table_patch

2379cbf

Added test for columns and names, fixed failing tests, some style imp…

0155a5b

…rovements

Added test for patching columns with no change.

aa3b9c8

Added test for changing names only

42cf0c4

Added tests for changing types only

447f8be

Added tests for dropping columns

1e548ab

Added tests for changing name and type together.

8a92cba

Added tests for mixed operations, slight refactor.

cd86b12

Added tests to ensure that patches take place all-or-none

c5444e2

Add DB test for no changes

e25aeba

Add DB test for name change

0f39489

Added more DB tests to cover other cases.

3f205d1

Don't try to retype the column if it's already the correct type.

7647633

kgodey mentioned this pull request Aug 21, 2021

Refactor db code to work with existing connections #592

Closed

Merge branch 'master' into table_patch

9be4847

kgodey changed the title ~~WIP - Update table PATCH endpoint to work with columns~~ Update table PATCH endpoint to work with columns Aug 21, 2021

Fix set_default function

b0866a4

Fix URL mocking in tests

fe3cc30

kgodey marked this pull request as ready for review August 22, 2021 09:50

kgodey requested review from a team, pavish and eito-fis August 22, 2021 09:50

github-actions bot requested review from mathemancer and powellc August 22, 2021 09:51

pavish approved these changes Aug 23, 2021

View reviewed changes

pavish merged commit 7b33cbd into master Aug 23, 2021

pavish deleted the table_patch branch August 23, 2021 11:56

eito-fis reviewed Aug 23, 2021

View reviewed changes

kgodey mentioned this pull request Aug 24, 2021

Various backend fixes #600

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update table PATCH endpoint to work with columns #578

Update table PATCH endpoint to work with columns #578

kgodey commented Aug 18, 2021 •

edited

Loading

kgodey commented Aug 19, 2021

eito-fis commented Aug 19, 2021

kgodey commented Aug 20, 2021

kgodey commented Aug 21, 2021

kgodey commented Aug 23, 2021

pavish left a comment

eito-fis left a comment

eito-fis Aug 23, 2021

kgodey Aug 24, 2021

eito-fis Aug 23, 2021

kgodey Aug 24, 2021

eito-fis Aug 23, 2021

eito-fis Aug 23, 2021

kgodey Aug 24, 2021

		return False


		def retype_column_in_connection(table, connection, engine, column_index, new_type, type_options={}):

		assert updated_table.columns[index].name == column_data[index - 2]['name']


		def test_batch_update_column_all_operations(engine_email_type):

Update table PATCH endpoint to work with columns #578

Update table PATCH endpoint to work with columns #578

Conversation

kgodey commented Aug 18, 2021 • edited Loading

Note to reviewers

Technical details

Work done in this PR

Checklist

Developer Certificate of Origin

kgodey commented Aug 19, 2021

eito-fis commented Aug 19, 2021

kgodey commented Aug 20, 2021

kgodey commented Aug 21, 2021

kgodey commented Aug 23, 2021

pavish left a comment

Choose a reason for hiding this comment

eito-fis left a comment

Choose a reason for hiding this comment

eito-fis Aug 23, 2021

Choose a reason for hiding this comment

kgodey Aug 24, 2021

Choose a reason for hiding this comment

eito-fis Aug 23, 2021

Choose a reason for hiding this comment

kgodey Aug 24, 2021

Choose a reason for hiding this comment

eito-fis Aug 23, 2021

Choose a reason for hiding this comment

eito-fis Aug 23, 2021

Choose a reason for hiding this comment

kgodey Aug 24, 2021

Choose a reason for hiding this comment

kgodey commented Aug 18, 2021 •

edited

Loading