sadpandajoe commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4000855267
##########
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:
This adds an unconditional second render without the execution path's
rendered-SQL access check, so a nondeterministic template can be authorized as
one table and then send `EXPLAIN` for another (for example, `{{ ['allowed_ds',
'secret_tbl'] | random }}` for a dataset-scoped user). Could we re-run
`raise_for_access` on the literal rendered SQL before estimating it, as the
execution command 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:
This catches every `SupersetParseError` from `_apply_sql_security`, so an
over-length templated query or a malformed RLS predicate is rewritten as a
missing-template-parameter error and loses the actual diagnostic. Could we
scope the reclassification to parsing the rendered query and preserve
downstream security errors?
##########
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:
This constructs the processor without a query, leaving its selected schema
unset; a Presto query in a non-default SQL Lab schema that uses
`presto.latest_partition("events")` can look up the default schema and estimate
different SQL than Run, which passes `query=query_model`. Could we supply
equivalent query/schema context and cover a non-default-schema macro?
##########
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:
The new contract requires custom processors with non-Jinja syntax to
implement `has_template`, but the operator-facing `CUSTOM_TEMPLATE_PROCESSORS`
example in `docs/admin_docs/configuration/sql-templating.mdx` still shows only
`process_template`. Could we update that example so copied `$`-macro processors
don't silently fall back to generic parse errors?
##########
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:
This assertion runs under the default disabled flag, so
`get_template_processor` returns `NoOpTemplateProcessor` and stays false
regardless of the base Jinja detector; the test therefore doesn't prove the
custom override is necessary and changes meaning with leaked flag state. Could
this sibling test also enable `ENABLE_TEMPLATE_PROCESSING` before asserting
that the base processor does not recognize `$DATE()`?
##########
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:
Reconstructing `SupersetParseError` without forwarding `line` and `column`
replaces the original parser coordinates with nulls, so consumers of the
`INVALID_SQL_ERROR` payload can no longer highlight the offending token. Could
we preserve the original location fields while changing only the message?
--
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]
