sadpandajoe commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4050299383
##########
superset/jinja_context.py:
##########
@@ -938,6 +941,26 @@ def is_safe_attribute(self, obj: Any, attr: str, value:
Any) -> bool:
return super().is_safe_attribute(obj, attr, value)
+PARAMETER_MISSING_ERR = _(
Review Comment:
This module-level eager `gettext` resolves `PARAMETER_MISSING_ERR` to a
plain string at import time. After this branch rebases, the target branch's
i18n fence imports this constant and requires a `LazyString`; it will also
freeze the suggestion in one locale while `undefined_parameters_message()`
translates per request. Could this use `lazy_gettext` and convert to `str` at
the response-building sites?
##########
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:
Making `schema` a named constructor parameter consumes a user dataset
template parameter that previously flowed through `**kwargs` into the Jinja
context. A virtual dataset with `template_params = {"schema": "sales"}` and SQL
`SELECT * FROM {{ schema }}.orders` now leaves `{{ schema }}` unresolved
instead of rendering `sales`. Could the internal schema override use a
non-colliding name or preserve the user value in the template context?
##########
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:
This still authorizes a re-render rather than the SQL passed to
`estimate_query_cost`: passing `sql=` creates an ephemeral query with no
`executed_sql`, so `raise_for_access` runs `process_jinja_sql` again before
extracting tables. A nondeterministic template can make the estimate render
contain `secret_tbl` while this authorization render turns that reference into
a SQL comment, leaving no table to check. Could this pin the already-rendered
text as literal and add an enforcement-level regression instead of mocking the
gate?
##########
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)
Review Comment:
The selected schema now reaches the processor, but the selected catalog is
still dropped. `PrestoTemplateProcessor.latest_partitions` constructs
`Table(table_name, schema)` with `catalog=None`, so `get_indexes`/`get_df` use
the connection's default catalog while `estimate_query_cost` receives
`self._catalog`. On a Trino connection whose default differs from the SQL Lab
selection, this reads partition metadata from one catalog and estimates
another. Could the processor carry the selected catalog too and cover a
non-default-catalog case?
--
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]
