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/01/22 18:44:41 UTC
[PR] feat: improve _extract_tables_from_sql [superset]
betodealmeida opened a new pull request, #26748:
URL: https://github.com/apache/superset/pull/26748
<!---
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 -->
Improve `_extract_tables_from_sql` from https://github.com/apache/superset/pull/26476 so that a proper exception is returned if the SQL fails to parse when extracting tables mentioned in a query. The previous behavior was to return an empty set of tables in the query.
### 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? -->
Updated unit tests.
### 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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #26748:
URL: https://github.com/apache/superset/pull/26748
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #26748:
URL: https://github.com/apache/superset/pull/26748#issuecomment-1989409073
The TL;DR is (per learnings during my internal postmortem writeup) `sqlparse` is a non-validating SQL parser whereas SQLGlot is a pseudo-validating* SQL parser. I'm not saying one is right or wrong, but sqlparse is definitely more lenient which could be perceived as beneficial for out specific use case, i.e., trying to identify the tables that a SQL query (be it valid or not) is referencing.
\* SQLGlot doesn't claim to validate SQL (per its [README.md](https://github.com/tobymao/sqlglot/blob/main/README.md)),
> SQLGlot does not aim to be a SQL validator - it is designed to be very forgiving
it does seem to be somewhat opinionated (understandably so, i.e., if you're unable to successfully parse token(s) anything downstream in the AST is likely deemed questionable).
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #26748:
URL: https://github.com/apache/superset/pull/26748#discussion_r1520212456
##########
superset/sql_parse.py:
##########
@@ -287,9 +292,15 @@ def _extract_tables_from_sql(self) -> set[Table]:
"""
try:
statements = parse(self.sql, dialect=self._dialect)
- except ParseError:
- logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
- return set()
+ except ParseError as ex:
+ dialect = self._dialect or "generic"
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ message=__(f"Unable to parse SQL ({dialect}): {self.sql}"),
+ level=ErrorLevel.ERROR,
Review Comment:
@betodealmeida I _think_ having Superset being less lenient/blasé is likely a good thing, i.e., previously it blindly would could have* given a user access whenever it wasn't able to parse the SQL, irrespective of whether the user had access to the underly data.
The major challenge I see is if SQLGlot wrongfully isn't able to parse a SQL statement and thus we're wrongfully blocking a query. How do we help unblock users here?
\* This is dependent on how one configured their security manager.
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #26748:
URL: https://github.com/apache/superset/pull/26748#discussion_r1462650020
##########
superset/sql_parse.py:
##########
@@ -287,9 +292,15 @@ def _extract_tables_from_sql(self) -> set[Table]:
"""
try:
statements = parse(self.sql, dialect=self._dialect)
- except ParseError:
- logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
- return set()
+ except ParseError as ex:
+ dialect = self._dialect or "generic"
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ message=__(f"Unable to parse SQL ({dialect}): {self.sql}"),
+ level=ErrorLevel.ERROR,
Review Comment:
Yeah, I'm worried about that too, and I think that's why originally the behavior was to not to fail. But the alternative is allowing people to run queries on tables they don't have access because we fail to extract the reference from the SQL.
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #26748:
URL: https://github.com/apache/superset/pull/26748#discussion_r1462464381
##########
superset/sql_parse.py:
##########
@@ -287,9 +292,15 @@ def _extract_tables_from_sql(self) -> set[Table]:
"""
try:
statements = parse(self.sql, dialect=self._dialect)
- except ParseError:
- logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
- return set()
+ except ParseError as ex:
+ dialect = self._dialect or "generic"
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ message=__(f"Unable to parse SQL ({dialect}): {self.sql}"),
+ level=ErrorLevel.ERROR,
Review Comment:
It's hard to tell. It could be invalid SQL (user error), or it could be because limitations of the parser (system error).
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #26748:
URL: https://github.com/apache/superset/pull/26748#discussion_r1462648808
##########
superset/sql_parse.py:
##########
@@ -287,9 +292,15 @@ def _extract_tables_from_sql(self) -> set[Table]:
"""
try:
statements = parse(self.sql, dialect=self._dialect)
- except ParseError:
- logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
- return set()
+ except ParseError as ex:
+ dialect = self._dialect or "generic"
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ message=__(f"Unable to parse SQL ({dialect}): {self.sql}"),
+ level=ErrorLevel.ERROR,
Review Comment:
Ok, that makes sense. I'm thinking of a scenario where someone is making a lot invalid sql calls and it just keeps loading up a system monitoring queue with what looks to be an outage of a sort.
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #26748:
URL: https://github.com/apache/superset/pull/26748#discussion_r1462433110
##########
superset/sql_parse.py:
##########
@@ -287,9 +292,15 @@ def _extract_tables_from_sql(self) -> set[Table]:
"""
try:
statements = parse(self.sql, dialect=self._dialect)
- except ParseError:
- logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
- return set()
+ except ParseError as ex:
+ dialect = self._dialect or "generic"
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ message=__(f"Unable to parse SQL ({dialect}): {self.sql}"),
+ level=ErrorLevel.ERROR,
Review Comment:
@betodealmeida is the underlying error a user error or system error? Seems kind of like a user error that the user should try to fix, is that right?
--
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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26748:
URL: https://github.com/apache/superset/pull/26748#issuecomment-1904621621
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26748?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `3 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`ce26ecb`)](https://app.codecov.io/gh/apache/superset/commit/ce26ecbd9091467e3f4662d7bb00ca0a24ed7a05?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.48% compared to head [(`0fa0b3e`)](https://app.codecov.io/gh/apache/superset/pull/26748?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 67.14%.
> Report is 12 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/superset/pull/26748?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/sql\_parse.py](https://app.codecov.io/gh/apache/superset/pull/26748?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | 50.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/26748?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26748 +/- ##
==========================================
- Coverage 69.48% 67.14% -2.34%
==========================================
Files 1894 1894
Lines 74130 74132 +2
Branches 8243 8243
==========================================
- Hits 51507 49776 -1731
- Misses 20554 22287 +1733
Partials 2069 2069
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26748/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/26748/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/26748/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.98% <50.00%> (-0.03%)` | :arrow_down: |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/26748/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.10% <50.00%> (-0.03%)` | :arrow_down: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/26748/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/26748/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.16% <50.00%> (-4.85%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/26748/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/26748/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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/26748?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] feat: `improve _extract_tables_from_sql` [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #26748:
URL: https://github.com/apache/superset/pull/26748#discussion_r1462433110
##########
superset/sql_parse.py:
##########
@@ -287,9 +292,15 @@ def _extract_tables_from_sql(self) -> set[Table]:
"""
try:
statements = parse(self.sql, dialect=self._dialect)
- except ParseError:
- logger.warning("Unable to parse SQL (%s): %s", self._dialect, self.sql)
- return set()
+ except ParseError as ex:
+ dialect = self._dialect or "generic"
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR,
+ message=__(f"Unable to parse SQL ({dialect}): {self.sql}"),
+ level=ErrorLevel.ERROR,
Review Comment:
@betodealmeida is this a user error or system error?
--
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