bito-code-review[bot] commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4038218403
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated undefined-param reporting</b></div>
<div id="fix">
This block duplicates
`SqlQueryRenderImpl._raise_undefined_parameter_exception`
(superset/sqllab/query_render.py:90-113): same `ngettext` strings,
`utils.format_list`, and `extra` keys. The copy already drifted — the execution
path adds `suggestion_help_msg` and `issue_codes` (code 1006) that the estimate
response omits. A shared helper would keep the two reports and their
translations consistent.
</div>
</div>
<small><i>Code Review Run #c5a8bf</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unhandled TemplateSyntaxError 500</b></div>
<div id="fix">
`get_undefined_parameters()` re-parses the rendered SQL with Jinja
(`env.parse` in `BaseTemplateProcessor.get_undefined_parameters`,
superset/jinja_context.py:1026), so a template parameter whose value contains
malformed Jinja (e.g. `{% for %}`) raises `TemplateSyntaxError` here, outside
the `except TemplateError` above. The mirrored execution path wraps `_validate`
in exactly that catch (`SqlQueryRenderImpl.render`,
superset/sqllab/query_render.py:63-66) and returns a structured 400; this path
leaks a raw jinja2 error and 500s.
</div>
</div>
<small><i>Code Review Run #c5a8bf</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
