mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4067893359
##########
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:
Confirmed, and worth recording where it comes from: this catch and its
`GENERIC_COMMAND_ERROR` predate the PR — it's master's, inside the `if
self._template_params:` gate that this branch removes. So the divergence you're
describing is pre-existing, but it was nearly unreachable before (no declared
parameter meant no render meant no `TemplateError`) and this branch makes it a
normal path. That makes it ours to fix even though we didn't write it.
Your diagnosis of the root cause matches what the other threads keep landing
on. Four things now have to agree between the two paths — render, detect
undefined parameters, report a template failure, re-authorize — and three of
them agree only because I copied Run's shape by hand, one thread at a time.
`get_undefined_parameters` is the only one that's genuinely shared code.
I'd rather not fold it into this PR, though, and I think you're right that
it isn't blocking. Making the two report identically means either estimate
raising `SqlQueryRenderException` (which wants a `SqlJsonExecutionContext` it
doesn't have) or lifting that error construction somewhere both can reach — the
same extraction question as the undefined-parameter message, but on an
exception type wired into SQL Lab's response handling. That's a change with its
own review surface, and this PR has already grown twice from "and while we're
here".
So: separate issue once this lands, and happy to pick it up. If you'd rather
see it here, say so and I'll do it rather than leave the inconsistency
documented but unfixed.
--
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]
