You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/03/08 02:28:38 UTC
[GitHub] [superset] betodealmeida opened a new pull request #19055: feat: helper functions for RLS
betodealmeida opened a new pull request #19055:
URL: https://github.com/apache/superset/pull/19055
<!---
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 -->
This PR introduces 2 helper functions for RLS:
- `has_table_query(statement: Statement) -> bool:`, which analyzes a SQL statement parsed by`sqlparse` and returns `True` if it queries a table (either in a `FROM` or `JOIN`).
- `insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:`, which given a parsed statement (or token list), a table name, and a parsed RLS expression, will reformat the query so that the RLS is present in any queries or subqueries referencing the table.
For example, if we have the RLS expression `id=42` on the table `my_table` we could do:
```python
>>> statement = sqlparse.parse('SELECT COUNT(*) FROM my_table')[0]
>>> has_table_query(statement)
True
>>> rls = sqlparse.parse('id=42')[0]
>>> print(insert_rls(statement, 'my_table', rls)
SELECT COUNT(*) FROM my_table WHERE id=42
```
### 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? -->
I added unit tests covering different cases.
### 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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823044342
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
+ """
+ # make sure the identifier has the table name
+ add_table_name(rls, table)
+
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # Recurse into child token list
+ if isinstance(token, TokenList):
+ i = token_list.tokens.index(token)
+ token_list.tokens[i] = insert_rls(token, table, rls)
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN, test for table
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, Identifier) or token.ttype == Keyword
+ ):
+ if matches_table_name(token, table):
+ state = InsertRLSState.FOUND_TABLE
+
+ # found table at the end of the statement; append a WHERE clause
+ if token == token_list[-1]:
+ token_list.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Where(
+ [Token(Keyword, "WHERE"), Token(Whitespace, " "), rls]
+ ),
+ ]
+ )
+ return token_list
+
+ # Found WHERE clause, insert RLS if not present
+ elif state == InsertRLSState.FOUND_TABLE and isinstance(token, Where):
+ if str(rls) not in {str(t) for t in token.tokens}:
+ token.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Token(Keyword, "AND"),
+ Token(Whitespace, " "),
+ ]
+ + rls.tokens
+ )
+ state = InsertRLSState.SCANNING
+
+ # Found ON clause, insert RLS if not present
Review comment:
There's no check for the "if not present" part of this.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823857877
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
+ """
+ # make sure the identifier has the table name
+ add_table_name(rls, table)
+
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # Recurse into child token list
+ if isinstance(token, TokenList):
+ i = token_list.tokens.index(token)
+ token_list.tokens[i] = insert_rls(token, table, rls)
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN, test for table
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, Identifier) or token.ttype == Keyword
+ ):
+ if matches_table_name(token, table):
+ state = InsertRLSState.FOUND_TABLE
+
+ # found table at the end of the statement; append a WHERE clause
+ if token == token_list[-1]:
+ token_list.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Where(
+ [Token(Keyword, "WHERE"), Token(Whitespace, " "), rls]
+ ),
+ ]
+ )
+ return token_list
+
+ # Found WHERE clause, insert RLS if not present
+ elif state == InsertRLSState.FOUND_TABLE and isinstance(token, Where):
+ if str(rls) not in {str(t) for t in token.tokens}:
+ token.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Token(Keyword, "AND"),
+ Token(Whitespace, " "),
+ ]
+ + rls.tokens
+ )
+ state = InsertRLSState.SCANNING
+
+ # Found ON clause, insert RLS if not present
Review comment:
Oh, good catch!
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6942b9e) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.59% +0.02%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
+ Hits 42265 42324 +59
- Misses 19550 19554 +4
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `81.88% <100.00%> (+0.02%)` | :arrow_up: |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.31% <100.00%> (+0.02%)` | :arrow_up: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `95.13% <0.00%> (-0.55%)` | :arrow_down: |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.14% <0.00%> (-0.37%)` | :arrow_down: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...6942b9e](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] betodealmeida commented on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1065486520
Addressed all of @villebro's comments.
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5328cdf) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.13%`.
> The diff coverage is `77.66%`.
> :exclamation: Current head 5328cdf differs from pull request most recent head 746a919. Consider uploading reports for the commit 746a919 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.43% -0.14%
==========================================
Files 1641 1641
Lines 63495 63564 +69
Branches 6425 6425
==========================================
- Hits 42265 42226 -39
- Misses 19550 19658 +108
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.85% <100.00%> (+0.04%)` | :arrow_up: |
| postgres | `81.90% <100.00%> (+0.04%)` | :arrow_up: |
| presto | `?` | |
| python | `81.98% <100.00%> (-0.31%)` | :arrow_down: |
| sqlite | `81.59% <100.00%> (+0.04%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...chart-controls/src/shared-controls/dndControls.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `35.89% <ø> (ø)` | |
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `36.36% <ø> (ø)` | |
| [...perset-ui-core/src/chart/components/SuperChart.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9TdXBlckNoYXJ0LnRzeA==) | `100.00% <ø> (ø)` | |
| [...src/BigNumber/BigNumberWithTrendline/buildQuery.ts](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvYnVpbGRRdWVyeS50cw==) | `9.09% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/ResultSet/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC9pbmRleC50c3g=) | `50.73% <ø> (ø)` | |
| [...d/src/SqlLab/components/SaveDatasetModal/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwvaW5kZXgudHN4) | `71.42% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/SaveQuery/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVRdWVyeS9pbmRleC50c3g=) | `57.89% <ø> (ø)` | |
| [...rc/SqlLab/components/ScheduleQueryButton/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NjaGVkdWxlUXVlcnlCdXR0b24vaW5kZXgudHN4) | `19.23% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/SqlEditor/index.jsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci9pbmRleC5qc3g=) | `51.44% <ø> (ø)` | |
| [superset-frontend/src/SqlLab/fixtures.ts](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| ... and [209 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...746a919](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] betodealmeida edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1064329958
@suddjian I modified the logic to always include the RLS even if it's already present, since there are a few corner cases that are hard to identify. For example, if we have the RLS `user_id=1` and this query:
```sql
SELECT * FROM table
WHERE TRUE OR user_id=1
```
Even though we already have the token `Comparison(user_id=1)` in the `WHERE` clause we still need to apply since in this case the comparison is a no-op. So we need to add it:
```sql
SELECT * FROM table
WHERE TRUE OR user_id=1 AND user_id=1
```
More importantly, because of the precedence of `AND` over `OR`, we need to wrap the original predicate in parenthesis:
```sql
SELECT * FROM table
WHERE (TRUE OR user_id=1) AND user_id=1
```
Without parenthesis the predicate evaluates to `TRUE OR (user_id=1 AND user_id=1)`, which bypasses the RLS!
--
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
[GitHub] [superset] codecov[bot] commented on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (11c2bb3) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.02%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.53% -0.03%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
+ Hits 42265 42288 +23
- Misses 19550 19590 +40
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `?` | |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.20% <100.00%> (-0.10%)` | :arrow_down: |
| sqlite | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/sqllab/sql\_json\_executer.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL3NxbF9qc29uX2V4ZWN1dGVyLnB5) | `64.63% <0.00%> (-9.76%)` | :arrow_down: |
| [superset/sqllab/execution\_context\_convertor.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2V4ZWN1dGlvbl9jb250ZXh0X2NvbnZlcnRvci5weQ==) | `78.26% <0.00%> (-8.70%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `91.89% <0.00%> (-5.41%)` | :arrow_down: |
| [superset/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3V0aWxzLnB5) | `88.23% <0.00%> (-3.93%)` | :arrow_down: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `95.45% <0.00%> (-1.82%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...11c2bb3](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6942b9e) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 6942b9e differs from pull request most recent head 746a919. Consider uploading reports for the commit 746a919 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.59% +0.03%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
+ Hits 42265 42328 +63
Misses 19550 19550
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `81.89% <100.00%> (+0.03%)` | :arrow_up: |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.33% <100.00%> (+0.03%)` | :arrow_up: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.51% <0.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...746a919](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r825073242
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
Review comment:
Thanks for the explanation! That would actually be super useful as a code comment 😁
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (00598fa) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.61% +0.05%
==========================================
Files 1641 1641
Lines 63495 63583 +88
Branches 6425 6425
==========================================
+ Hits 42265 42358 +93
+ Misses 19550 19545 -5
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.53% <11.26%> (-0.07%)` | :arrow_down: |
| mysql | `81.87% <100.00%> (+0.06%)` | :arrow_up: |
| postgres | `81.92% <100.00%> (+0.06%)` | :arrow_up: |
| presto | `52.38% <11.26%> (-0.07%)` | :arrow_down: |
| python | `82.36% <100.00%> (+0.06%)` | :arrow_up: |
| sqlite | `81.67% <100.00%> (+0.12%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.95% <100.00%> (+0.34%)` | :arrow_up: |
| [superset/viz.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdml6LnB5) | `58.26% <0.00%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.51% <0.00%> (ø)` | |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `98.47% <0.00%> (+<0.01%)` | :arrow_up: |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
| [superset/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvZXhjZXB0aW9ucy5weQ==) | `92.98% <0.00%> (+0.25%)` | :arrow_up: |
| [superset/security/manager.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | `94.72% <0.00%> (+0.31%)` | :arrow_up: |
| [superset/common/query\_context\_processor.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfcHJvY2Vzc29yLnB5) | `91.46% <0.00%> (+0.47%)` | :arrow_up: |
| [superset/db\_engine\_specs/databricks.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RhdGFicmlja3MucHk=) | `90.90% <0.00%> (+0.90%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...00598fa](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e816857) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.54% -0.02%
==========================================
Files 1641 1641
Lines 63495 63569 +74
Branches 6425 6425
==========================================
+ Hits 42265 42303 +38
- Misses 19550 19586 +36
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.94%> (-0.09%)` | :arrow_down: |
| mysql | `81.86% <100.00%> (+0.05%)` | :arrow_up: |
| postgres | `?` | |
| presto | `52.35% <11.94%> (-0.09%)` | :arrow_down: |
| python | `82.22% <100.00%> (-0.08%)` | :arrow_down: |
| sqlite | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.93% <100.00%> (+0.32%)` | :arrow_up: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/sqllab/sql\_json\_executer.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL3NxbF9qc29uX2V4ZWN1dGVyLnB5) | `64.63% <0.00%> (-9.76%)` | :arrow_down: |
| [superset/sqllab/execution\_context\_convertor.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2V4ZWN1dGlvbl9jb250ZXh0X2NvbnZlcnRvci5weQ==) | `78.26% <0.00%> (-8.70%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `91.89% <0.00%> (-5.41%)` | :arrow_down: |
| [superset/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3V0aWxzLnB5) | `88.23% <0.00%> (-3.93%)` | :arrow_down: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `95.45% <0.00%> (-1.82%)` | :arrow_down: |
| ... and [17 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...e816857](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] betodealmeida edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1064329958
@suddjian I modified the logic to always include the RLS even if it's already present, since there are a few corner cases that are hard to identify. For example, if we have the RLS `user_id=1` and this query:
```sql
SELECT * FROM table
WHERE TRUE OR user_id=1
```
Even though we already have the token `Comparison(user_id=1)` in the `WHERE` clause we still need to apply since in this case the comparison is a no-op. So we need to add it:
```sql
SELECT * FROM table
WHERE TRUE OR user_id=1 AND user_id=1
```
More importantly, because of the precedence of `AND` over `OR`, we need to wrap the original predicate in parenthesis:
```sql
SELECT * FROM table
WHERE (TRUE OR user_id=1) AND user_id=1
```
Without parenthesis the predicate evaluates to `TRUE OR (user_id=1 AND user_id=1)`, which bypasses the RLS!
I implemented the logic to wrap the original predicate and added tests covering it.
--
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
[GitHub] [superset] betodealmeida commented on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1064329958
@suddjian I modified the logic to always include the RLS even if it's already present, since there are a few corner cases that are hard to identify. For example, if we have the RLS `user_id=1` and this query:
```sql
SELECT * FROM table
WHERE TRUE OR user_id=1
```
Even though we already have the token `Comparison(user_id=1)` in the `WHERE` clause we still need to apply since in this case the clause is a no-op. So we need to add it:
```sql
SELECT * FROM table
WHERE TRUE OR user_id=1 AND user_id=1
```
More importantly, because of the precedence of `AND` over `OR`, we need to wrap the original predicate in parenthesis:
```sql
SELECT * FROM table
WHERE (TRUE OR user_id=1) AND user_id=1
```
Without parenthesis the predicate evaluates to `TRUE OR (user_id=1 AND user_id=1)`, which bypasses the RLS!
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r824762461
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
Review comment:
Ah, good point, @villebro! I'll add more tests.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823863876
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
+ """
+ # make sure the identifier has the table name
+ add_table_name(rls, table)
+
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # Recurse into child token list
+ if isinstance(token, TokenList):
+ i = token_list.tokens.index(token)
+ token_list.tokens[i] = insert_rls(token, table, rls)
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN, test for table
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, Identifier) or token.ttype == Keyword
+ ):
+ if matches_table_name(token, table):
+ state = InsertRLSState.FOUND_TABLE
+
+ # found table at the end of the statement; append a WHERE clause
+ if token == token_list[-1]:
+ token_list.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Where(
+ [Token(Keyword, "WHERE"), Token(Whitespace, " "), rls]
+ ),
+ ]
+ )
+ return token_list
+
+ # Found WHERE clause, insert RLS if not present
+ elif state == InsertRLSState.FOUND_TABLE and isinstance(token, Where):
+ if str(rls) not in {str(t) for t in token.tokens}:
+ token.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Token(Keyword, "AND"),
+ Token(Whitespace, " "),
+ ]
+ + rls.tokens
+ )
+ state = InsertRLSState.SCANNING
+
+ # Found ON clause, insert RLS if not present
+ elif (
+ state == InsertRLSState.FOUND_TABLE
+ and token.ttype == Keyword
+ and token.value.upper() == "ON"
+ ):
+ i = token_list.tokens.index(token)
+ token.parent.tokens[i + 1 : i + 1] = [
+ Token(Whitespace, " "),
+ rls,
+ Token(Whitespace, " "),
+ Token(Keyword, "AND"),
+ ]
+ state = InsertRLSState.SCANNING
+
+ # Found table but no WHERE clause found, insert one
+ elif state == InsertRLSState.FOUND_TABLE and token.ttype != Whitespace:
+ i = token_list.tokens.index(token)
+
+ # Left pad with space, if needed
+ if i > 0 and token_list.tokens[i - 1].ttype != Whitespace:
+ token_list.tokens.insert(i, Token(Whitespace, " "))
+ i += 1
+
+ # Insert predicate
+ token_list.tokens.insert(
+ i, Where([Token(Keyword, "WHERE"), Token(Whitespace, " "), rls]),
+ )
+
+ # Right pad with space, if needed
Review comment:
I think it's because it makes it easier to convert the parse tree back to a string. Not sure.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823851671
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
Review comment:
Ah, thanks! I changed the function and forgot to update the docstring, will fix.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823854840
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
Review comment:
If we scan left to right then `schema.table != table`, since we'd compare `schema` with `table`. Doing it right to left we compare `table` with `table`, and stop because one side has no more tokens.
The goal here is to be conservative. If we have an RLS on `my_schema.my_table` and the query references only `my_table`, without the explicit schema, we don't know if they represent the same table unless we know the context in which the query is being executed (ie, what is the default schema when the query runs). This method will err on the side of applying the RLS to tables that maybe shouldn't, instead of not applying when it should.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823855884
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
+ """
+ # make sure the identifier has the table name
+ add_table_name(rls, table)
+
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # Recurse into child token list
+ if isinstance(token, TokenList):
+ i = token_list.tokens.index(token)
+ token_list.tokens[i] = insert_rls(token, table, rls)
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN, test for table
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, Identifier) or token.ttype == Keyword
+ ):
+ if matches_table_name(token, table):
+ state = InsertRLSState.FOUND_TABLE
+
+ # found table at the end of the statement; append a WHERE clause
Review comment:
Good point, I'll add. It should be covered by the function, this is an edge case where the iteration ended because the table was the last token, so we can't add the RLS on the next step of the loop.
--
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
[GitHub] [superset] betodealmeida merged pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #19055:
URL: https://github.com/apache/superset/pull/19055
--
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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822908745
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
Review comment:
It doesn't matter whether we scan this from right to left or left to right... 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
[GitHub] [superset] john-bodley commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822090110
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
Review comment:
```suggestion
if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
```
Note this would require the `from sqlparse.utils import imt` import.
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
Review comment:
@betodealmeida there's also [this](https://github.com/andialbrecht/sqlparse/blob/master/examples/extract_table_names.py) example which has logic for identifying tables.
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
Review comment:
You likely could just do `for token in stmt.flatten():` and remove the logic from lines 483–485.
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+ elif seen_source and token.ttype not in (Whitespace, Punctuation):
+ seen_source = False
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
Review comment:
You likely could use `flatten` here. It uses a generator so likely a copy should be made given you're mutating the tokens, i.e.,
```python
for token in list(rls.flatten()):
if imt(token, i=Identifier) and token.get_parent_name() is None:
...
```
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
Review comment:
The challenge here is there's no strong connection to ensure that the consecutive (or near consecutive) tokens are those which are being identified here. I guess the question is how robust do we want this logic. The proposed solution may well we suffice.
The _correct_ way of doing this is more of a tree traversal (as opposed to a flattened list) where one checks the next token (which could be a group) from the `FROM` or `JOIN` keyword and iterate from there.
My sense is that can likely be addressed later. We probably need to cleanup the `sqlparse` logic to junk it completely in favor of something else given that it seems like the package is somewhat on life support.
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+ elif seen_source and token.ttype not in (Whitespace, Punctuation):
+ seen_source = False
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
Review comment:
Should there be specific unit tests—docstring might be suffice—for this function?
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822128924
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
Review comment:
I don't remember the details, but I've had issues with that example code before — I think it failed to identify table names when they were considered keywords (even though the example calls it out).
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r824770343
##########
File path: tests/unit_tests/sql_parse_tests.py
##########
@@ -1189,3 +1193,225 @@ def test_sqlparse_issue_652():
stmt = sqlparse.parse(r"foo = '\' AND bar = 'baz'")[0]
assert len(stmt.tokens) == 5
assert str(stmt.tokens[0]) == "foo = '\\'"
+
+
+@pytest.mark.parametrize(
+ "sql,expected",
+ [
+ ("SELECT * FROM table", True),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN (SELECT * FROM table)", True),
+ ("(SELECT COUNT(DISTINCT name) AS foo FROM birth_names)", True),
+ ("COUNT(*)", False),
+ ("SELECT a FROM (SELECT 1 AS a)", False),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN table", True),
+ ("SELECT * FROM (SELECT 1 AS foo, 2 AS bar) ORDER BY foo ASC, bar", False),
+ ("SELECT * FROM other_table", True),
+ ],
+)
Review comment:
Ah, let me add it. Thanks!
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822115838
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+ elif seen_source and token.ttype not in (Whitespace, Punctuation):
+ seen_source = False
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
Review comment:
Good call, I'll add some!
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6942b9e) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.15%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.41% -0.16%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
- Hits 42265 42210 -55
- Misses 19550 19668 +118
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `81.95% <100.00%> (-0.35%)` | :arrow_down: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `31.37% <0.00%> (-56.87%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `60.34% <0.00%> (-20.69%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/common/utils/dataframe\_utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3V0aWxzL2RhdGFmcmFtZV91dGlscy5weQ==) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `87.98% <0.00%> (-6.01%)` | :arrow_down: |
| [superset/databases/dao.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2Rhby5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
| [superset/views/database/validators.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvdmFsaWRhdG9ycy5weQ==) | `78.94% <0.00%> (-5.27%)` | :arrow_down: |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `94.27% <0.00%> (-4.20%)` | :arrow_down: |
| ... and [14 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...6942b9e](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (11c2bb3) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
=======================================
Coverage 66.56% 66.57%
=======================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
=======================================
+ Hits 42265 42312 +47
- Misses 19550 19566 +16
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `?` | |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.27% <100.00%> (-0.02%)` | :arrow_down: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `95.45% <0.00%> (-1.82%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `95.13% <0.00%> (-0.55%)` | :arrow_down: |
| [superset/views/base\_api.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvYmFzZV9hcGkucHk=) | `97.47% <0.00%> (-0.43%)` | :arrow_down: |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.14% <0.00%> (-0.37%)` | :arrow_down: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...11c2bb3](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822120474
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+ elif seen_source and token.ttype not in (Whitespace, Punctuation):
+ seen_source = False
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
Review comment:
Same issue, if we call `.flatten()` we would never get an `Identifier`.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822115077
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
Review comment:
`.flatten()` is a bit different in that it returns the leaf nodes only, converting an identifier into 1+ `Name` tokens:
```python
>>> list(sqlparse.parse('SELECT * FROM my_table')[0].flatten())
[<DML 'SELECT' at 0x10FF019A0>, <Whitespace ' ' at 0x10FF01D00>, <Wildcard '*' at 0x10FF01D60>, <Whitespace ' ' at 0x10FF01DC0>, <Keyword 'FROM' at 0x10FF01E20>, <Whitespace ' ' at 0x10FF01E80>, <Name 'my_tab...' at 0x10FF01EE0>]
```
Since I'm looking for identifiers after a `FROM` or `JOIN` I thought it was easier to implement a traversal logic that actually inspects the parents, not just the leaves.
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5328cdf) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.13%`.
> The diff coverage is `77.66%`.
> :exclamation: Current head 5328cdf differs from pull request most recent head 5b32f9b. Consider uploading reports for the commit 5b32f9b to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.43% -0.14%
==========================================
Files 1641 1641
Lines 63495 63564 +69
Branches 6425 6425
==========================================
- Hits 42265 42226 -39
- Misses 19550 19658 +108
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.85% <100.00%> (+0.04%)` | :arrow_up: |
| postgres | `81.90% <100.00%> (+0.04%)` | :arrow_up: |
| presto | `?` | |
| python | `81.98% <100.00%> (-0.31%)` | :arrow_down: |
| sqlite | `81.59% <100.00%> (+0.04%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...chart-controls/src/shared-controls/dndControls.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `35.89% <ø> (ø)` | |
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `36.36% <ø> (ø)` | |
| [...perset-ui-core/src/chart/components/SuperChart.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9TdXBlckNoYXJ0LnRzeA==) | `100.00% <ø> (ø)` | |
| [...src/BigNumber/BigNumberWithTrendline/buildQuery.ts](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvYnVpbGRRdWVyeS50cw==) | `9.09% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/ResultSet/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC9pbmRleC50c3g=) | `50.73% <ø> (ø)` | |
| [...d/src/SqlLab/components/SaveDatasetModal/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwvaW5kZXgudHN4) | `71.42% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/SaveQuery/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVRdWVyeS9pbmRleC50c3g=) | `57.89% <ø> (ø)` | |
| [...rc/SqlLab/components/ScheduleQueryButton/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NjaGVkdWxlUXVlcnlCdXR0b24vaW5kZXgudHN4) | `19.23% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/SqlEditor/index.jsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci9pbmRleC5qc3g=) | `51.44% <ø> (ø)` | |
| [superset-frontend/src/SqlLab/fixtures.ts](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| ... and [209 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...5b32f9b](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (11c2bb3) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.59% +0.03%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
+ Hits 42265 42328 +63
Misses 19550 19550
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `81.89% <100.00%> (+0.03%)` | :arrow_up: |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.33% <100.00%> (+0.03%)` | :arrow_up: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.51% <0.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...11c2bb3](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (11c2bb3) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 11c2bb3 differs from pull request most recent head 6942b9e. Consider uploading reports for the commit 6942b9e to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.59% +0.03%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
+ Hits 42265 42328 +63
Misses 19550 19550
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `81.89% <100.00%> (+0.03%)` | :arrow_up: |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.33% <100.00%> (+0.03%)` | :arrow_up: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.51% <0.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...6942b9e](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5328cdf) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.13%`.
> The diff coverage is `77.66%`.
> :exclamation: Current head 5328cdf differs from pull request most recent head b5bfb94. Consider uploading reports for the commit b5bfb94 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.43% -0.14%
==========================================
Files 1641 1641
Lines 63495 63564 +69
Branches 6425 6425
==========================================
- Hits 42265 42226 -39
- Misses 19550 19658 +108
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.85% <100.00%> (+0.04%)` | :arrow_up: |
| postgres | `81.90% <100.00%> (+0.04%)` | :arrow_up: |
| presto | `?` | |
| python | `81.98% <100.00%> (-0.31%)` | :arrow_down: |
| sqlite | `81.59% <100.00%> (+0.04%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...chart-controls/src/shared-controls/dndControls.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `35.89% <ø> (ø)` | |
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `36.36% <ø> (ø)` | |
| [...perset-ui-core/src/chart/components/SuperChart.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY2hhcnQvY29tcG9uZW50cy9TdXBlckNoYXJ0LnRzeA==) | `100.00% <ø> (ø)` | |
| [...src/BigNumber/BigNumberWithTrendline/buildQuery.ts](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQmlnTnVtYmVyL0JpZ051bWJlcldpdGhUcmVuZGxpbmUvYnVpbGRRdWVyeS50cw==) | `9.09% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/ResultSet/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC9pbmRleC50c3g=) | `50.73% <ø> (ø)` | |
| [...d/src/SqlLab/components/SaveDatasetModal/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwvaW5kZXgudHN4) | `71.42% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/SaveQuery/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVRdWVyeS9pbmRleC50c3g=) | `57.89% <ø> (ø)` | |
| [...rc/SqlLab/components/ScheduleQueryButton/index.tsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NjaGVkdWxlUXVlcnlCdXR0b24vaW5kZXgudHN4) | `19.23% <ø> (ø)` | |
| [...frontend/src/SqlLab/components/SqlEditor/index.jsx](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci9pbmRleC5qc3g=) | `51.44% <ø> (ø)` | |
| [superset-frontend/src/SqlLab/fixtures.ts](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| ... and [209 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...b5bfb94](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823057159
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
+ """
+ # make sure the identifier has the table name
+ add_table_name(rls, table)
+
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # Recurse into child token list
+ if isinstance(token, TokenList):
+ i = token_list.tokens.index(token)
+ token_list.tokens[i] = insert_rls(token, table, rls)
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN, test for table
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, Identifier) or token.ttype == Keyword
+ ):
+ if matches_table_name(token, table):
+ state = InsertRLSState.FOUND_TABLE
+
+ # found table at the end of the statement; append a WHERE clause
Review comment:
should add a test case where the query has trailing whitespace
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822112553
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
Review comment:
Thanks #TIL
--
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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823007021
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
+ """
+ # make sure the identifier has the table name
+ add_table_name(rls, table)
+
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # Recurse into child token list
+ if isinstance(token, TokenList):
+ i = token_list.tokens.index(token)
+ token_list.tokens[i] = insert_rls(token, table, rls)
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN, test for table
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, Identifier) or token.ttype == Keyword
+ ):
+ if matches_table_name(token, table):
+ state = InsertRLSState.FOUND_TABLE
+
+ # found table at the end of the statement; append a WHERE clause
+ if token == token_list[-1]:
+ token_list.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Where(
+ [Token(Keyword, "WHERE"), Token(Whitespace, " "), rls]
+ ),
+ ]
+ )
+ return token_list
+
+ # Found WHERE clause, insert RLS if not present
+ elif state == InsertRLSState.FOUND_TABLE and isinstance(token, Where):
+ if str(rls) not in {str(t) for t in token.tokens}:
+ token.tokens.extend(
+ [
+ Token(Whitespace, " "),
+ Token(Keyword, "AND"),
+ Token(Whitespace, " "),
+ ]
+ + rls.tokens
+ )
+ state = InsertRLSState.SCANNING
+
+ # Found ON clause, insert RLS if not present
+ elif (
+ state == InsertRLSState.FOUND_TABLE
+ and token.ttype == Keyword
+ and token.value.upper() == "ON"
+ ):
+ i = token_list.tokens.index(token)
+ token.parent.tokens[i + 1 : i + 1] = [
+ Token(Whitespace, " "),
+ rls,
+ Token(Whitespace, " "),
+ Token(Keyword, "AND"),
+ ]
+ state = InsertRLSState.SCANNING
+
+ # Found table but no WHERE clause found, insert one
+ elif state == InsertRLSState.FOUND_TABLE and token.ttype != Whitespace:
+ i = token_list.tokens.index(token)
+
+ # Left pad with space, if needed
+ if i > 0 and token_list.tokens[i - 1].ttype != Whitespace:
+ token_list.tokens.insert(i, Token(Whitespace, " "))
+ i += 1
+
+ # Insert predicate
+ token_list.tokens.insert(
+ i, Where([Token(Keyword, "WHERE"), Token(Whitespace, " "), rls]),
+ )
+
+ # Right pad with space, if needed
Review comment:
why does sqlparse even tokenize whitespace?
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r823862353
##########
File path: tests/unit_tests/sql_parse_tests.py
##########
@@ -1189,3 +1193,169 @@ def test_sqlparse_issue_652():
stmt = sqlparse.parse(r"foo = '\' AND bar = 'baz'")[0]
assert len(stmt.tokens) == 5
assert str(stmt.tokens[0]) == "foo = '\\'"
+
+
+@pytest.mark.parametrize(
+ "sql,expected",
+ [
+ ("SELECT * FROM table", True),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN (SELECT * FROM table)", True),
+ ("(SELECT COUNT(DISTINCT name) AS foo FROM birth_names)", True),
+ ("COUNT(*)", False),
+ ("SELECT a FROM (SELECT 1 AS a)", False),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN table", True),
+ ("SELECT * FROM (SELECT 1 AS foo, 2 AS bar) ORDER BY foo ASC, bar", False),
+ ("SELECT * FROM other_table", True),
+ ],
+)
+def test_has_table_query(sql: str, expected: bool) -> None:
+ """
+ Test if a given statement queries a table.
+
+ This is used to prevent ad-hoc metrics from querying unauthorized tables, bypassing
+ row-level security.
+ """
+ statement = sqlparse.parse(sql)[0]
+ assert has_table_query(statement) == expected
+
+
+@pytest.mark.parametrize(
+ "sql,table,rls,expected",
+ [
+ # append RLS to an existing WHERE clause
+ (
+ "SELECT * FROM other_table WHERE 1=1",
+ "other_table",
+ "id=42",
+ "SELECT * FROM other_table WHERE 1=1 AND other_table.id=42",
+ ),
+ # "table" is a reserved word; since sqlparse is too aggressive when characterizing
+ # reserved words we need to support them even when not quoted
Review comment:
Yeah, we had a few problems with SQL Lab because of that. The list of "keywords" in `sqlparse` is not dialect-dependent, so identifiers get misclassified often.
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822128167
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
Review comment:
@john-bodley I reimplemented it following the same logic as `insert_rls` (recursive tree traversal instead of flattening).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e816857) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.58% +0.02%
==========================================
Files 1641 1641
Lines 63495 63569 +74
Branches 6425 6425
==========================================
+ Hits 42265 42327 +62
- Misses 19550 19562 +12
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.94%> (-0.09%)` | :arrow_down: |
| mysql | `81.86% <100.00%> (+0.05%)` | :arrow_up: |
| postgres | `81.90% <100.00%> (+0.03%)` | :arrow_up: |
| presto | `52.35% <11.94%> (-0.09%)` | :arrow_down: |
| python | `82.29% <100.00%> (+<0.01%)` | :arrow_up: |
| sqlite | `?` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.93% <100.00%> (+0.32%)` | :arrow_up: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `91.89% <0.00%> (-5.41%)` | :arrow_down: |
| [superset/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3V0aWxzLnB5) | `88.23% <0.00%> (-3.93%)` | :arrow_down: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `86.20% <0.00%> (-3.45%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/result\_set.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVzdWx0X3NldC5weQ==) | `96.77% <0.00%> (-1.62%)` | :arrow_down: |
| [superset/databases/commands/test\_connection.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3Rlc3RfY29ubmVjdGlvbi5weQ==) | `98.57% <0.00%> (-1.43%)` | :arrow_down: |
| [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `73.58% <0.00%> (-0.95%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `95.13% <0.00%> (-0.55%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...e816857](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] villebro commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r824720026
##########
File path: tests/unit_tests/sql_parse_tests.py
##########
@@ -1189,3 +1193,225 @@ def test_sqlparse_issue_652():
stmt = sqlparse.parse(r"foo = '\' AND bar = 'baz'")[0]
assert len(stmt.tokens) == 5
assert str(stmt.tokens[0]) == "foo = '\\'"
+
+
+@pytest.mark.parametrize(
+ "sql,expected",
+ [
+ ("SELECT * FROM table", True),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN (SELECT * FROM table)", True),
+ ("(SELECT COUNT(DISTINCT name) AS foo FROM birth_names)", True),
+ ("COUNT(*)", False),
+ ("SELECT a FROM (SELECT 1 AS a)", False),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN table", True),
+ ("SELECT * FROM (SELECT 1 AS foo, 2 AS bar) ORDER BY foo ASC, bar", False),
+ ("SELECT * FROM other_table", True),
+ ],
+)
Review comment:
Could we add a test case for that `EXTRACT` expression above: `("extract(HOUR from from_unixtime(hour_ts)", False)` which caued a downgrade of `sqlparse` a while back: #10165. I tested that it fails currently - not sure if that's a problem or if we can live with it?
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,199 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Returns if the token represents a reference to the table.
+
+ Tables can be fully qualified with periods.
+
+ Note that in theory a table should be represented as an identifier, but due to
+ sqlparse's aggressive list of keywords (spanning multiple dialects) often it gets
+ classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
+ if left != right:
+ return False
+
+ return True
+
+
+def insert_rls(token_list: TokenList, table: str, rls: TokenList) -> TokenList:
+ """
+ Update a statement inpalce applying an RLS associated with a given table.
Review comment:
typo
```suggestion
Update a statement inplace applying an RLS associated with a given table.
```
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
+
+ A table should be represented as an identifier, but due to sqlparse's aggressive list
+ of keywords (spanning multiple dialects) often it gets classified as a keyword.
+ """
+ candidate = token.value
+
+ # match from right to left, splitting on the period, eg, schema.table == table
+ for left, right in zip(candidate.split(".")[::-1], table.split(".")[::-1]):
Review comment:
edge case: WIth more and more dbs nowadays referencing files as tables that often include periods, I wonder if we need to consider quoted entities, like `schema."table.parquet"`, in which case the right most element would be `table.parquet`, not `parquet"`
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (00598fa) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **decrease** coverage by `0.10%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
- Coverage 66.56% 66.45% -0.11%
==========================================
Files 1641 1641
Lines 63495 63583 +88
Branches 6425 6425
==========================================
- Hits 42265 42257 -8
- Misses 19550 19646 +96
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.53% <11.26%> (-0.07%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.38% <11.26%> (-0.07%)` | :arrow_down: |
| python | `82.03% <100.00%> (-0.26%)` | :arrow_down: |
| sqlite | `81.67% <100.00%> (+0.12%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.95% <100.00%> (+0.34%)` | :arrow_up: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `64.70% <0.00%> (-23.53%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `60.34% <0.00%> (-20.69%)` | :arrow_down: |
| [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
| [superset/common/utils/dataframe\_utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3V0aWxzL2RhdGFmcmFtZV91dGlscy5weQ==) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [superset/databases/api.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `87.98% <0.00%> (-6.01%)` | :arrow_down: |
| [superset/views/database/validators.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvdmFsaWRhdG9ycy5weQ==) | `78.94% <0.00%> (-5.27%)` | :arrow_down: |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `94.29% <0.00%> (-4.18%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `93.97% <0.00%> (-3.62%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...00598fa](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e816857) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.60% +0.03%
==========================================
Files 1641 1641
Lines 63495 63569 +74
Branches 6425 6425
==========================================
+ Hits 42265 42339 +74
Misses 19550 19550
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.94%> (-0.09%)` | :arrow_down: |
| mysql | `81.86% <100.00%> (+0.05%)` | :arrow_up: |
| postgres | `81.90% <100.00%> (+0.03%)` | :arrow_up: |
| presto | `52.35% <11.94%> (-0.09%)` | :arrow_down: |
| python | `82.33% <100.00%> (+0.04%)` | :arrow_up: |
| sqlite | `81.66% <100.00%> (+0.10%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.93% <100.00%> (+0.32%)` | :arrow_up: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `95.13% <0.00%> (-0.55%)` | :arrow_down: |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.14% <0.00%> (-0.37%)` | :arrow_down: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/databases/schemas.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YWJhc2VzL3NjaGVtYXMucHk=) | `98.47% <0.00%> (+<0.01%)` | :arrow_up: |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
| [superset/commands/exceptions.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvZXhjZXB0aW9ucy5weQ==) | `92.98% <0.00%> (+0.25%)` | :arrow_up: |
| [superset/common/query\_context\_processor.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X2NvbnRleHRfcHJvY2Vzc29yLnB5) | `91.46% <0.00%> (+0.47%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...e816857](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6942b9e) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
=======================================
Coverage 66.56% 66.57%
=======================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
=======================================
+ Hits 42265 42311 +46
- Misses 19550 19567 +17
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `?` | |
| postgres | `81.88% <100.00%> (+0.02%)` | :arrow_up: |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.27% <100.00%> (-0.02%)` | :arrow_down: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/common/utils/dataframe\_utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3V0aWxzL2RhdGFmcmFtZV91dGlscy5weQ==) | `85.71% <0.00%> (-7.15%)` | :arrow_down: |
| [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `93.97% <0.00%> (-3.62%)` | :arrow_down: |
| [superset/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9sb2dfcHJ1bmUucHk=) | `85.71% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbWFuZHMvaW1wb3J0ZXJzL3YxL3V0aWxzLnB5) | `89.13% <0.00%> (-2.18%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `88.19% <0.00%> (-0.73%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `95.13% <0.00%> (-0.55%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `77.03% <0.00%> (-0.45%)` | :arrow_down: |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.14% <0.00%> (-0.37%)` | :arrow_down: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| ... and [1 more](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...6942b9e](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] codecov[bot] edited a comment on pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19055:
URL: https://github.com/apache/superset/pull/19055#issuecomment-1061353101
# [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19055](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6942b9e) into [master](https://codecov.io/gh/apache/superset/commit/77063cc814edc9c227efdfc2ff34710f9177aa63?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77063cc) will **increase** coverage by `0.03%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #19055 +/- ##
==========================================
+ Coverage 66.56% 66.59% +0.03%
==========================================
Files 1641 1641
Lines 63495 63558 +63
Branches 6425 6425
==========================================
+ Hits 42265 42328 +63
Misses 19550 19550
Partials 1680 1680
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.51% <11.47%> (-0.09%)` | :arrow_down: |
| mysql | `81.85% <100.00%> (+0.03%)` | :arrow_up: |
| postgres | `81.89% <100.00%> (+0.03%)` | :arrow_up: |
| presto | `52.35% <11.47%> (-0.09%)` | :arrow_down: |
| python | `82.33% <100.00%> (+0.03%)` | :arrow_up: |
| sqlite | `81.59% <100.00%> (+0.03%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `98.91% <100.00%> (+0.30%)` | :arrow_up: |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.82% <0.00%> (ø)` | |
| [superset/reports/commands/execute.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGVjdXRlLnB5) | `91.51% <0.00%> (ø)` | |
| [superset/utils/core.py](https://codecov.io/gh/apache/superset/pull/19055/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | `90.25% <0.00%> (+0.02%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [77063cc...6942b9e](https://codecov.io/gh/apache/superset/pull/19055?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [superset] betodealmeida commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822119884
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +460,178 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+def has_table_query(statement: Statement) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ seen_source = False
+ tokens = statement.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+ if isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+ if token.ttype == Keyword and token.value.lower() in ("from", "join"):
+ seen_source = True
+ elif seen_source and (
Review comment:
Yeah, in the `insert_rls` function I had to implement tree traversal to get it right. Let me give it a try rewriting this one.
--
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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822908285
##########
File path: superset/sql_parse.py
##########
@@ -458,3 +459,183 @@ def validate_filter_clause(clause: str) -> None:
)
if open_parens > 0:
raise QueryClauseValidationException("Unclosed parenthesis in filter clause")
+
+
+class InsertRLSState(str, Enum):
+ """
+ State machine that scans for WHERE and ON clauses referencing tables.
+ """
+
+ SCANNING = "SCANNING"
+ SEEN_SOURCE = "SEEN_SOURCE"
+ FOUND_TABLE = "FOUND_TABLE"
+
+
+def has_table_query(token_list: TokenList) -> bool:
+ """
+ Return if a stament has a query reading from a table.
+
+ >>> has_table_query(sqlparse.parse("COUNT(*)")[0])
+ False
+ >>> has_table_query(sqlparse.parse("SELECT * FROM table")[0])
+ True
+
+ Note that queries reading from constant values return false:
+
+ >>> has_table_query(sqlparse.parse("SELECT * FROM (SELECT 1)")[0])
+ False
+
+ """
+ state = InsertRLSState.SCANNING
+ for token in token_list.tokens:
+
+ # # Recurse into child token list
+ if isinstance(token, TokenList) and has_table_query(token):
+ return True
+
+ # Found a source keyword (FROM/JOIN)
+ if imt(token, m=[(Keyword, "FROM"), (Keyword, "JOIN")]):
+ state = InsertRLSState.SEEN_SOURCE
+
+ # Found identifier/keyword after FROM/JOIN
+ elif state == InsertRLSState.SEEN_SOURCE and (
+ isinstance(token, sqlparse.sql.Identifier) or token.ttype == Keyword
+ ):
+ return True
+
+ # Found nothing, leaving source
+ elif state == InsertRLSState.SEEN_SOURCE and token.ttype != Whitespace:
+ state = InsertRLSState.SCANNING
+
+ return False
+
+
+def add_table_name(rls: TokenList, table: str) -> None:
+ """
+ Modify a RLS expression ensuring columns are fully qualified.
+ """
+ tokens = rls.tokens[:]
+ while tokens:
+ token = tokens.pop(0)
+
+ if isinstance(token, Identifier) and token.get_parent_name() is None:
+ token.tokens = [
+ Token(Name, table),
+ Token(Punctuation, "."),
+ Token(Name, token.get_name()),
+ ]
+ elif isinstance(token, TokenList):
+ tokens.extend(token.tokens)
+
+
+def matches_table_name(token: Token, table: str) -> bool:
+ """
+ Return the name of a table.
Review comment:
docstring is inaccurate
```suggestion
Returns true if the token represents a reference to the table (tables can be fully qualified with .'s)
```
--
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
[GitHub] [superset] suddjian commented on a change in pull request #19055: feat: helper functions for RLS
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19055:
URL: https://github.com/apache/superset/pull/19055#discussion_r822964917
##########
File path: tests/unit_tests/sql_parse_tests.py
##########
@@ -1189,3 +1193,169 @@ def test_sqlparse_issue_652():
stmt = sqlparse.parse(r"foo = '\' AND bar = 'baz'")[0]
assert len(stmt.tokens) == 5
assert str(stmt.tokens[0]) == "foo = '\\'"
+
+
+@pytest.mark.parametrize(
+ "sql,expected",
+ [
+ ("SELECT * FROM table", True),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN (SELECT * FROM table)", True),
+ ("(SELECT COUNT(DISTINCT name) AS foo FROM birth_names)", True),
+ ("COUNT(*)", False),
+ ("SELECT a FROM (SELECT 1 AS a)", False),
+ ("SELECT a FROM (SELECT 1 AS a) JOIN table", True),
+ ("SELECT * FROM (SELECT 1 AS foo, 2 AS bar) ORDER BY foo ASC, bar", False),
+ ("SELECT * FROM other_table", True),
+ ],
+)
+def test_has_table_query(sql: str, expected: bool) -> None:
+ """
+ Test if a given statement queries a table.
+
+ This is used to prevent ad-hoc metrics from querying unauthorized tables, bypassing
+ row-level security.
+ """
+ statement = sqlparse.parse(sql)[0]
+ assert has_table_query(statement) == expected
+
+
+@pytest.mark.parametrize(
+ "sql,table,rls,expected",
+ [
+ # append RLS to an existing WHERE clause
+ (
+ "SELECT * FROM other_table WHERE 1=1",
+ "other_table",
+ "id=42",
+ "SELECT * FROM other_table WHERE 1=1 AND other_table.id=42",
+ ),
+ # "table" is a reserved word; since sqlparse is too aggressive when characterizing
+ # reserved words we need to support them even when not quoted
Review comment:
good catch, damn
--
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