mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r3963632667
##########
superset/jinja_context.py:
##########
@@ -797,6 +797,36 @@ def get_context(self) -> dict[str, Any]:
"""
return self._context.copy()
+ def has_template(self, sql: str) -> bool:
+ """Whether the SQL contains anything for ``process_template`` to expand
+
+ Lexed rather than parsed, so that a comment -- which leaves no trace in
+ a parsed template -- still counts, and using this processor's own
+ environment, so that any customized delimiters are honored. Lexing
+ evaluates nothing.
+
+ >>> has_template("SELECT '{{ current_username() }}'")
+ True
+ >>> has_template("{{ dataset(1) }}")
+ True
+ >>> has_template("SELECT '{{1,2},{3,4}}'::int[]")
+ False
+ """
+ try:
+ # The whole stream is consumed before deciding, rather than
stopping
+ # at the first non-`data` token: `'{{1,2},{3,4}}'` opens like a
+ # template and only turns out not to be one further along, where
the
+ # lexer gives up.
+ kinds = {kind for _, kind, _ in self.env.lex(sql)}
+ except TemplateSyntaxError:
Review Comment:
Confirmed — your SQL reproduces it.
I tried the direction you suggested first, and keeping the tokens seen
before the error flips the array literal on its own to `True`: `{{1,2}` emits
`variable_begin` before the lexer gives up, so "saw a template token" cannot
separate the two cases.
What does separate them is requiring the construct to have *closed*. The
literal is abandoned unterminated, so it never emits `variable_end`:
| SQL | want | seen-tokens | closed-only |
|---|---|---|---|
| `SELECT '{{1,2},{3,4}}'::int[]` | False | True ❌ | False ✅ |
| `SELECT '{{ current_username() }}', '{{1,2},{3,4}}'::int[]` | True | True
✅ | True ✅ |
Changed to that, with your example as a test case.
Also worth saying that the stakes here dropped along the way: detection now
only picks the wording of a parse error that is raised either way, so getting
it wrong costs the better message and nothing else.
##########
superset/jinja_context.py:
##########
@@ -797,6 +797,36 @@ def get_context(self) -> dict[str, Any]:
"""
return self._context.copy()
+ def has_template(self, sql: str) -> bool:
+ """Whether the SQL contains anything for ``process_template`` to expand
+
+ Lexed rather than parsed, so that a comment -- which leaves no trace in
+ a parsed template -- still counts, and using this processor's own
+ environment, so that any customized delimiters are honored. Lexing
+ evaluates nothing.
+
+ >>> has_template("SELECT '{{ current_username() }}'")
+ True
+ >>> has_template("{{ dataset(1) }}")
+ True
+ >>> has_template("SELECT '{{1,2},{3,4}}'::int[]")
+ False
+ """
+ try:
+ # The whole stream is consumed before deciding, rather than
stopping
+ # at the first non-`data` token: `'{{1,2},{3,4}}'` opens like a
+ # template and only turns out not to be one further along, where
the
+ # lexer gives up.
+ kinds = {kind for _, kind, _ in self.env.lex(sql)}
+ except TemplateSyntaxError:
+ # Jinja does not recognize this as one of its own constructs, so
+ # there is nothing here it would expand.
+ return False
+
+ # SQL that is nothing but a template produces no `data` token at all,
so
+ # this asks for any other kind rather than for `data` plus another.
+ return bool(kinds - {"data"})
Review Comment:
Good catch, and taken as stated: the contract is now explicit and the
example implements it.
`CustomPrestoTemplateProcessor` gets a `has_template` override that reports
its `$` syntax and still defers to Jinja via `super()`. The base docstring
names the obligation and points at that module, so the next processor with its
own syntax has somewhere to look. There is a test both ways — the custom
processor reports `$DATE(...)` as a template, and the base processor reports
the same SQL as having none, which is what makes the override necessary.
One thing worth flagging that falls out of the first thread: because the
render gate is gone, `process_template` is now always called, so that processor
does get to expand `$DATE(...)` on the estimate path. The old gate skipped it
whenever `template_params` was empty, and master still does.
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ 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,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # 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)
+ 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
# Apply the same SQL security controls used by the execution path
# (sql_lab.execute_sql_statements) so cost estimation cannot be used to
# probe disallowed functions/tables, bypass the DML guard, or confirm
# the existence of rows hidden by row-level security.
- sql = self._apply_sql_security(sql)
+ try:
+ sql = self._apply_sql_security(sql)
+ except SupersetParseError as ex:
Review Comment:
Confirmed, and it decided the direction. `DebugUndefined` leaves `{{ ds }}`
in a string literal, the SQL parses, nothing raises, and the block below never
ran — so a cost came back for a query whose `d` column is the literal text.
Reusing `SqlQueryRenderImpl`'s check as you suggested, rather than adding a
fourth condition to the block: it moved on to the processor as
`get_undefined_parameters`, both paths call it, and estimate raises the
existing `MISSING_TEMPLATE_PARAMS_ERROR` naming `ds`.
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ 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,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # 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)
+ 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
# Apply the same SQL security controls used by the execution path
# (sql_lab.execute_sql_statements) so cost estimation cannot be used to
# probe disallowed functions/tables, bypass the DML guard, or confirm
# the existence of rows hidden by row-level security.
- sql = self._apply_sql_security(sql)
+ try:
+ sql = self._apply_sql_security(sql)
+ except SupersetParseError as ex:
Review Comment:
Gone with the block. A missing parameter is caught before parsing, so there
is nothing left to rewrite and a `SupersetParseError` from
`_apply_sql_security` propagates as itself — RLS and length errors included.
Covered by a test that malformed SQL keeps the parser's own error.
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ 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,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # 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)
+ 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
# Apply the same SQL security controls used by the execution path
# (sql_lab.execute_sql_statements) so cost estimation cannot be used to
# probe disallowed functions/tables, bypass the DML guard, or confirm
# the existence of rows hidden by row-level security.
- sql = self._apply_sql_security(sql)
+ try:
+ sql = self._apply_sql_security(sql)
+ except SupersetParseError as ex:
+ # An unprovided parameter is left in place by `DebugUndefined`
+ # rather than raising, and in some positions the leftover then
+ # fails to parse. Reported as written, that reads as a typo in the
+ # SQL; name the actual cause instead.
+ if template_processor.has_template(sql):
+ raise SupersetParseError(
Review Comment:
Same removal: nothing reconstructs the error, so it reaches the client with
its coordinates intact.
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ 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,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # 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)
+ try:
+ sql = template_processor.process_template(
Review Comment:
Fixed, and thanks for pressing on it. I had been weighing whether to
restructure `validate()` when the execution path already had the answer:
`_validate_rendered_access` re-authorizes the literal rendered text before
executing, for the same reason.
Estimation now does the same after rendering, passing the rendered SQL with
no template params so there is nothing left to expand differently. `validate()`
is unchanged, so the pre-render check still runs first as it does on the
execution path. Tests cover both the two-check sequence and a template that
renders to a table the caller cannot read.
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ 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,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # 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)
Review Comment:
Confirmed for `presto.latest_partition` in a non-default schema, and fixed
here.
It turned out smaller than I expected. `BaseTemplateProcessor._schema` is
one assignment fed only by `query.schema` or `table.schema`, so taking a
`schema` argument replaces a hard-coded `None` rather than adding a branch — a
query still wins when there is one, and no existing caller passes it, so
nothing else moves. Estimation passes the requested schema.
The same gap runs through `validate()`'s `process_jinja_sql`, which builds a
processor with neither. That is outside this command, so I have left it alone
rather than widen the diff — happy to follow up if you want it.
##########
superset/jinja_context.py:
##########
@@ -983,6 +983,42 @@ def get_template_context(self, **kwargs: Any) -> dict[str,
Any]:
kwargs.update(self._context)
return validate_template_context(self.engine, kwargs)
+ def has_template(self, sql: str) -> bool:
+ """Whether the SQL contains anything for ``process_template`` to expand
+
+ Lexed rather than parsed, so that a comment -- which leaves no trace in
+ a parsed template -- still counts, and using this processor's own
+ environment, so that any customized delimiters are honored. Lexing
+ evaluates nothing.
+
+ Answers for Jinja, so a subclass whose ``process_template`` expands a
+ syntax of its own has to answer for that syntax too -- see
Review Comment:
No longer applies: `has_template` is removed, so
`CUSTOM_TEMPLATE_PROCESSORS` keeps the contract it has on master and the docs
example stays correct. The override I had added to
`CustomPrestoTemplateProcessor` is reverted.
##########
tests/unit_tests/jinja_context_test.py:
##########
@@ -3416,3 +3416,82 @@ def
test_get_rendered_sql_filter_values_index_error_on_empty_list() -> None:
match=r"Virtual dataset template error: list object has no element 0",
):
table.get_rendered_sql(processor)
+
+
[email protected](
+ "sql,expected",
+ [
+ pytest.param("SELECT 1", False, id="plain"),
+ pytest.param("SELECT '{{ current_username() }}'", True,
id="expression"),
+ pytest.param("{% set a = 1 %}SELECT {{ a }}", True, id="statement"),
+ # A comment leaves no trace in a parsed template, but still has to be
+ # expanded away before the SQL is SQL.
+ pytest.param("SELECT 1 {# a comment #}", True, id="comment"),
+ # A whole query that is one macro lexes without a `data` token at all.
+ pytest.param("{{ dataset(1) }}", True, id="template_only"),
+ # Merely containing braces is not templating: the array literal opens
+ # like a template and is abandoned unterminated, and the JSON literal
is
+ # never even mistaken for one.
+ pytest.param("SELECT '{{1,2},{3,4}}'::int[]", False,
id="postgres_array"),
+ pytest.param("""SELECT '{"a": 1}'::json""", False, id="json_literal"),
+ # A real template alongside an array literal is still a template: the
+ # first construct closes before the lexer gives up on the second.
+ pytest.param(
+ "SELECT '{{ current_username() }}', '{{1,2},{3,4}}'::int[]",
+ True,
+ id="template_beside_array",
+ ),
+ ],
+)
+@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=True)
+def test_has_template(sql: str, expected: bool) -> None:
+ """
+ Test the ``has_template`` method.
+ """
+ database = Database(id=1, database_name="my_database",
sqlalchemy_uri="sqlite://")
+ processor = get_template_processor(database=database)
+
+ assert processor.has_template(sql) is expected
+
+
+@with_feature_flags(ENABLE_TEMPLATE_PROCESSING=False)
+def test_has_template_when_processing_is_disabled() -> None:
+ """
+ Test that ``has_template`` reports no template when nothing is expanded.
+
+ With ``ENABLE_TEMPLATE_PROCESSING`` off, ``get_template_processor`` returns
+ a ``NoOpTemplateProcessor``: the braces are never expanded, so they are not
+ a template, they are just part of the SQL.
+ """
+ database = Database(id=1, database_name="my_database",
sqlalchemy_uri="sqlite://")
+ processor = get_template_processor(database=database)
+
+ assert processor.has_template("SELECT '{{ current_username() }}'") is False
+
+
+def test_has_template_for_a_processor_with_its_own_syntax() -> None:
+ """
+ Test that a processor expanding its own syntax can report it.
+
+ ``has_template`` answers for Jinja, so a processor whose
``process_template``
+ expands something else has to override it, or it reports no template for
SQL
+ it would in fact expand. ``CustomPrestoTemplateProcessor`` is the in-repo
+ example of such a processor, and of the override.
+ """
+ from tests.integration_tests.superset_test_custom_template_processors
import (
+ CustomPrestoTemplateProcessor,
+ )
+
+ database = Database(id=1, database_name="my_database",
sqlalchemy_uri="sqlite://")
+ processor = CustomPrestoTemplateProcessor(database=database)
+
+ assert processor.has_template("SELECT '$DATE()'") is True
+ # Jinja is still recognized, and plain SQL is still plain.
+ assert processor.has_template("SELECT '{{ current_username() }}'") is True
+ assert processor.has_template("SELECT 1") is False
+
+ # Without the override, the same SQL reads as having no template at all.
+ assert (
+ get_template_processor(database=database).has_template("SELECT
'$DATE()'")
Review Comment:
Correct — with `NoOpTemplateProcessor` in play the assertion proved nothing.
It is removed along with the detector it tested. The replacement tests for
`get_undefined_parameters` carry the flag decorator, and the disabled-flag case
is its own test asserting the No-op behavior deliberately.
##########
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]
