diff --git a/superset/commands/database/validate_sql.py b/superset/commands/database/validate_sql.py index c252cf6fdb8..d7ab40f882c 100644 --- a/superset/commands/database/validate_sql.py +++ b/superset/commands/database/validate_sql.py @@ -32,6 +32,11 @@ from superset.commands.database.exceptions import ( ) from superset.daos.database import DatabaseDAO from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import ( + SupersetSyntaxErrorException, + SupersetTemplateException, +) +from superset.jinja_context import get_template_processor from superset.models.core import Database from superset.sql_validators import get_validator_by_name from superset.sql_validators.base import BaseSQLValidator @@ -62,12 +67,51 @@ class ValidateSQLCommand(BaseCommand): sql = self._properties["sql"] catalog = self._properties.get("catalog") schema = self._properties.get("schema") + template_params = self._properties.get("template_params") or {} + try: + # Render Jinja templates to handle template syntax before + # validation. Note: The ENABLE_TEMPLATE_PROCESSING feature flag is + # checked within get_template_processor(), which returns + # NoOpTemplateProcessor when disabled. Template processing errors + # (e.g., undefined filters, syntax errors) are caught by this + # exception handler and surfaced to the client as + # ValidatorSQLError or ValidatorSQL400Error with appropriate error + # messages. + template_processor = get_template_processor(self._model) + # process_template() renders Jinja2 templates and always returns a + # new string (does not mutate the input SQL). May raise + # SupersetSyntaxErrorException for template syntax errors or + # SupersetTemplateException for internal errors. + sql = template_processor.process_template(sql, **template_params) + timeout = app.config["SQLLAB_VALIDATION_TIMEOUT"] timeout_msg = f"The query exceeded the {timeout} seconds timeout." with utils.timeout(seconds=timeout, error_message=timeout_msg): errors = self._validator.validate(sql, catalog, schema, self._model) return [err.to_dict() for err in errors] + except SupersetSyntaxErrorException as ex: + # Template syntax errors (e.g., invalid Jinja2 syntax, undefined variables) + # These contain detailed error information including line numbers + logger.warning( + "Template syntax error during SQL validation", + extra={"errors": [err.message for err in ex.errors]}, + ) + raise ValidatorSQL400Error(ex.errors[0]) from ex + except SupersetTemplateException as ex: + # Internal template processing errors (e.g., recursion, unexpected failures) + logger.error( + "Template processing error during SQL validation", exc_info=True + ) + superset_error = SupersetError( + message=__( + "Template processing failed: %(ex)s", + ex=str(ex), + ), + error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, + level=ErrorLevel.ERROR, + ) + raise ValidatorSQL400Error(superset_error) from ex except Exception as ex: logger.exception(ex) superset_error = SupersetError( diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py index cfad9615893..94b24896a59 100644 --- a/tests/integration_tests/databases/api_tests.py +++ b/tests/integration_tests/databases/api_tests.py @@ -4104,6 +4104,115 @@ class TestDatabaseApi(SupersetTestCase): assert rv.status_code == 422 assert "Kaboom!" in response["errors"][0]["message"] + @mock.patch.dict( + "superset.config.SQL_VALIDATORS_BY_ENGINE", + SQL_VALIDATORS_BY_ENGINE, + clear=True, + ) + def test_validate_sql_with_jinja_templates(self): + """ + Database API: validate SQL with Jinja templates + """ + request_payload = { + "sql": ( + "SELECT *\nFROM birth_names\nWHERE 1=1\n" + "{% if city_filter is defined %}\n" + " AND city = '{{ city_filter }}'\n{% endif %}\n" + "LIMIT {{ limit | default(100) }}" + ), + "schema": None, + "template_params": {}, + } + + example_db = get_example_database() + if example_db.backend not in ("presto", "postgresql"): + pytest.skip("Only presto and PG are implemented") + + self.login(ADMIN_USERNAME) + uri = f"api/v1/database/{example_db.id}/validate_sql/" + rv = self.client.post(uri, json=request_payload) + response = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + # Template was successfully rendered and validated + # so a valid query returns an empty result list + result = response["result"] + assert isinstance(result, list) + assert len(result) == 0 + + @mock.patch.dict( + "superset.config.SQL_VALIDATORS_BY_ENGINE", + SQL_VALIDATORS_BY_ENGINE, + clear=True, + ) + def test_validate_sql_with_jinja_templates_and_params(self): + """ + Database API: validate SQL with Jinja templates and parameters + """ + request_payload = { + "sql": ( + "SELECT *\nFROM birth_names\nWHERE 1=1\n" + "{% if city_filter is defined %}\n" + " AND city = '{{ city_filter }}'\n" + "{% endif %}\nLIMIT {{ limit }}" + ), + "schema": None, + "template_params": {"city_filter": "New York", "limit": 50}, + } + + example_db = get_example_database() + if example_db.backend not in ("presto", "postgresql"): + pytest.skip("Only presto and PG are implemented") + + self.login(ADMIN_USERNAME) + uri = f"api/v1/database/{example_db.id}/validate_sql/" + rv = self.client.post(uri, json=request_payload) + response = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + # Template was successfully rendered with parameters and validated + # so a valid query returns an empty result list + result = response["result"] + assert isinstance(result, list) + assert len(result) == 0 + + @mock.patch.dict( + "superset.config.SQL_VALIDATORS_BY_ENGINE", + SQL_VALIDATORS_BY_ENGINE, + clear=True, + ) + def test_validate_sql_with_jinja_invalid_sql_after_render(self): + """ + Database API: validate SQL with Jinja templates that renders to invalid SQL + + This test ensures that SQL validation errors are not hidden by template + processing. The template should render successfully, but the resulting SQL + should fail syntax validation. + """ + request_payload = { + "sql": ( + "SELECT *\nFROM birth_names\n" + "{% if add_invalid_clause %}\n" + "WHERE\n" + "{% endif %}" + ), + "schema": None, + "template_params": {"add_invalid_clause": True}, + } + + example_db = get_example_database() + if example_db.backend not in ("presto", "postgresql"): + pytest.skip("Only presto and PG are implemented") + + self.login(ADMIN_USERNAME) + uri = f"api/v1/database/{example_db.id}/validate_sql/" + rv = self.client.post(uri, json=request_payload) + response = json.loads(rv.data.decode("utf-8")) + assert rv.status_code == 200 + # The template should render successfully, but SQL validation + # should catch the syntax error (WHERE clause with no condition) + result = response["result"] + assert isinstance(result, list) + assert len(result) > 0 + def test_get_databases_with_extra_filters(self): """ API: Test get database with extra query filter. diff --git a/tests/unit_tests/commands/databases/validate_sql_test.py b/tests/unit_tests/commands/databases/validate_sql_test.py new file mode 100644 index 00000000000..46df10cecfd --- /dev/null +++ b/tests/unit_tests/commands/databases/validate_sql_test.py @@ -0,0 +1,279 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from typing import Any +from unittest.mock import MagicMock + +import pytest +from pytest_mock import MockerFixture + +from superset.commands.database.exceptions import ( + ValidatorSQL400Error, + ValidatorSQLError, +) +from superset.commands.database.validate_sql import ValidateSQLCommand +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import ( + SupersetSyntaxErrorException, + SupersetTemplateException, +) + + +@pytest.fixture +def mock_database(mocker: MockerFixture) -> MagicMock: + """Create a mock database with PostgreSQL engine.""" + database = mocker.MagicMock() + database.id = 1 + database.db_engine_spec.engine = "postgresql" + + DatabaseDAO = mocker.patch( # noqa: N806 + "superset.commands.database.validate_sql.DatabaseDAO" + ) + DatabaseDAO.find_by_id.return_value = database + return database + + +@pytest.fixture +def mock_validator(mocker: MockerFixture) -> MagicMock: + """Create a mock SQL validator.""" + validator = mocker.MagicMock() + validator.name = "PostgreSQLValidator" + validator.validate.return_value = [] + + get_validator_by_name = mocker.patch( + "superset.commands.database.validate_sql.get_validator_by_name" + ) + get_validator_by_name.return_value = validator + return validator + + +@pytest.fixture +def mock_config(mocker: MockerFixture) -> dict[str, Any]: + """Mock the application config.""" + config = { + "SQL_VALIDATORS_BY_ENGINE": {"postgresql": "PostgreSQLValidator"}, + "SQLLAB_VALIDATION_TIMEOUT": 30, + } + mocker.patch("superset.commands.database.validate_sql.app.config", config) + return config + + +@pytest.fixture +def mock_template_processor( + mocker: MockerFixture, mock_database: MagicMock +) -> MagicMock: + """Create a mock template processor.""" + template_processor = mocker.MagicMock() + get_template_processor = mocker.patch( + "superset.commands.database.validate_sql.get_template_processor" + ) + get_template_processor.return_value = template_processor + return template_processor + + +def test_validate_sql_with_jinja_templates( + mock_database: MagicMock, + mock_validator: MagicMock, + mock_template_processor: MagicMock, + mock_config: dict[str, Any], +) -> None: + """Test that Jinja templates are rendered before SQL validation.""" + sql_with_jinja = """SELECT * + FROM birth_names + WHERE 1=1 + {% if city_filter is defined %} + AND city = '{{ city_filter }}' + {% endif %} + LIMIT {{ limit | default(100) }}""" + + mock_template_processor.process_template.return_value = ( + "SELECT *\nFROM birth_names\nWHERE 1=1\nLIMIT 100" + ) + + data = {"sql": sql_with_jinja, "schema": "public", "template_params": {}} + command = ValidateSQLCommand(model_id=1, data=data) + result = command.run() + + mock_template_processor.process_template.assert_called_once_with(sql_with_jinja) + mock_validator.validate.assert_called_once() + assert result == [] + + +def test_validate_sql_with_jinja_templates_and_params( + mock_database: MagicMock, + mock_validator: MagicMock, + mock_template_processor: MagicMock, + mock_config: dict[str, Any], +) -> None: + """Test that Jinja templates are rendered with parameters before SQL validation.""" + sql_with_jinja = """SELECT * + FROM birth_names + WHERE 1=1 + {% if city_filter is defined %} + AND city = '{{ city_filter }}' + {% endif %} + LIMIT {{ limit }}""" + + template_params = {"city_filter": "New York", "limit": 50} + mock_template_processor.process_template.return_value = ( + "SELECT *\nFROM birth_names\nWHERE 1=1\n AND city = 'New York'\nLIMIT 50" + ) + + data = { + "sql": sql_with_jinja, + "schema": "public", + "template_params": template_params, + } + command = ValidateSQLCommand(model_id=1, data=data) + result = command.run() + + mock_template_processor.process_template.assert_called_once_with( + sql_with_jinja, **template_params + ) + mock_validator.validate.assert_called_once() + assert result == [] + + +def test_validate_sql_without_jinja_templates( + mock_database: MagicMock, + mock_validator: MagicMock, + mock_template_processor: MagicMock, + mock_config: dict[str, Any], +) -> None: + """Test that regular SQL without Jinja templates still works.""" + simple_sql = "SELECT * FROM birth_names LIMIT 100" + mock_template_processor.process_template.return_value = simple_sql + + data = {"sql": simple_sql, "schema": "public", "template_params": {}} + command = ValidateSQLCommand(model_id=1, data=data) + result = command.run() + + mock_template_processor.process_template.assert_called_once() + mock_validator.validate.assert_called_once() + assert result == [] + + +def test_validate_sql_template_syntax_error( + mock_database: MagicMock, + mock_validator: MagicMock, + mock_template_processor: MagicMock, + mock_config: dict[str, Any], +) -> None: + """ + Test that template syntax errors are properly surfaced to the client. + + When template processing raises a SupersetSyntaxErrorException (e.g., + invalid Jinja2 syntax, undefined variables), it should be caught and + converted to a ValidatorSQL400Error with detailed error information + including line numbers. + """ + syntax_error = SupersetError( + message="Jinja2 template error (UndefinedError): 'city_filter' is undefined", + error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, + level=ErrorLevel.ERROR, + extra={"template": "SELECT * FROM...", "line": 3}, + ) + mock_template_processor.process_template.side_effect = SupersetSyntaxErrorException( + [syntax_error] + ) + + sql_with_undefined_var = """SELECT * + FROM birth_names + WHERE city = '{{ city_filter }}' + LIMIT 100""" + + data = { + "sql": sql_with_undefined_var, + "schema": "public", + "template_params": {}, + } + command = ValidateSQLCommand(model_id=1, data=data) + + with pytest.raises(ValidatorSQL400Error) as exc_info: + command.run() + + error = exc_info.value + assert error.error.message is not None + assert "'city_filter' is undefined" in error.error.message + mock_validator.validate.assert_not_called() + + +def test_validate_sql_template_processing_error( + mock_database: MagicMock, + mock_validator: MagicMock, + mock_template_processor: MagicMock, + mock_config: dict[str, Any], +) -> None: + """ + Test that internal template processing errors are properly surfaced to the client. + + When template processing raises a SupersetTemplateException (e.g., recursion, + unexpected failures), it should be caught and converted to a ValidatorSQL400Error + with an appropriate error message. + """ + mock_template_processor.process_template.side_effect = SupersetTemplateException( + "Infinite recursion detected in template" + ) + + data = { + "sql": "SELECT * FROM birth_names LIMIT 100", + "schema": "public", + "template_params": {}, + } + command = ValidateSQLCommand(model_id=1, data=data) + + with pytest.raises(ValidatorSQL400Error) as exc_info: + command.run() + + error = exc_info.value + assert error.error.message is not None + assert "Template processing failed" in error.error.message + assert "Infinite recursion" in error.error.message + mock_validator.validate.assert_not_called() + + +def test_validate_sql_generic_exception( + mock_database: MagicMock, + mock_validator: MagicMock, + mock_template_processor: MagicMock, + mock_config: dict[str, Any], +) -> None: + """ + Test that unexpected exceptions are still caught and handled gracefully. + + When an unexpected exception occurs (not template-related), it should be caught + and converted to a ValidatorSQLError with the validator name in the message. + """ + mock_template_processor.process_template.side_effect = RuntimeError( + "Unexpected error occurred" + ) + + data = { + "sql": "SELECT * FROM birth_names", + "schema": "public", + "template_params": {}, + } + command = ValidateSQLCommand(model_id=1, data=data) + + with pytest.raises(ValidatorSQLError) as exc_info: + command.run() + + error = exc_info.value + assert error.error.message is not None + assert "PostgreSQLValidator" in error.error.message + assert "Unexpected error occurred" in error.error.message + mock_validator.validate.assert_not_called()