bito-code-review[bot] commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4038751508
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -163,21 +163,69 @@ 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
+ )
+ try:
+ sql = template_processor.process_template(
+ self._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
+
+ # Reported the same way the execution path reports it
+ # (`SqlQueryRenderImpl._validate`): a parameter left unresolved makes
+ # the estimate describe a different query than the one Run would
+ # execute, and in some positions it does not even parse.
+ if undefined_parameters := sorted(
+ template_processor.get_undefined_parameters(sql)
+ ):
Review Comment:
<!-- Bito Reply -->
The suggestion provided by the reviewer is correct and appropriate. It
addresses a potential 500 error caused by `get_undefined_parameters` re-parsing
rendered SQL with Jinja, which can raise a `TemplateSyntaxError` if the
template parameters contain malformed Jinja. Moving this call inside the
existing `except TemplateError` block ensures that such errors are caught and
reported as a structured 400 error, maintaining parity with the execution path.
**superset/commands/sql_lab/estimate.py**
```
# Reported the same way the execution path reports it
# (`SqlQueryRenderImpl._validate`): a parameter left unresolved makes
# the estimate describe a different query than the one Run would
# execute, and in some positions it does not even parse.
if undefined_parameters := sorted(
template_processor.get_undefined_parameters(sql)
):
```
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -163,21 +163,69 @@ 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
+ )
+ try:
+ sql = template_processor.process_template(
+ self._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
+
+ # Reported the same way the execution path reports it
+ # (`SqlQueryRenderImpl._validate`): a parameter left unresolved makes
+ # the estimate describe a different query than the one Run would
+ # execute, and in some positions it does not even parse.
+ if undefined_parameters := sorted(
+ template_processor.get_undefined_parameters(sql)
+ ):
+ raise SupersetErrorException(
+ SupersetError(
+ message=ngettext(
+ "The parameter %(parameters)s in your query is
undefined.",
+ "The following parameters in your query are undefined:
"
+ "%(parameters)s.",
+ len(undefined_parameters),
+ parameters=utils.format_list(undefined_parameters),
),
- status=400,
- ) from ex
+ error_type=SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={
+ "undefined_parameters": undefined_parameters,
+ "template_parameters": self._template_params,
+ },
+ ),
+ status=400,
+ )
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to share the undefined parameter reporting logic
between the estimate and execution paths is valid. The current implementation
in the provided diff duplicates the `ngettext` strings, `utils.format_list`,
and `extra` keys, which risks inconsistency as the two paths evolve. Moving
this logic to a shared helper would ensure that both paths consistently report
undefined parameters, including any future additions like suggestion messages
or issue codes.
**superset/commands/sql_lab/estimate.py**
```
# Reported the same way the execution path reports it
# (`SqlQueryRenderImpl._validate`): a parameter left unresolved makes
# the estimate describe a different query than the one Run would
# execute, and in some positions it does not even parse.
if undefined_parameters := sorted(
template_processor.get_undefined_parameters(sql)
):
raise SupersetErrorException(
SupersetError(
message=ngettext(
"The parameter %(parameters)s in your query is
undefined.",
"The following parameters in your query are
undefined: "
"%(parameters)s.",
len(undefined_parameters),
parameters=utils.format_list(undefined_parameters),
),
error_type=SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR,
level=ErrorLevel.ERROR,
extra={
"undefined_parameters": undefined_parameters,
"template_parameters": self._template_params,
},
),
status=400,
)
```
--
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]
