mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4051827836
##########
superset/jinja_context.py:
##########
@@ -951,14 +974,19 @@ def __init__(
database: "Database",
query: "Query" | None = None,
table: "SqlaTable" | None = None,
+ schema: str | None = None,
Review Comment:
Good catch, and this one reaches existing data — `models/helpers` does
`template_kwargs.update(self.template_params_dict)`, so a dataset's own params
arrive as constructor keywords. `{"schema": "sales"}` with `SELECT * FROM {{
schema }}.orders` stopped rendering and left the reference in place. Confirmed
against master, which renders it.
Rather than rename the internal one, I dropped it. Estimation builds an
unpersisted `Query` and passes that, which is where the processor has always
read the location from — so no keyword is claimed and the constructor goes back
to its master shape. That also carries the catalog you raise below, and lets
the rendered SQL ride along as `executed_sql`.
What I'm less comfortable leaving alone is the general shape: every keyword
this constructor names is a name a user can no longer use in `template_params`,
and nothing anywhere says so. `extra_cache_keys`, `removed_filters` and
`applied_filters` sit in the same position today. Is that worth a guard, or at
least something on the signature? It feels wider than this PR, so I'd rather
raise it separately than grow this one — but say the word if you'd prefer it
here.
##########
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:
Pinned as `executed_sql` and dropped the `sql=` form, so this matches
`_validate_rendered_access`.
One thing I'd rather flag than paper over: I can't construct a case where
the two forms actually differ. With no `template_params` passed, `sql=` builds
an ephemeral query whose `sql` is already the rendered text, and pinning puts
the same string in `executed_sql` — both land in `process_jinja_sql(rendered,
database, None)`. I wrote the `| random` case as a test and it passes against
either version, so as it stands I'd be adding a test that doesn't exercise the
change.
The macro-neutralisation you describe (a reference becoming a comment before
the tables are extracted) happens inside `process_jinja_sql`, which both forms
reach, so I don't think that separates them either.
So I've kept the change as parity of mechanism and said exactly that in the
test, rather than have it claim a hole is closed. Is there a case you had in
mind where the authorization render and the estimate render land on different
tables without params in play? If so I'd rather cover it properly than leave
this as a shape change.
--
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]
