rusackas commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4067376906
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -163,21 +167,71 @@ 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.
+ template_processor = get_template_processor(
+ self._database, schema=self._schema or None
+ )
+ # 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,
Review Comment:
One more parity gap while I was in here: a parameter whose value carries
malformed Jinja (`{"x": "'{% for %}'"}`) makes `get_undefined_parameters` raise
`TemplateSyntaxError`, and this catch reports it as `GENERIC_COMMAND_ERROR`
with the raw jinja2 text. `SqlQueryRenderImpl.render`'s identical catch reports
`INVALID_TEMPLATE_PARAMS_ERROR` with a translated message instead. Same
failure, two different contracts depending on which endpoint hit it, probably
the same root cause as the other threads here: nothing forces estimate and Run
to share one render/detect/reauthorize path.
Not blocking on its own, just worth folding into the next pass.
--
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]
