feat: show missing parameters in query #12049
Conversation
| }} | ||
| code={qe.templateParams} | ||
| /> | ||
| {isFeatureEnabled(FeatureFlag.ENABLE_TEMPLATE_PROCESSING) && ( |
There was a problem hiding this comment.
We are showing "Parameters" in SQL Lab even though the functionality is now behind a feature flag in the backend. I turned it off here if the functionality is not enabled.
cc: @hughhhh
Codecov Report
@@ Coverage Diff @@
## master #12049 +/- ##
==========================================
- Coverage 66.58% 63.48% -3.10%
==========================================
Files 952 957 +5
Lines 46703 47010 +307
Branches 4578 4594 +16
==========================================
- Hits 31096 29845 -1251
- Misses 15458 16980 +1522
- Partials 149 185 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| ast = template_processor._env.parse(rendered_query) | ||
| undefined = find_undeclared_variables(ast) # type: ignore |
There was a problem hiding this comment.
@robdiciuccio want to confirm that this is ok?
There was a problem hiding this comment.
It's not guaranteed that the template processor with be Jinja-based, due to how CUSTOM_TEMPLATE_PROCESSORS operates. I suggest checking that template_processor is an instance of JinjaTemplateProcessor here instead of checking ENABLE_TEMPLATE_PROCESSING.
There was a problem hiding this comment.
Other than that, I don't see any security issues with using find_undeclared_variables in this context. I'm assuming that undefined variables don't raise an error during process_template?
| @@ -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) | |||
There was a problem hiding this comment.
Is DebugUndefined required to make find_undeclared_variables function? Doesn't seem to be a requirement in the docs.
| @@ -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) | |||
There was a problem hiding this comment.
Nit: accessing a private property from outside the class
mistercrunch
added
🏷️ bot
SUMMARY
This PR makes SQL Lab discriminate errors that happen because of missing parameters in the query, showing them as "Parameter Error" instead of "Database Error".
The error component in SQL Lab seems to be outdated, but I didn't want to change it now since we would like to include this fix before 1.0. Since I'm using SIP-40 for the error in the backend, hopefully we can migrate SQL Lab to use something like
<DatabaseErrorMessage />and it will Just Work™.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Tested manually (see screenshot) and also added a unit test.
ADDITIONAL INFORMATION