bito-code-review[bot] commented on code in PR #41127:
URL: https://github.com/apache/superset/pull/41127#discussion_r3566979568
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -193,6 +194,107 @@ def mock_serialize_payload(payload, use_msgpack):
)
+def test_execute_sql_statements_mutates_before_split_by_default(
+ mocker: MockerFixture, app: SupersetApp
+) -> None:
+ """
+ With the default `MUTATE_AFTER_SPLIT=False`, `execute_sql_statements`
should
+ mutate the whole, un-split query once before splitting it into individual
+ statement blocks, for engines that execute statements individually rather
+ than as one. Regression guard for issue #30169.
+ """
+ query = mocker.MagicMock()
+ query.limit = 1
+ query.database = mocker.MagicMock()
+ query.database.cache_timeout = 100
+ query.status = "RUNNING"
+ query.select_as_cta = False
+ query.database.allow_run_async = True
+ query.database.db_engine_spec.engine = "sqlite"
+ query.database.db_engine_spec.run_multiple_statements_as_one = False
+ query.database.db_engine_spec.allows_sql_comments = True
+
+ mutate_mock = mocker.patch.object(
+ query.database,
+ "mutate_sql_based_on_config",
+ side_effect=lambda sql, **kw: sql,
+ )
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("sys.getsizeof", return_value=10000000)
+ mocker.patch(
+ "superset.sql_lab._serialize_payload",
+ side_effect=lambda payload, use_msgpack: "serialized_payload",
+ )
+ mocker.patch("superset.sql_lab.db.session.refresh", return_value=None)
+ mocker.patch("superset.sql_lab.results_backend", return_value=True)
+
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="SELECT 1; SELECT 2;",
+ return_results=True,
+ store_results=True,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
+
+ is_split_values = [
+ call.kwargs.get("is_split") for call in mutate_mock.call_args_list
+ ]
+ # The mutator is called once on the whole, un-split query before
splitting...
+ assert is_split_values[0] is False
+ first_call_sql = mutate_mock.call_args_list[0].args[0]
+ assert "1" in first_call_sql
+ assert "2" in first_call_sql
+ # Both statements are present in a single, un-split call.
+ assert first_call_sql.count("SELECT") == 2
+ # ...and once again per already-split statement (a no-op when
+ # `MUTATE_AFTER_SPLIT=False`, since `is_split=True` won't match the
config).
+ assert all(value is True for value in is_split_values[1:])
+ assert len(is_split_values) == 3
+
+
+def test_execute_sql_statements_raises_when_mutator_strips_all_statements(
+ mocker: MockerFixture, app: SupersetApp
+) -> None:
+ """
+ A `SQL_QUERY_MUTATOR` that strips a query down to nothing (e.g. only
+ comments/whitespace) must raise a clean error instead of silently
+ producing an empty block list.
+ """
+ query = mocker.MagicMock()
+ query.limit = 1
+ query.database = mocker.MagicMock()
+ query.database.cache_timeout = 100
+ query.status = "RUNNING"
+ query.select_as_cta = False
+ query.database.allow_run_async = True
+ query.database.db_engine_spec.engine = "sqlite"
+ query.database.db_engine_spec.run_multiple_statements_as_one = False
+ query.database.db_engine_spec.allows_sql_comments = True
+
+ mocker.patch.object(
+ query.database,
+ "mutate_sql_based_on_config",
+ side_effect=lambda sql, **kw: "-- just a comment",
+ )
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("superset.sql_lab.db.session.refresh", return_value=None)
+
+ with pytest.raises(SupersetErrorException):
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="SELECT 1;",
+ return_results=True,
+ store_results=True,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing results_backend mock</b></div>
<div id="fix">
The test at line 258 sets `query.database.allow_run_async = True` (line
272), which triggers the guard at `sql_lab.py:413` that checks
`database.allow_run_async and not results_backend`. Without mocking
`results_backend` to return a truthy value, the function raises
`SupersetResultsBackendNotConfigureException` instead of reaching the
empty-statement guard and raising the expected `SupersetErrorException`.
Compare with existing tests at lines 119, 178, and 230 — all three mock
`results_backend` when `allow_run_async = True`. The new test is missing this
critical mock.
</div>
</div>
<small><i>Code Review Run #3e6ba3</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
