rusackas commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3580496596
##########
tests/unit_tests/sql/execution/conftest.py:
##########
@@ -94,6 +94,10 @@ def mock_database() -> MagicMock:
database.db_engine_spec.get_cancel_query_id = MagicMock(return_value=None)
database.db_engine_spec.patch = MagicMock()
database.db_engine_spec.fetch_data = MagicMock(return_value=[])
+ # Mirrors the real `Database.mutate_sql_based_on_config` default (no-op
+ # when no `SQL_QUERY_MUTATOR` is configured), so SQL parsed from its
+ # return value stays valid instead of an un-parseable `MagicMock`.
+ database.mutate_sql_based_on_config = MagicMock(side_effect=lambda sql,
**kw: sql)
Review Comment:
Fixed — replaced the untyped lambda in `mock_database` with a typed
`_passthrough_mutate_sql_based_on_config` helper (and reused it in
`test_executor.py` too).
##########
superset/sql_lab.py:
##########
@@ -474,12 +474,55 @@ def execute_sql_statements( # noqa: C901
for statement in parsed_script.statements:
apply_limit(query, statement)
- # some databases (like BigQuery and Kusto) do not persist state across
mmultiple
+ # some databases (like BigQuery and Kusto) do not persist state across
multiple
# statements if they're run separately (especially when using `NullPool`),
so we run
# the query as a single block.
if db_engine_spec.run_multiple_statements_as_one:
- blocks =
[parsed_script.format(comments=db_engine_spec.allows_sql_comments)]
+ if app.config["MUTATE_AFTER_SPLIT"]:
+ # These engines never actually execute statements individually, so
the
+ # per-block mutation call further down (whose `is_split` is always
+ # `False` here) would never fire. Mutate each statement here,
before
+ # joining them into the single block this engine requires, so
+ # `MUTATE_AFTER_SPLIT=True` still applies the mutator per
statement.
+ blocks = [
+ ";\n".join(
+ database.mutate_sql_based_on_config(
+
statement.format(comments=db_engine_spec.allows_sql_comments),
+ is_split=True,
+ )
+ for statement in parsed_script.statements
+ )
+ ]
Review Comment:
Fixed — `blocks` now has an explicit `list[str]` annotation at its first
assignment.
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -871,6 +871,45 @@ def test_execute_applies_sql_mutator(
mutate_mock.assert_called()
[email protected]("is_split", [True, False])
+def test_execute_sql_with_cursor_forwards_is_split(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_db_session: MagicMock,
+ mock_query: MagicMock,
+ is_split: bool,
+) -> None:
+ """
+ `execute_sql_with_cursor` must forward `is_split` to the SQL mutator.
+
+ `Database.mutate_sql_based_on_config` only fires `SQL_QUERY_MUTATOR` when
+ `is_split == MUTATE_AFTER_SPLIT`, so passing the wrong value silently skips
+ mutation (the SQL Lab bug behind issue #30169). This guards the contract.
+ """
+ from superset.sql.execution.executor import execute_sql_with_cursor
+
+ mutate_mock = mocker.patch.object(
+ database, "mutate_sql_based_on_config", side_effect=lambda sql, **kw:
sql
+ )
Review Comment:
Fixed — `mutate_mock` is now typed as `MagicMock`.
##########
tests/unit_tests/sql/execution/test_executor.py:
##########
@@ -871,6 +871,45 @@ def test_execute_applies_sql_mutator(
mutate_mock.assert_called()
[email protected]("is_split", [True, False])
+def test_execute_sql_with_cursor_forwards_is_split(
+ mocker: MockerFixture,
+ database: Database,
+ app_context: None,
+ mock_db_session: MagicMock,
+ mock_query: MagicMock,
+ is_split: bool,
+) -> None:
+ """
+ `execute_sql_with_cursor` must forward `is_split` to the SQL mutator.
+
+ `Database.mutate_sql_based_on_config` only fires `SQL_QUERY_MUTATOR` when
+ `is_split == MUTATE_AFTER_SPLIT`, so passing the wrong value silently skips
+ mutation (the SQL Lab bug behind issue #30169). This guards the contract.
+ """
+ from superset.sql.execution.executor import execute_sql_with_cursor
+
+ mutate_mock = mocker.patch.object(
+ database, "mutate_sql_based_on_config", side_effect=lambda sql, **kw:
sql
+ )
+ mocker.patch.object(database.db_engine_spec, "execute")
+ mocker.patch.object(database.db_engine_spec, "fetch_data",
return_value=[(1,)])
+ mocker.patch("superset.result_set.SupersetResultSet",
return_value=MagicMock())
+
+ cursor = create_mock_cursor(["id"], data=[(1,)])
Review Comment:
Fixed — `cursor` is now typed as `MagicMock`.
--
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]
