diff --git a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx index b34209786b777..abf4886ce169d 100644 --- a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx +++ b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx @@ -73,3 +73,14 @@ The table was deleted or renamed in the database. Your query failed because it is referencing a table that no longer exists in the underlying database. You should modify your query to reference the correct table. + +## Issue 1006 + +``` +One or more parameters specified in the query are missing. +``` + +Your query was not submitted to the database because it's missing one or more +parameters. You should define all the parameters referenced in the query in a +valid JSON document. Check that the parameters are spelled correctly and that +the document has a valid syntax. diff --git a/superset-frontend/src/SqlLab/components/ResultSet.tsx b/superset-frontend/src/SqlLab/components/ResultSet.tsx index a5a1d5f842054..d421a43a1cabb 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet.tsx @@ -28,6 +28,7 @@ import { styled, t } from '@superset-ui/core'; import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace'; import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal'; import { getByUser, put as updateDatset } from 'src/api/dataset'; +import { ErrorTypeEnum } from 'src/components/ErrorMessage/types'; import Loading from '../../components/Loading'; import ExploreCtasResultsButton from './ExploreCtasResultsButton'; import ExploreResultsButton from './ExploreResultsButton'; @@ -489,10 +490,17 @@ export default class ResultSet extends React.PureComponent< return Query was stopped; } if (query.state === 'failed') { + // TODO (betodealmeida): handle this logic through the error component + // registry + const title = + query?.errors?.[0].error_type === + ErrorTypeEnum.MISSING_TEMPLATE_PARAMS_ERROR + ? t('Parameter Error') + : t('Database Error'); return (
{query.errorMessage}} copyText={query.errorMessage || undefined} diff --git a/superset-frontend/src/SqlLab/components/SqlEditor.jsx b/superset-frontend/src/SqlLab/components/SqlEditor.jsx index 044fee86ae579..4b4a1bc1acb4d 100644 --- a/superset-frontend/src/SqlLab/components/SqlEditor.jsx +++ b/superset-frontend/src/SqlLab/components/SqlEditor.jsx @@ -566,13 +566,15 @@ class SqlEditor extends React.PureComponent { {' '} {t('Autocomplete')} {' '} - { - this.props.actions.queryEditorSetTemplateParams(qe, params); - }} - code={qe.templateParams} - /> + {isFeatureEnabled(FeatureFlag.ENABLE_TEMPLATE_PROCESSING) && ( + { + this.props.actions.queryEditorSetTemplateParams(qe, params); + }} + code={qe.templateParams} + /> + )} {limitWarning} {this.props.latestQuery && ( = T[keyof T]; diff --git a/superset-frontend/src/featureFlags.ts b/superset-frontend/src/featureFlags.ts index 93b909ddcb3b1..75367d23a3f83 100644 --- a/superset-frontend/src/featureFlags.ts +++ b/superset-frontend/src/featureFlags.ts @@ -35,6 +35,7 @@ export enum FeatureFlag { ESCAPE_MARKDOWN_HTML = 'ESCAPE_MARKDOWN_HTML', VERSIONED_EXPORT = 'VERSIONED_EXPORT', GLOBAL_ASYNC_QUERIES = 'GLOBAL_ASYNC_QUERIES', + ENABLE_TEMPLATE_PROCESSING = 'ENABLE_TEMPLATE_PROCESSING', } export type FeatureFlagMap = { diff --git a/superset/errors.py b/superset/errors.py index 14d389fa3c607..416132b05b29d 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -27,6 +27,7 @@ class SupersetErrorType(str, Enum): Types of errors that can exist within Superset. Keep in sync with superset-frontend/src/components/ErrorMessage/types.ts + and docs/src/pages/docs/Miscellaneous/issue_codes.mdx """ # Frontend errors @@ -52,6 +53,9 @@ class SupersetErrorType(str, Enum): # Other errors BACKEND_TIMEOUT_ERROR = "BACKEND_TIMEOUT_ERROR" + # Sql Lab errors + MISSING_TEMPLATE_PARAMS_ERROR = "MISSING_TEMPLATE_PARAMS_ERROR" + ERROR_TYPES_TO_ISSUE_CODES_MAPPING = { SupersetErrorType.BACKEND_TIMEOUT_ERROR: [ @@ -100,6 +104,15 @@ class SupersetErrorType(str, Enum): ), }, ], + SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR: [ + { + "code": 1006, + "message": _( + "Issue 1006 - One or more parameters specified in the query are " + "missing." + ), + }, + ], } diff --git a/superset/jinja_context.py b/superset/jinja_context.py index 400b9d60df7e0..b86b466e91c40 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -22,6 +22,7 @@ from flask import current_app, g, request from flask_babel import gettext as _ +from jinja2 import DebugUndefined from jinja2.sandbox import SandboxedEnvironment from superset.exceptions import SupersetTemplateException @@ -60,7 +61,7 @@ def context_addons() -> Dict[str, Any]: def filter_values(column: str, default: Optional[str] = None) -> List[str]: - """ Gets a values for a particular filter as a list + """Gets a values for a particular filter as a list This is useful if: - you want to use a filter box to filter a query where the name of filter box @@ -296,7 +297,7 @@ def __init__( self._schema = table.schema self._extra_cache_keys = extra_cache_keys self._context: Dict[str, Any] = {} - self._env = SandboxedEnvironment() + self._env = SandboxedEnvironment(undefined=DebugUndefined) self.set_context(**kwargs) def set_context(self, **kwargs: Any) -> None: diff --git a/superset/utils/core.py b/superset/utils/core.py index 52b0544cf5fa9..91de0c0d2238a 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -1669,3 +1669,8 @@ def get_time_filter_status( # pylint: disable=too-many-branches ) return applied, rejected + + +def format_list(items: Sequence[str], sep: str = ", ", quote: str = '"') -> str: + quote_escaped = "\\" + quote + return sep.join(f"{quote}{x.replace(quote, quote_escaped)}{quote}" for x in items) diff --git a/superset/views/core.py b/superset/views/core.py index 63591b73efb94..6c687d074ddec 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -14,7 +14,8 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. -# pylint: disable=comparison-with-callable +# pylint: disable=comparison-with-callable, line-too-long, too-many-branches +import dataclasses import logging import re from contextlib import closing @@ -34,8 +35,9 @@ permission_name, ) from flask_appbuilder.security.sqla import models as ab_models -from flask_babel import gettext as __, lazy_gettext as _ +from flask_babel import gettext as __, lazy_gettext as _, ngettext from jinja2.exceptions import TemplateError +from jinja2.meta import find_undeclared_variables from sqlalchemy import and_, or_ from sqlalchemy.engine.url import make_url from sqlalchemy.exc import ArgumentError, DBAPIError, NoSuchModuleError, SQLAlchemyError @@ -69,6 +71,7 @@ from superset.dashboards.dao import DashboardDAO from superset.databases.dao import DatabaseDAO from superset.databases.filters import DatabaseFilter +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( CacheLoadError, CertificateException, @@ -157,6 +160,11 @@ DATASOURCE_MISSING_ERR = __("The data source seems to have been deleted") USER_MISSING_ERR = __("The user seems to have been deleted") +PARAMETER_MISSING_ERR = ( + "Please check your template parameters for syntax errors and make sure " + "they match across your SQL query and Set Parameters. Then, try running " + "your query again." +) class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods @@ -2506,6 +2514,28 @@ def sql_json_exec( # pylint: disable=too-many-statements,too-many-locals f"Query {query_id}: Template syntax error: {error_msg}" ) + # pylint: disable=protected-access + if is_feature_enabled("ENABLE_TEMPLATE_PROCESSING"): + ast = template_processor._env.parse(rendered_query) + undefined = find_undeclared_variables(ast) # type: ignore + if undefined: + error = SupersetError( + message=ngettext( + "There's an error with the parameter %(parameters)s.", + "There's an error with the parameters %(parameters)s.", + len(undefined), + parameters=utils.format_list(undefined), + ) + + " " + + PARAMETER_MISSING_ERR, + level=ErrorLevel.ERROR, + error_type=SupersetErrorType.MISSING_TEMPLATE_PARAMS_ERROR, + extra={"missing_parameters": list(undefined)}, + ) + return json_error_response( + payload={"errors": [dataclasses.asdict(error)]}, + ) + # Limit is not applied to the CTA queries if SQLLAB_CTAS_NO_LIMIT flag is set # to True. if not (config.get("SQLLAB_CTAS_NO_LIMIT") and select_as_cta): diff --git a/tests/base_tests.py b/tests/base_tests.py index 51bae4883bf31..6f3819996f52c 100644 --- a/tests/base_tests.py +++ b/tests/base_tests.py @@ -324,6 +324,7 @@ def run_sql( tmp_table_name=None, schema=None, ctas_method=CtasMethod.TABLE, + template_params="{}", ): if user_name: self.logout() @@ -336,6 +337,7 @@ def run_sql( "queryLimit": query_limit, "sql_editor_id": sql_editor_id, "ctas_method": ctas_method, + "templateParams": template_params, } if tmp_table_name: json_payload["tmp_table_name"] = tmp_table_name diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index 52fd32c488400..59ce8adeb03bf 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -552,7 +552,6 @@ def test_query_admin_can_access_all_queries(self) -> None: url = "/api/v1/query/" data = self.get_json_resp(url) - admin = security_manager.find_user("admin") self.assertEqual(3, len(data["result"])) def test_api_database(self): @@ -576,3 +575,23 @@ def test_api_database(self): {r.get("database_name") for r in self.get_json_resp(url)["result"]}, ) self.delete_fake_db() + + @mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + {"ENABLE_TEMPLATE_PROCESSING": True}, + clear=True, + ) + def test_sql_json_parameter_error(self): + self.login("admin") + + data = self.run_sql( + "SELECT * FROM birth_names WHERE state = '{{ state }}' LIMIT 10", + "1", + template_params=json.dumps({"state": "CA"}), + ) + assert data["status"] == "success" + + data = self.run_sql( + "SELECT * FROM birth_names WHERE state = '{{ state }}' LIMIT 10", "2" + ) + assert data["errors"][0]["error_type"] == "MISSING_TEMPLATE_PARAMS_ERROR"