You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2024/03/11 16:57:24 UTC
[PR] fix: pass valid SQL to SM [superset]
betodealmeida opened a new pull request, #27464:
URL: https://github.com/apache/superset/pull/27464
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
When creating a virtual dataset we pass the unprocessed SQL to security manager. If the SQL contains Jinja it might not be valid and might fail to parse, which causes the security manager to allow the query to run (see https://github.com/apache/superset/pull/26748).
This PR fixes it by processing the templated SQL before passing it to the security manager. See the discussion in https://github.com/apache/superset/pull/26476.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1529134773
##########
tests/unit_tests/commands/dataset/__init__.py:
##########
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
@betodealmeida I gather this file is now obsolete and can be deleted.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1521352342
##########
superset/commands/dataset/create.py:
##########
@@ -85,10 +88,12 @@ def validate(self) -> None:
exceptions.append(TableNotFoundValidationError(table_name))
if sql:
+ processor = jinja_context.get_template_processor(database=database)
Review Comment:
@john-bodley as far as I could tell, in those places it's already handled correctly. For SQL Lab I can confirm that the template is processed before being passed to the security manager for parsing. I'll test with ad-hoc filters, but when I inspected the code I only saw this place in need of fixing.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1529154339
##########
tests/unit_tests/commands/dataset/__init__.py:
##########
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Ah, good point.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1520265947
##########
superset/commands/dataset/create.py:
##########
@@ -85,10 +88,12 @@ def validate(self) -> None:
exceptions.append(TableNotFoundValidationError(table_name))
if sql:
+ processor = jinja_context.get_template_processor(database=database)
Review Comment:
@betodealmeida I think we need this in more places than just virtual datasets per my comment in the PR, i.e., a chart (via an ad-hoc filter), SQL Lab query, etc. could contain a Jinja template.
Should this two-phase approach be part of the `ParsedQuery` class so it's applicable to all use cases?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #27464:
URL: https://github.com/apache/superset/pull/27464#issuecomment-1992685491
For reference [here's](https://github.com/apache/superset/blob/85d0d88fc276e147c51056aec87854e5feb4176e/superset/sqllab/query_render.py#L61-L70) another place in code (for right or wrong) which renders the Jinja template (for validation purposes), yet faces the same chicken-and-egg situation where any `latest[_sub]_partition` macros are executed prior to checking if the user has access.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27464:
URL: https://github.com/apache/superset/pull/27464#issuecomment-1988965018
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27464?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 59.71%. Comparing base [(`fde93dc`)](https://app.codecov.io/gh/apache/superset/commit/fde93dcf08122d8b41ca296213e09616d1b71782?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`1f8c47a`)](https://app.codecov.io/gh/apache/superset/pull/27464?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 2 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #27464 +/- ##
==========================================
- Coverage 67.34% 59.71% -7.64%
==========================================
Files 1909 1909
Lines 74623 74667 +44
Branches 8324 8324
==========================================
- Hits 50255 44587 -5668
- Misses 22314 28026 +5712
Partials 2054 2054
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [mysql](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.67% <50.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.40% <100.00%> (-15.83%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/27464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.61% <100.00%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27464?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1520393408
##########
superset/commands/dataset/create.py:
##########
@@ -85,10 +88,12 @@ def validate(self) -> None:
exceptions.append(TableNotFoundValidationError(table_name))
if sql:
+ processor = jinja_context.get_template_processor(database=database)
+ rendered_sql = processor.process_template(sql)
Review Comment:
The additional challenge is that—depending on the macro—an actual query may need to be executed in order to process the template, i.e., the `[hive|presto|trino].latest_partition` Jinja macro actually executes a query in order to resolve the macro. This results in somewhat of a chicken-and-egg scenario, i.e., one has to _try_ to execute a query (on behalf of the user) in order to resolve whether the user has access to it.
Furthermore if the user doesn't have access to the tables which are used to resolve the `[hive|presto|trino].latest_partition` Jinja macro then line #92 will raise an `SupersetDBAPIProgrammingError` (or similar).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1520265947
##########
superset/commands/dataset/create.py:
##########
@@ -85,10 +88,12 @@ def validate(self) -> None:
exceptions.append(TableNotFoundValidationError(table_name))
if sql:
+ processor = jinja_context.get_template_processor(database=database)
Review Comment:
@betodealmeida I think we need this in more places than just virtual datasets per my comment in the PR, i.e., a chart (via an ad-hoc filter), SQL Lab query, etc. could contain a Jinja template.
My sense is this two-phase approach be part of the `ParsedQuery` class so it's applicable to all use cases.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1520393408
##########
superset/commands/dataset/create.py:
##########
@@ -85,10 +88,12 @@ def validate(self) -> None:
exceptions.append(TableNotFoundValidationError(table_name))
if sql:
+ processor = jinja_context.get_template_processor(database=database)
+ rendered_sql = processor.process_template(sql)
Review Comment:
The additional challenge is that—depending on the macro—an actual query may need to be executed in order to process the template, i.e., the `[hive|presto|trino].latest_partition` Jinja macro actually executes a query in order to resolve the macro. This results in somewhat of a chicken-and-egg scenario, i.e., one has to _try_ to execute a query (on behalf of the user) in order to resolve whether the user has access to it.
Furthermore if the user doesn't have access to the tables which are used to resolve the `[hive|presto|trino].latest_partition` Jinja macro then line #92 will raise an `SupersetDBAPIProgrammingError` (or similar).
I'm still not sure what the ideal solution is here. Without doubt this is non-trivial.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #27464:
URL: https://github.com/apache/superset/pull/27464#issuecomment-1989433700
Regarding https://github.com/apache/superset/pull/27464#discussion_r1520265947 and https://github.com/apache/superset/pull/27464#discussion_r1520393408, I was thinking maybe an alternative approach would be a multi-phase approach for the `ParsedQuery` table extraction where:
- Phase I: Identify all the Jinja macros—hopefully by leveraging Jinja functionality—and extract any table names present, i.e., we would need to parse (likely via SQLGlot) the macros
```python
>>> from sqlglot import parse
>>> parse("trino.latest_partition('foo')")
[Dot(
this=Identifier(this=trino, quoted=False),
expression=Anonymous(
this=latest_partition,
expressions=[
Literal(this=foo, is_string=True)]))]
```
- Phase II: Replace the Jinja macros with some SQL placeholder* and then parse the full query to extract any remaining table names.
The extracted set of tables is then the union of phase I and phase II.
\* I wonder if `NULL` would work for all scenarios. We don't need to generate SQL which is actually executable, but merely SQL which SQLGlot will be able to successfully parse.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #27464:
URL: https://github.com/apache/superset/pull/27464#issuecomment-1991531350
@john-bodley why not simply translate the macro (by executing queries, if necessary) in order to have the actual query that's going to run, so we can check for all the tables used? I think it's the best way to prevent a tight coupling between Jinja templates and SQL parsing, and I'm worried that with the approach you're proposing we might have errors in the future when people introduce new macros and the logic to extract tables misses them.
Is your concern strictly performance (ie, running a query that might be expensive only for the security manager to block the main query)? Or are you worried that it might leak metadata, eg, with a query like this, where the user doesn't have access to table `bar`:
```sql
SELECT '{{ latest_partition('bar') }}';
```
Is a user knowing the latest partition of a table they don't have access to a problem? One solution to this problem would be checking the macro queries against the security manager as well, so the process would be:
1. Render all macros
2. When macros like `latest_parititon` render, they run their queries against the security manager before executing them
3. Run the rendered query against the security manager
4. Run query
Though it again requires new macros to adopt this pattern (of doing step 2) if we want to keep Superset secure.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #27464:
URL: https://github.com/apache/superset/pull/27464#issuecomment-1992376478
@betodealmeida I raised the issue with simply rendering all macros [here](https://github.com/apache/superset/pull/27464#discussion_r1520393408). The challenge is the `[hive|presto|trino].latest[_sub]_partition` macros likely will resolve to Hive, Presto, or Trino queries which need to be executed, however the user may not have access to the underlying table(s)—hence my chicken-and-egg statement. I don't believe it's viable to first run a precursor query in order to then resolve which tables are associated with the query and whether said user has access to them.
The logic I outlined in https://github.com/apache/superset/pull/27470 doesn't render all macros, which @michael-s-molina, @tobyma, et. al have correctly pointed out could be problematic—especially if there's SQL injection which likely would come from the `url_param` macro.
Regarding your four steps I think they would need to be tweaked—in essence a hybrid of the logic in this PR and https://github.com/apache/superset/pull/27470 as the `latest_[_sub]_parititon` macros need special treatment. Specifically reordering/tweaking steps 1 and 2 and tweaking steps 3 and 4:
1. Via Jinja extract the table referenced in the `latest_[_sub]_parititon` macro and replace the contents with some benign query.
2. Render all remaining macros.
3. Extract the tables from the rendered query and combine with 1.
4. Verify that via the security manager that the user has access to said tables.
5. Run query.
https://github.com/apache/superset/pull/27470 currently defined steps 1, 3, 4, and 5. The only missing part is 2, rendering the SQL query to resolve any remaining macros.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1529133824
##########
superset/security/manager.py:
##########
@@ -1956,11 +1957,14 @@ def raise_for_access(
return
if query:
+ # make sure the quuery is valid SQL by rendering any Jinja
Review Comment:
```suggestion
# make sure the query is valid SQL by rendering any Jinja
```
##########
tests/unit_tests/commands/dataset/__init__.py:
##########
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
@betodealmeida I gather this file is now obsolete.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #27464:
URL: https://github.com/apache/superset/pull/27464#discussion_r1529154003
##########
superset/security/manager.py:
##########
@@ -1956,11 +1957,14 @@ def raise_for_access(
return
if query:
+ # make sure the quuery is valid SQL by rendering any Jinja
Review Comment:
Good catch! Let me fix this in another PR so we can merge this already.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #27464:
URL: https://github.com/apache/superset/pull/27464
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: pass valid SQL to SM [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #27464:
URL: https://github.com/apache/superset/pull/27464#issuecomment-1989170716
Thanks @betodealmeida for authoring this fix. Thinking a bit about this over the weekend, I think this—a two phase approach—is likely the right technique.
Interestingly previously using `sqlparse` (3.0.3) the following queries returned different results,
```python
>>> from superset.sql_parse import ParsedQuery
>>> ParsedQuery("SELECT * FROM foo WHERE ds = '{{ presto.latest_partition('foo') }}'").tables
{Table(table='foo', schema=None, catalog=None)}
>>> ParsedQuery("SELECT '{{ presto.latest_partition('foo') }}'").tables
set()
```
i.e., Superset only identified the `foo` table from the `FROM` clause and didn't identify the table from within the Jinja macro.
Finally, @betodealmeida regarding your comment,
> When creating a virtual dataset we pass the unprocessed SQL to security manager.
This could also be a physical dataset which contains a freeform SQL filter containing a Jinja macro.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org