mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4036329758
##########
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 as you asked, and thank you for pushing on it — I had been reasoning
about
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.
Estimate 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
rather than deferred.
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.
Estimate 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:
Moot: `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 behaviour deliberately.
--
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]
