mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4117845270
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ 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,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # 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.
+ template_processor = get_template_processor(self._database)
+ try:
+ sql = template_processor.process_template(
Review Comment:
Withdrawing the pinning, in 0aaf3a4.
It had no behavioral effect — as rusackas's trace and mine both found, with
no template params `sql=` and a pinned `executed_sql` authorize the same text —
but it was the only reason the command built a full ORM `Query` outside the
gate with `database=` set. That relationship assignment inspects the database
as a mapped instance, and three existing integration tests that pass a plain
`mock.Mock()` failed there on every backend. My unit tests used `MagicMock`,
which happens to survive it, so it only showed up in CI.
Authorization is back to `raise_for_access(database=, sql=, schema=,
catalog=, force_dataset_match=True)`, which builds its own ephemeral query
inside the gate as it always has. The rendered text is still what's authorized,
and `test_run_refuses_a_render_the_caller_cannot_access` still checks that
through the real gate. The processor keeps an unpersisted query, but carrying
only schema and catalog — the two fields it reads — so there's no relationship
to trip on.
--
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]
