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]
