mapledan commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4036330412
##########
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.
--
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]
