mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4073042785
##########
tests/unit_tests/commands/sql_lab/test_estimate.py:
##########
@@ -30,8 +32,11 @@
OAuth2RedirectError,
SupersetErrorException,
SupersetGenericDBErrorException,
+ SupersetParseError,
SupersetSecurityException,
)
+from superset.models.core import Database
+from tests.unit_tests.conftest import with_feature_flags # noqa: E402
Review Comment:
Removed in 48b81a9. You're right that it never fired — ruff is clean on the
file without it. I added the suppression without checking whether anything was
complaining, which is the wrong order.
##########
tests/unit_tests/commands/sql_lab/test_estimate.py:
##########
@@ -577,3 +617,447 @@ def
test_run_reraises_oauth2_redirect_error_from_cost_estimation(
command.run()
assert exc_info.value.status == 403
+
+
+# ---------------------------------------------------------------------------
+# Templates are rendered before estimating, as on the execution path
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_renders_a_template_without_template_params(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """A query needs no declared parameter to need rendering:
``get_time_filter()``
+ and friends take none, and SQL Lab posts an empty ``template_params`` for
an
+ estimate, so gating the render on it left the template in place for the
+ parser to choke on."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_database.db_engine_spec.query_cost_formatter.return_value = [{"Cost":
"1"}]
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT 1"
+ processor.get_undefined_parameters.return_value = set()
+
+ sql = "{% set tf = get_time_filter('ds') %}SELECT 1 {% if tf %}{% endif %}"
+ command = QueryEstimationCommand(_make_params(sql=sql))
+
+ assert command.run() == [{"Cost": "1"}]
+
mock_get_template_processor.return_value.process_template.assert_called_once_with(
+ sql
+ )
+ # What reaches the engine is the rendered SQL.
+ assert (
+ mock_database.db_engine_spec.estimate_query_cost.call_args.args[3] ==
"SELECT 1"
+ )
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_estimates_a_template_its_parameters_fully_bind(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """A template whose parameters are all supplied renders to the same SQL the
+ query would run, so it is estimated rather than refused."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_database.db_engine_spec.query_cost_formatter.return_value = [{"Cost":
"2"}]
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT '2026-08-20'"
+ processor.get_undefined_parameters.return_value = set()
+
+ command = QueryEstimationCommand(
+ _make_params(sql="SELECT '{{ ds }}'", template_params={"ds":
"2026-08-20"})
+ )
+
+ assert command.run() == [{"Cost": "2"}]
+
mock_get_template_processor.return_value.process_template.assert_called_once_with(
+ "SELECT '{{ ds }}'", ds="2026-08-20"
+ )
+ # What reaches the engine is the rendered SQL, not the template.
+ assert (
+ mock_database.db_engine_spec.estimate_query_cost.call_args.args[3]
+ == "SELECT '2026-08-20'"
+ )
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_reports_an_unprovided_parameter_as_missing(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """``DebugUndefined`` leaves an unprovided parameter in place instead of
+ raising, and in a position like a string literal the leftover still parses.
+ Estimating it would describe a query the user cannot run, so it gets the
+ same typed response the execution path gives it."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT '{{ ds }}' AS d"
+ processor.get_undefined_parameters.return_value = {"ds"}
+
+ command = QueryEstimationCommand(_make_params(sql="SELECT '{{ ds }}' AS
d"))
+ with pytest.raises(SupersetErrorException) as exc_info:
+ command.run()
+
+ error = exc_info.value.error
+ assert exc_info.value.status == 400
+ assert error.error_type == SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR
+ assert error.message.startswith('The parameter "ds" in your query is
undefined.')
+ # The execution path's suggestion travels with it.
+ assert "Set Parameters" in error.message
+ assert error.extra["undefined_parameters"] == ["ds"]
+ assert error.extra["issue_codes"][0]["code"] == 1006
+ # Nothing was estimated.
+ mock_database.db_engine_spec.estimate_query_cost.assert_not_called()
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_leaves_a_genuine_syntax_error_alone(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """SQL that fails to parse with nothing undefined in it keeps the parser's
+ own error -- the query really is malformed."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT FROM FROM"
+ processor.get_undefined_parameters.return_value = set()
+
+ command = QueryEstimationCommand(_make_params(sql="SELECT FROM FROM"))
+ with pytest.raises(SupersetParseError) as exc_info:
+ command.run()
+
+ assert exc_info.value.error.error_type ==
SupersetErrorType.INVALID_SQL_ERROR
+
+
+# ---------------------------------------------------------------------------
+# What is authorized is what is estimated
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_reauthorizes_the_rendered_sql(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """``validate()`` authorizes a render of its own, and a template need not
+ render the same way twice. The SQL that will be estimated is authorized as
+ a literal, as ``_validate_rendered_access`` does on the execution path."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_database.db_engine_spec.query_cost_formatter.return_value = [{"Cost":
"1"}]
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT * FROM allowed_ds"
+ processor.get_undefined_parameters.return_value = set()
+
+ sql = "SELECT * FROM {{ ['allowed_ds', 'secret_tbl'] | random }}"
Review Comment:
This one was the useful catch. The template was decoration: with
`process_template` mocked to a fixed string, nothing about `random` — or about
rendering at all — was exercised, while the docstring described exactly that.
Fixed in 48b81a9 by taking the claim out rather than dressing it up. The SQL
is now plainly different from the mocked render, the docstring says this is a
call-shape test and nothing more, and it points at
`test_run_refuses_a_render_the_caller_cannot_access`, which does run the real
authorization gate on a rendered string.
Four neighbouring tests had the same shape and are now built on a real
template processor instead. One of them was worse than a wording problem: it
asserted `SupersetParseError` arriving from `_apply_sql_security`, but with a
real processor the parse fails earlier, in `get_undefined_parameters` — it was
covering a path production cannot take. A fifth, which only asserted that a
mocked gate's exception propagates, is deleted.
Every test in this PR that claims to guard a regression now has the defect
reintroduced and confirmed failing — the render gate, the undefined-parameter
check, the pinned `executed_sql`, the query handed to the processor, and the
`schema` keyword that shadowed a user's template parameter.
##########
tests/unit_tests/commands/sql_lab/test_estimate.py:
##########
@@ -577,3 +617,447 @@ def
test_run_reraises_oauth2_redirect_error_from_cost_estimation(
command.run()
assert exc_info.value.status == 403
+
+
+# ---------------------------------------------------------------------------
+# Templates are rendered before estimating, as on the execution path
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_renders_a_template_without_template_params(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """A query needs no declared parameter to need rendering:
``get_time_filter()``
+ and friends take none, and SQL Lab posts an empty ``template_params`` for
an
+ estimate, so gating the render on it left the template in place for the
+ parser to choke on."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_database.db_engine_spec.query_cost_formatter.return_value = [{"Cost":
"1"}]
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT 1"
+ processor.get_undefined_parameters.return_value = set()
+
+ sql = "{% set tf = get_time_filter('ds') %}SELECT 1 {% if tf %}{% endif %}"
+ command = QueryEstimationCommand(_make_params(sql=sql))
+
+ assert command.run() == [{"Cost": "1"}]
+
mock_get_template_processor.return_value.process_template.assert_called_once_with(
+ sql
+ )
+ # What reaches the engine is the rendered SQL.
+ assert (
+ mock_database.db_engine_spec.estimate_query_cost.call_args.args[3] ==
"SELECT 1"
+ )
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_estimates_a_template_its_parameters_fully_bind(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """A template whose parameters are all supplied renders to the same SQL the
+ query would run, so it is estimated rather than refused."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_database.db_engine_spec.query_cost_formatter.return_value = [{"Cost":
"2"}]
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT '2026-08-20'"
+ processor.get_undefined_parameters.return_value = set()
+
+ command = QueryEstimationCommand(
+ _make_params(sql="SELECT '{{ ds }}'", template_params={"ds":
"2026-08-20"})
+ )
+
+ assert command.run() == [{"Cost": "2"}]
+
mock_get_template_processor.return_value.process_template.assert_called_once_with(
+ "SELECT '{{ ds }}'", ds="2026-08-20"
+ )
+ # What reaches the engine is the rendered SQL, not the template.
+ assert (
+ mock_database.db_engine_spec.estimate_query_cost.call_args.args[3]
+ == "SELECT '2026-08-20'"
+ )
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_reports_an_unprovided_parameter_as_missing(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """``DebugUndefined`` leaves an unprovided parameter in place instead of
+ raising, and in a position like a string literal the leftover still parses.
+ Estimating it would describe a query the user cannot run, so it gets the
+ same typed response the execution path gives it."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT '{{ ds }}' AS d"
+ processor.get_undefined_parameters.return_value = {"ds"}
+
+ command = QueryEstimationCommand(_make_params(sql="SELECT '{{ ds }}' AS
d"))
+ with pytest.raises(SupersetErrorException) as exc_info:
+ command.run()
+
+ error = exc_info.value.error
+ assert exc_info.value.status == 400
+ assert error.error_type == SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR
+ assert error.message.startswith('The parameter "ds" in your query is
undefined.')
+ # The execution path's suggestion travels with it.
+ assert "Set Parameters" in error.message
+ assert error.extra["undefined_parameters"] == ["ds"]
+ assert error.extra["issue_codes"][0]["code"] == 1006
+ # Nothing was estimated.
+ mock_database.db_engine_spec.estimate_query_cost.assert_not_called()
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_leaves_a_genuine_syntax_error_alone(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """SQL that fails to parse with nothing undefined in it keeps the parser's
+ own error -- the query really is malformed."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT FROM FROM"
+ processor.get_undefined_parameters.return_value = set()
+
+ command = QueryEstimationCommand(_make_params(sql="SELECT FROM FROM"))
+ with pytest.raises(SupersetParseError) as exc_info:
+ command.run()
+
+ assert exc_info.value.error.error_type ==
SupersetErrorType.INVALID_SQL_ERROR
+
+
+# ---------------------------------------------------------------------------
+# What is authorized is what is estimated
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.commands.sql_lab.estimate.app")
+@patch("superset.commands.sql_lab.estimate.get_template_processor")
+@patch("superset.commands.sql_lab.estimate.security_manager",
new_callable=MagicMock)
+@patch("superset.commands.sql_lab.estimate.DatabaseDAO")
+def test_run_reauthorizes_the_rendered_sql(
+ mock_dao: MagicMock,
+ mock_security_manager: MagicMock,
+ mock_get_template_processor: MagicMock,
+ mock_app: MagicMock,
+) -> None:
+ """``validate()`` authorizes a render of its own, and a template need not
+ render the same way twice. The SQL that will be estimated is authorized as
+ a literal, as ``_validate_rendered_access`` does on the execution path."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ mock_database.db_engine_spec.engine = "postgresql"
+ mock_database.allow_dml = False
+ mock_database.db_engine_spec.query_cost_formatter.return_value = [{"Cost":
"1"}]
+ mock_dao.find_by_id.return_value = mock_database
+ mock_security_manager.raise_for_access.return_value = None
+ processor = mock_get_template_processor.return_value
+ processor.process_template.return_value = "SELECT * FROM allowed_ds"
+ processor.get_undefined_parameters.return_value = set()
+
+ sql = "SELECT * FROM {{ ['allowed_ds', 'secret_tbl'] | random }}"
+ command = QueryEstimationCommand(_make_params(sql=sql, schema="public"))
+
+ assert command.run() == [{"Cost": "1"}]
+
+ first, second = mock_security_manager.raise_for_access.call_args_list
Review Comment:
Added in 48b81a9, along with the assertion you flagged last round that only
the error type was checked: the test now also asserts the jinja2 reason
survives the wrap, so a swap to a generic message fails.
--
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]
