sadpandajoe commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4128824796
##########
superset/jinja_context.py:
##########
@@ -1002,6 +1039,23 @@ def get_template_context(self, **kwargs: Any) ->
dict[str, Any]:
kwargs.update(self._context)
return validate_template_context(self.engine, kwargs)
+ def get_undefined_parameters(self, sql: str) -> set[str]:
+ """The template references ``process_template`` was unable to resolve
+
+ An unprovided parameter is left in place by ``DebugUndefined`` rather
+ than raising, so rendered SQL can still carry ``{{ name }}``. Parsing
+ the rendered SQL names what was left behind.
+
+ SQL comments are stripped first, so a parameter the author commented
+ out is not reported as missing. Stripping them parses the SQL, so SQL
+ that does not parse raises ``SupersetParseError`` from here rather than
+ being reported as having no undefined parameter.
+ """
+ stripped = SQLScript(sql, self._database.db_engine_spec.engine).format(
Review Comment:
This relies on `SQLScript(sql, engine).format(comments=False)` to strip SQL
comments so a `{{ }}` reference that's commented out isn't reported as missing,
but `KustoKQLStatement.format()` ignores its `comments` argument and always
returns the text unstripped. A Kusto query with a legitimately commented-out
reference (e.g. `StormEvents | take 1 // {{ ds }}`) would report the reference
as an undefined parameter and refuse the estimate/run even though it's inert.
Should `get_undefined_parameters` account for an engine whose comment stripping
is a no-op?
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -163,21 +168,78 @@ def run(
) -> list[dict[str, Any]]:
self.validate()
- sql = self._sql
- if self._template_params:
- # Access is already checked in validate() before any rendering.
- template_processor = get_template_processor(self._database)
- try:
- sql = template_processor.process_template(sql,
**self._template_params)
- except TemplateError as ex:
- raise SupersetErrorException(
- SupersetError(
- message=str(ex),
- error_type=SupersetErrorType.GENERIC_COMMAND_ERROR,
- level=ErrorLevel.ERROR,
+ # Rendered whether or not `template_params` was supplied, the way
+ # `validate()` above already jinja-processes for authorization and the
+ # execution path does in `SqlQueryRenderImpl.render`. A query needs no
+ # declared parameter to need rendering -- `get_time_filter()`,
+ # `current_username()`, `url_param()` take none -- and SQL Lab posts an
+ # empty `template_params` for an estimate, so those never rendered.
+ # The execution path builds its processor from the SQL Lab query, which
+ # is where a macro resolving an unqualified table (`latest_partition`)
+ # reads the schema and catalog from. Nothing is persisted here, so an
+ # unpersisted query carries just those two; the processor reads nothing
+ # else from it, and without a `database` it has no relationship that
+ # could attach it to a session.
+ template_processor = get_template_processor(
+ self._database,
+ query=Query(schema=self._schema or None, catalog=self._catalog),
+ )
+ # Both calls sit inside the `TemplateError` catch, as they do in
+ # `SqlQueryRenderImpl.render`: `get_undefined_parameters` parses the
+ # rendered SQL with Jinja, so a parameter whose *value* carries
+ # malformed Jinja raises from there too, and reads the same to the
+ # caller as one raised while rendering.
+ try:
+ sql = template_processor.process_template(
+ self._sql, **self._template_params
+ )
+ # A parameter left unresolved makes the estimate describe a
+ # different query than the one Run would execute, so it is
+ # reported the way `SqlQueryRenderImpl._validate` reports it.
+ undefined_parameters = sorted(
+ template_processor.get_undefined_parameters(sql)
+ )
+ except TemplateError as ex:
+ raise SupersetErrorException(
+ SupersetError(
+ message=str(ex),
+ error_type=SupersetErrorType.GENERIC_COMMAND_ERROR,
+ level=ErrorLevel.ERROR,
+ ),
+ status=400,
+ ) from ex
+
+ if undefined_parameters:
+ raise SupersetErrorException(
+ SupersetError(
+ message=(
+ f"{undefined_parameters_message(undefined_parameters)}
"
+ f"{str(PARAMETER_MISSING_ERR)}"
),
- status=400,
- ) from ex
+ error_type=SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={
+ "undefined_parameters": undefined_parameters,
Review Comment:
This `MISSING_TEMPLATE_PARAMS_ERROR` omits `issue_codes`, unlike the
equivalent error on the execution path (`superset/sqllab/query_render.py`'s
`_raise_undefined_parameter_exception`), which includes `extra["issue_codes"]`.
The shared frontend `ParameterErrorMessage` component (registered for this
error type in `setupErrorMessages.ts`) reads `extra.issue_codes.length`
unconditionally, so any caller that routes this response through the shared
error-message registry instead of the Estimate button's own plain-text display
would throw. Could the extra payload include the same `issue_codes` entry for
consistency with the Run path?
##########
tests/unit_tests/commands/sql_lab/test_estimate.py:
##########
@@ -577,3 +617,402 @@ 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
+# ---------------------------------------------------------------------------
+
+
+@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True)
+@patch("superset.commands.sql_lab.estimate.app")
+@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_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.
+
+ Rendered for real -- a mocked processor returning a fixed string would pass
+ whether or not the command rendered anything."""
+ mock_app.config = {
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_QUERY_COST_ESTIMATE_TIMEOUT": 10,
+ "QUERY_COST_FORMATTERS_BY_ENGINE": {},
+ }
+ mock_database = MagicMock()
+ # Real strings: the processor selects on `backend` and `SQLScript` parses
+ # with `engine`; left as mocks both silently fall back to a default.
+ mock_database.backend = "postgresql"
+ 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
+
+ sql = "{% set tf = get_time_filter('ds') %}SELECT 1 {% if tf %}{% endif %}"
+ command = QueryEstimationCommand(_make_params(sql=sql))
+
+ assert command.run() == [{"Cost": "1"}]
+ estimated =
mock_database.db_engine_spec.estimate_query_cost.call_args.args[3]
+ assert "{%" not in estimated
+ assert estimated.strip() == "SELECT 1"
+
+
+@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True)
+@patch("superset.commands.sql_lab.estimate.app")
+@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_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.backend = "postgresql"
+ 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
+
+ command = QueryEstimationCommand(
+ _make_params(sql="SELECT '{{ ds }}'", template_params={"ds":
"2026-08-20"})
+ )
+
+ assert command.run() == [{"Cost": "2"}]
+ # The parameter really was substituted, not merely passed along.
+ assert (
+ mock_database.db_engine_spec.estimate_query_cost.call_args.args[3]
+ == "SELECT '2026-08-20'"
+ )
+
+
+@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True)
+@patch("superset.commands.sql_lab.estimate.app")
+@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_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.
+
+ Rendered and detected for real: a mocked processor handed the answer would
+ prove only that the command reacts to a non-empty set."""
+ 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.backend = "postgresql"
+ 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
+
+ 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()
+
+
+@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True)
+@patch("superset.commands.sql_lab.estimate.app")
+@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_app: MagicMock,
+) -> None:
+ """SQL that fails to parse with nothing undefined in it keeps the parser's
+ own error -- the query really is malformed.
+
+ It raises from ``get_undefined_parameters``, which parses to strip
comments,
+ before the security controls parse it again; the type reaching the caller
is
+ the same either way. Mocking the processor would move the raise to a place
+ production never reaches it from."""
+ 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.backend = "postgresql"
+ 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
+
+ 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
+ mock_database.db_engine_spec.estimate_query_cost.assert_not_called()
+
+
+# ---------------------------------------------------------------------------
+# 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:
+ """The rendered SQL, not the source, is what reaches the second check.
+
+ A call-shape test: the processor is mocked so that the two strings are
+ unmistakably different, which is the only thing asserted here. It says
+ nothing about *why* a render can differ from its source -- exercising that
+ would need a nondeterministic template actually rendered, and the
+ divergence it protects against is covered by
+ ``test_run_refuses_a_render_the_caller_cannot_access`` through the real
+ gate."""
+ 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 rendered_tbl"
+ processor.get_undefined_parameters.return_value = set()
+
+ sql = "SELECT * FROM source_tbl"
+ command = QueryEstimationCommand(_make_params(sql=sql, schema="public"))
+
+ assert command.run() == [{"Cost": "1"}]
+
+ first, second = mock_security_manager.raise_for_access.call_args_list
+ # The first check is the unrendered source, as before.
+ assert first.kwargs["sql"] == sql
+ # The second is the rendered SQL that goes on to be estimated, with no
+ # template params left to expand it differently.
+ assert second.kwargs["sql"] == "SELECT * FROM rendered_tbl"
+ assert "template_params" not in second.kwargs
+ assert second.kwargs["force_dataset_match"] is True
+ assert (
+ mock_database.db_engine_spec.estimate_query_cost.call_args.args[3]
+ == "SELECT * FROM rendered_tbl"
+ )
+
+
+@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_gives_the_processor_the_query_location(
Review Comment:
This test mocks `get_template_processor` entirely, so it doesn't exercise a
real `PrestoTemplateProcessor` resolving
`latest_partition`/`latest_sub_partition` against the propagated schema/catalog
through an actual `run()` call; `latest_sub_partition` isn't covered by any
test added in this PR. A regression in processor factory selection or
sub-partition catalog propagation could estimate against the wrong table while
every current test still passes. Could a test exercise `run()` with a real
Presto processor and a mocked partition lookup, covering `latest_sub_partition`
too?
--
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]
