From df73d07590fe1aa64f489516ca74f5613ed7db46 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Mon, 10 Jan 2022 19:28:08 +0000 Subject: [PATCH 01/11] Assign an owner when creating a dataset from a csv, excel or columnar --- superset/views/database/views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/views/database/views.py b/superset/views/database/views.py index 0f2d0f6c2d76..a739a035a1ad 100644 --- a/superset/views/database/views.py +++ b/superset/views/database/views.py @@ -223,7 +223,7 @@ def form_post(self, form: CsvToDatabaseForm) -> Response: sqla_table = SqlaTable(table_name=csv_table.table) sqla_table.database = expore_database sqla_table.database_id = database.id - sqla_table.user_id = g.user.get_id() + sqla_table.owners = [g.user] sqla_table.schema = csv_table.schema sqla_table.fetch_metadata() db.session.add(sqla_table) @@ -369,7 +369,7 @@ def form_post(self, form: ExcelToDatabaseForm) -> Response: sqla_table = SqlaTable(table_name=excel_table.table) sqla_table.database = expore_database sqla_table.database_id = database.id - sqla_table.user_id = g.user.get_id() + sqla_table.owners = [g.user] sqla_table.schema = excel_table.schema sqla_table.fetch_metadata() db.session.add(sqla_table) @@ -521,7 +521,7 @@ def form_post( # pylint: disable=too-many-locals sqla_table = SqlaTable(table_name=columnar_table.table) sqla_table.database = expore_database sqla_table.database_id = database.id - sqla_table.user_id = g.user.get_id() + sqla_table.owners = [g.user] sqla_table.schema = columnar_table.schema sqla_table.fetch_metadata() db.session.add(sqla_table) From 5ee2c31fa087dc35ca4ec223af565da2679dfe83 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Mon, 24 Jan 2022 21:23:08 +0000 Subject: [PATCH 02/11] Added some unit tests --- tests/integration_tests/csv_upload_tests.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 59ffd05e7835..309907e121d4 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -28,6 +28,7 @@ import pytest from superset.sql_parse import Table +from superset import security_manager from tests.integration_tests.conftest import ADMIN_SCHEMA_NAME from tests.integration_tests.test_app import app # isort:skip from superset import db @@ -362,6 +363,10 @@ def test_import_csv(mock_event_logger): data = engine.execute(f"SELECT * from {CSV_UPLOAD_TABLE}").fetchall() assert data == [("john", 1, "x"), ("paul", 2, None)] + # ensure user is assigned as an owner + assert security_manager.find_user("admin") in table.owners + + @pytest.mark.usefixtures("setup_csv_upload") @pytest.mark.usefixtures("create_excel_files") @@ -419,6 +424,10 @@ def test_import_excel(mock_event_logger): ) assert data == [(0, "john", 1), (1, "paul", 2)] + # ensure user is assigned as an owner + table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) + assert security_manager.find_user("admin") in table.owners + @pytest.mark.usefixtures("setup_csv_upload") @pytest.mark.usefixtures("create_columnar_files") @@ -498,3 +507,8 @@ def test_import_parquet(mock_event_logger): .fetchall() ) assert data == [("john", 1), ("paul", 2), ("max", 3), ("bob", 4)] + + # ensure user is assigned as an owner + table = SupersetTestCase.get_table(name=PARQUET_UPLOAD_TABLE) + assert security_manager.find_user("admin") in table.owners + From eda41ac1c4bfc2fcdb0811824b1519aadff5dc63 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Mon, 24 Jan 2022 21:57:09 +0000 Subject: [PATCH 03/11] Code cleanup --- tests/integration_tests/csv_upload_tests.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 309907e121d4..5dcc29d58365 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -346,6 +346,9 @@ def test_import_csv(mock_event_logger): # make sure the new column name is reflected in the table metadata assert "d" in table.column_names + # ensure user is assigned as an owner + assert security_manager.find_user("admin") in table.owners + # null values are set upload_csv( CSV_FILENAME2, @@ -363,9 +366,6 @@ def test_import_csv(mock_event_logger): data = engine.execute(f"SELECT * from {CSV_UPLOAD_TABLE}").fetchall() assert data == [("john", 1, "x"), ("paul", 2, None)] - # ensure user is assigned as an owner - assert security_manager.find_user("admin") in table.owners - @pytest.mark.usefixtures("setup_csv_upload") @@ -475,10 +475,13 @@ def test_import_parquet(mock_event_logger): ) assert success_msg_f1 in resp - # make sure only specified column name was read table = SupersetTestCase.get_table(name=PARQUET_UPLOAD_TABLE, schema=None) + # make sure only specified column name was read assert "b" not in table.column_names + # ensure user is assigned as an owner + assert security_manager.find_user("admin") in table.owners + # upload again with replace mode resp = upload_columnar( PARQUET_FILENAME1, PARQUET_UPLOAD_TABLE, extra={"if_exists": "replace"} @@ -507,8 +510,3 @@ def test_import_parquet(mock_event_logger): .fetchall() ) assert data == [("john", 1), ("paul", 2), ("max", 3), ("bob", 4)] - - # ensure user is assigned as an owner - table = SupersetTestCase.get_table(name=PARQUET_UPLOAD_TABLE) - assert security_manager.find_user("admin") in table.owners - From 19120fb5ee81dff3e7f51d3fa5f5a9c3ed58d105 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Mon, 24 Jan 2022 21:58:17 +0000 Subject: [PATCH 04/11] Removed blank line --- tests/integration_tests/csv_upload_tests.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 5dcc29d58365..bfa40d33f863 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -367,7 +367,6 @@ def test_import_csv(mock_event_logger): assert data == [("john", 1, "x"), ("paul", 2, None)] - @pytest.mark.usefixtures("setup_csv_upload") @pytest.mark.usefixtures("create_excel_files") @mock.patch("superset.db_engine_specs.hive.upload_to_s3", mock_upload_to_s3) From 41f74b0f640985a80ac75d47369a5c3f5942234d Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Tue, 25 Jan 2022 22:07:46 +0000 Subject: [PATCH 05/11] Attempt to fix a broken test --- tests/integration_tests/csv_upload_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index bfa40d33f863..268d6a5bd0d7 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -424,7 +424,7 @@ def test_import_excel(mock_event_logger): assert data == [(0, "john", 1), (1, "paul", 2)] # ensure user is assigned as an owner - table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) + table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE, schema=None) assert security_manager.find_user("admin") in table.owners From 7e982946680ab5ffc8a04ef349d8cc714e7458a5 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Wed, 26 Jan 2022 16:06:31 +0000 Subject: [PATCH 06/11] Attempt # 2 to fix a broken test --- tests/integration_tests/csv_upload_tests.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 268d6a5bd0d7..9431e6e3b3f4 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -424,8 +424,9 @@ def test_import_excel(mock_event_logger): assert data == [(0, "john", 1), (1, "paul", 2)] # ensure user is assigned as an owner - table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE, schema=None) - assert security_manager.find_user("admin") in table.owners + # Disabling the following to troubleshoot a broken test upstream + # table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE, schema=None) + # assert security_manager.find_user("admin") in table.owners @pytest.mark.usefixtures("setup_csv_upload") From e073e9cec2e8db1e07fe632455d32866ca2ccbe3 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Wed, 26 Jan 2022 17:05:57 +0000 Subject: [PATCH 07/11] Attempt # 3 to fix a broken test --- tests/integration_tests/csv_upload_tests.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 9431e6e3b3f4..1eda3a53ff77 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -391,6 +391,10 @@ def test_import_excel(mock_event_logger): table=EXCEL_UPLOAD_TABLE, ) + # ensure user is assigned as an owner + table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) + assert security_manager.find_user("admin") in table.owners + # upload again with fail mode; should fail fail_msg = f'Unable to upload Excel file "{EXCEL_FILENAME}" to table "{EXCEL_UPLOAD_TABLE}"' resp = upload_excel(EXCEL_FILENAME, EXCEL_UPLOAD_TABLE) @@ -423,11 +427,6 @@ def test_import_excel(mock_event_logger): ) assert data == [(0, "john", 1), (1, "paul", 2)] - # ensure user is assigned as an owner - # Disabling the following to troubleshoot a broken test upstream - # table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE, schema=None) - # assert security_manager.find_user("admin") in table.owners - @pytest.mark.usefixtures("setup_csv_upload") @pytest.mark.usefixtures("create_columnar_files") From 74c5a8dbb43dcd8d00814b5e4979f1c144c5ca27 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Wed, 26 Jan 2022 19:06:34 +0000 Subject: [PATCH 08/11] Attempt # 4 to fix a broken test --- tests/integration_tests/csv_upload_tests.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 1eda3a53ff77..90f580dc6465 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -142,15 +142,20 @@ def upload_csv(filename: str, table_name: str, extra: Optional[Dict[str, str]] = def upload_excel( filename: str, table_name: str, extra: Optional[Dict[str, str]] = None ): + + csv_upload_db_id = get_upload_db().id + schema = utils.get_example_default_schema() form_data = { "excel_file": open(filename, "rb"), "name": table_name, - "con": get_upload_db().id, + "con": csv_upload_db_id, "sheet_name": "Sheet1", "if_exists": "fail", "index_label": "test_label", "mangle_dupe_cols": False, } + if schema: + form_data["schema"] = schema if extra: form_data.update(extra) return get_resp(test_client, "/exceltodatabaseview/form", data=form_data) @@ -391,10 +396,6 @@ def test_import_excel(mock_event_logger): table=EXCEL_UPLOAD_TABLE, ) - # ensure user is assigned as an owner - table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) - assert security_manager.find_user("admin") in table.owners - # upload again with fail mode; should fail fail_msg = f'Unable to upload Excel file "{EXCEL_FILENAME}" to table "{EXCEL_UPLOAD_TABLE}"' resp = upload_excel(EXCEL_FILENAME, EXCEL_UPLOAD_TABLE) @@ -427,6 +428,10 @@ def test_import_excel(mock_event_logger): ) assert data == [(0, "john", 1), (1, "paul", 2)] + # ensure user is assigned as an owner + table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) + assert security_manager.find_user("admin") in table.owners + @pytest.mark.usefixtures("setup_csv_upload") @pytest.mark.usefixtures("create_columnar_files") From e47814c9343f85e594c82e4dd4ae11da3af1ab39 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Wed, 26 Jan 2022 19:42:31 +0000 Subject: [PATCH 09/11] Attempt # 5 to fix a broken test --- tests/integration_tests/csv_upload_tests.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 90f580dc6465..9c3d121fa572 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -142,13 +142,12 @@ def upload_csv(filename: str, table_name: str, extra: Optional[Dict[str, str]] = def upload_excel( filename: str, table_name: str, extra: Optional[Dict[str, str]] = None ): - - csv_upload_db_id = get_upload_db().id + excel_upload_db_id = get_upload_db().id schema = utils.get_example_default_schema() form_data = { "excel_file": open(filename, "rb"), "name": table_name, - "con": csv_upload_db_id, + "con": excel_upload_db_id, "sheet_name": "Sheet1", "if_exists": "fail", "index_label": "test_label", From 37f18e99f2e79a64134b620ae697fe6b3322f571 Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Wed, 26 Jan 2022 22:10:13 +0000 Subject: [PATCH 10/11] Attempt # 6 to fix a broken test --- tests/integration_tests/csv_upload_tests.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 9c3d121fa572..0e14ff0c0882 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -379,10 +379,12 @@ def test_import_excel(mock_event_logger): if utils.backend() == "hive": pytest.skip("Hive doesn't excel upload.") + schema = utils.get_example_default_schema() + full_table_name = f"{schema}.{EXCEL_UPLOAD_TABLE}" if schema else EXCEL_UPLOAD_TABLE test_db = get_upload_db() success_msg = ( - f'Excel file "{EXCEL_FILENAME}" uploaded to table "{EXCEL_UPLOAD_TABLE}"' + f'Excel file "{EXCEL_FILENAME}" uploaded to table "{full_table_name}"' ) # initial upload with fail mode @@ -391,7 +393,7 @@ def test_import_excel(mock_event_logger): mock_event_logger.assert_called_with( action="successful_excel_upload", database=test_db.name, - schema=None, + schema=schema, table=EXCEL_UPLOAD_TABLE, ) @@ -415,7 +417,7 @@ def test_import_excel(mock_event_logger): mock_event_logger.assert_called_with( action="successful_excel_upload", database=test_db.name, - schema=None, + schema=schema, table=EXCEL_UPLOAD_TABLE, ) From 394119276256c7df9832892d84eb889762f6901c Mon Sep 17 00:00:00 2001 From: Joel Gregoire Date: Wed, 26 Jan 2022 22:35:43 +0000 Subject: [PATCH 11/11] Broken test fixed, code cleanup --- tests/integration_tests/csv_upload_tests.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/integration_tests/csv_upload_tests.py b/tests/integration_tests/csv_upload_tests.py index 0e14ff0c0882..95ae3e49e2f5 100644 --- a/tests/integration_tests/csv_upload_tests.py +++ b/tests/integration_tests/csv_upload_tests.py @@ -383,9 +383,7 @@ def test_import_excel(mock_event_logger): full_table_name = f"{schema}.{EXCEL_UPLOAD_TABLE}" if schema else EXCEL_UPLOAD_TABLE test_db = get_upload_db() - success_msg = ( - f'Excel file "{EXCEL_FILENAME}" uploaded to table "{full_table_name}"' - ) + success_msg = f'Excel file "{EXCEL_FILENAME}" uploaded to table "{full_table_name}"' # initial upload with fail mode resp = upload_excel(EXCEL_FILENAME, EXCEL_UPLOAD_TABLE) @@ -397,6 +395,10 @@ def test_import_excel(mock_event_logger): table=EXCEL_UPLOAD_TABLE, ) + # ensure user is assigned as an owner + table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) + assert security_manager.find_user("admin") in table.owners + # upload again with fail mode; should fail fail_msg = f'Unable to upload Excel file "{EXCEL_FILENAME}" to table "{EXCEL_UPLOAD_TABLE}"' resp = upload_excel(EXCEL_FILENAME, EXCEL_UPLOAD_TABLE) @@ -429,10 +431,6 @@ def test_import_excel(mock_event_logger): ) assert data == [(0, "john", 1), (1, "paul", 2)] - # ensure user is assigned as an owner - table = SupersetTestCase.get_table(name=EXCEL_UPLOAD_TABLE) - assert security_manager.find_user("admin") in table.owners - @pytest.mark.usefixtures("setup_csv_upload") @pytest.mark.usefixtures("create_columnar_files")