sadpandajoe commented on code in PR #43808:
URL: https://github.com/apache/superset/pull/43808#discussion_r3937771672
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -773,6 +773,97 @@ def test_import_dataset_no_folder(mocker: MockerFixture,
session: Session) -> No
assert sqla_table.folders is None
+def test_import_dataset_skips_has_table_check_without_data_uri(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Importing a dataset with no ``data`` URI should never call
+ ``Database.has_table`` - its result only ever gates ``load_data``, which
+ is already a no-op when there's no data URI to load. Skipping the call
+ avoids an unnecessary round trip to the target database on every
+ imported dataset, which is what made bulk imports of many datasets slow
+ enough to hit the gunicorn worker timeout.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ has_table = mocker.patch.object(Database, "has_table")
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = {
+ "table_name": "no_data_table",
+ "main_dttm_col": None,
+ "description": None,
+ "default_endpoint": None,
+ "offset": 0,
+ "cache_timeout": None,
+ "schema": None,
+ "sql": None,
+ "params": None,
+ "template_params": None,
+ "filter_select_enabled": False,
+ "fetch_values_predicate": None,
+ "extra": None,
+ "uuid": uuid.uuid4(),
+ "metrics": [],
+ "columns": [],
+ "database_uuid": database.uuid,
+ "database_id": database.id,
+ }
+
+ import_dataset(config)
+
+ has_table.assert_not_called()
+
+
+def test_import_dataset_checks_has_table_with_data_uri(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ When a ``data`` URI is present, ``Database.has_table`` should still be
+ consulted to decide whether the data needs to be (re-)loaded.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ has_table = mocker.patch.object(Database, "has_table", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = {
+ "table_name": "has_data_table",
+ "main_dttm_col": None,
+ "description": None,
+ "default_endpoint": None,
+ "offset": 0,
+ "cache_timeout": None,
+ "schema": None,
+ "sql": None,
+ "params": None,
+ "template_params": None,
+ "filter_select_enabled": False,
+ "fetch_values_predicate": None,
+ "extra": None,
+ "uuid": uuid.uuid4(),
+ "metrics": [],
+ "columns": [],
+ "database_uuid": database.uuid,
+ "database_id": database.id,
+ "data": "https://example.com/data.csv";,
+ }
+
+ import_dataset(config)
+
+ has_table.assert_called_once()
Review Comment:
If `has_table()` returns `False`, this import should still load the supplied
data, but this test returns `True` and never reaches `load_data`. An
inverted/reindented condition would therefore pass the new coverage while
skipping initialization of a missing table. Could this patch `load_data` and
assert one call for the absent-table case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
