mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4038749504
##########
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:
Reproduced and fixed in cd17f3ec74.
`get_undefined_parameters` parses the rendered SQL with Jinja, so a parameter
whose value carries malformed Jinja raises from there, and the call sat after
the `except TemplateError` block — `{"x": "'{% for %}'"}` escaped `run()` as
a
raw `TemplateSyntaxError`. The execution path wraps the same check inside
`render`'s catch, which is exactly the parity this PR is arguing for, so the
call moved inside the catch.
The regression test deliberately uses a real template processor: with
`get_template_processor` mocked — as the other command tests do — the code
that
raises never runs, which is why nothing here caught it.
##########
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:
Half right, and the half that is right is fixed in cd17f3ec74.
The duplication was real: `undefined_parameters_message` and
`PARAMETER_MISSING_ERR` now live in `jinja_context` and both paths use them,
so
the reason string, the suggestion and their translations stay in one place.
The
estimate response carries the suggestion it was missing.
Issue code 1006 was not missing, though. `SupersetError.__post_init__`
injects
it from `ERROR_TYPES_TO_ISSUE_CODES_MAPPING` for any
`MISSING_TEMPLATE_PARAMS_ERROR`, so constructing the error is enough —
`test_run_reports_an_unprovided_parameter_as_missing` asserts
`extra["issue_codes"][0]["code"] == 1006` and passed before this change too.
--
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]
