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/31 05:02:10 UTC

[GitHub] [superset] betodealmeida opened a new pull request #19454: feat: improve adhoc SQL validation

betodealmeida opened a new pull request #19454:
URL: https://github.com/apache/superset/pull/19454


   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   Improve the `validate_adhoc_subquery` function.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   N/A
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   Updated unit tests, will add more.
   
   ### 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] betodealmeida merged pull request #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #19454:
URL: https://github.com/apache/superset/pull/19454


   


-- 
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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19454:
URL: https://github.com/apache/superset/pull/19454#issuecomment-1084946384


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19454?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 [#19454](https://codecov.io/gh/apache/superset/pull/19454?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b83a64f) into [master](https://codecov.io/gh/apache/superset/commit/8e29ec5a6685867ffc035d20999c54c2abe36fb1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e29ec5) will **increase** coverage by `0.00%`.
   > The diff coverage is `93.02%`.
   
   ```diff
   @@           Coverage Diff           @@
   ##           master   #19454   +/-   ##
   =======================================
     Coverage   66.57%   66.58%           
   =======================================
     Files        1675     1675           
     Lines       64092    64107   +15     
     Branches     6519     6519           
   =======================================
   + Hits        42672    42684   +12     
   - Misses      19729    19732    +3     
     Partials     1691     1691           
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `52.69% <44.18%> (-0.01%)` | :arrow_down: |
   | mysql | `81.90% <93.02%> (-0.01%)` | :arrow_down: |
   | postgres | `81.95% <93.02%> (-0.01%)` | :arrow_down: |
   | presto | `52.54% <44.18%> (-0.01%)` | :arrow_down: |
   | python | `82.39% <93.02%> (-0.01%)` | :arrow_down: |
   | sqlite | `81.72% <93.02%> (-0.01%)` | :arrow_down: |
   
   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/19454?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/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `94.50% <88.88%> (+0.12%)` | :arrow_up: |
   | [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `97.38% <92.30%> (-0.92%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/19454/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.32% <100.00%> (+<0.01%)` | :arrow_up: |
   | [superset/db\_engine\_specs/drill.py](https://codecov.io/gh/apache/superset/pull/19454/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2RyaWxsLnB5) | `87.17% <0.00%> (+0.33%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19454?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/19454?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 [8e29ec5...b83a64f](https://codecov.io/gh/apache/superset/pull/19454?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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #19454:
URL: https://github.com/apache/superset/pull/19454#discussion_r839292500



##########
File path: superset/sql_parse.py
##########
@@ -539,45 +540,67 @@ def add_table_name(rls: TokenList, table: str) -> None:
             tokens.extend(token.tokens)
 
 
-def matches_table_name(candidate: Token, table: str) -> bool:
+def get_rls_for_table(
+    candidate: Token,
+    database_id: int,
+    default_schema: Optional[str],
+) -> Optional[TokenList]:
     """
-    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.
+    Given a table name, return any associated RLS predicates.
     """
+    # pylint: disable=import-outside-toplevel
+    from superset import db
+    from superset.connectors.sqla.models import SqlaTable
+
     if not isinstance(candidate, Identifier):
         candidate = Identifier([Token(Name, candidate.value)])
 
-    target = sqlparse.parse(table)[0].tokens[0]
-    if not isinstance(target, Identifier):
-        target = Identifier([Token(Name, target.value)])
+    table = ParsedQuery._get_table(candidate)  # pylint: disable=protected-access
+    if not table:
+        return None
 
-    # match from right to left, splitting on the period, eg, schema.table == table
-    for left, right in zip(candidate.tokens[::-1], target.tokens[::-1]):
-        if left.value != right.value:
-            return False
+    dataset = (
+        db.session.query(SqlaTable)
+        .filter(
+            and_(
+                SqlaTable.database_id == database_id,
+                SqlaTable.schema == (table.schema or default_schema),
+                SqlaTable.table_name == table.table,
+            )
+        )
+        .one_or_none()
+    )
+    if not dataset:
+        return None
 
-    return True
+    template_processor = dataset.get_template_processor()
+    # pylint: disable=protected-access
+    predicate = " AND ".join(
+        str(filter_)
+        for filter_ in dataset._get_sqla_row_level_filters(template_processor)
+    )
+    rls = sqlparse.parse(predicate)[0]
+    add_table_name(rls, str(dataset))
 
+    return rls

Review comment:
       I hit a problem here where a user didn't have Roles that would have applied RLS filters to a dataset, despite there being filters for some other roles. This caused `dataset._get_sqla_row_level_filters(template_processor)` to return an empty `List`. So I'd add the following early return here if the predicate is falsy:
   ```suggestion
       if not predicate:
           return None
           
       rls = sqlparse.parse(predicate)[0]
       add_table_name(rls, str(dataset))
   
       return rls
   ```

##########
File path: tests/unit_tests/sql_parse_tests.py
##########
@@ -1391,13 +1392,37 @@ def test_has_table_query(sql: str, expected: bool) -> None:
         ),
     ],
 )
-def test_insert_rls(sql: str, table: str, rls: str, expected: str) -> None:
+def test_insert_rls(
+    mocker: MockerFixture, sql: str, table: str, rls: str, expected: str
+) -> None:
     """
     Insert into a statement a given RLS condition associated with a table.
     """
-    statement = sqlparse.parse(sql)[0]
     condition = sqlparse.parse(rls)[0]
-    assert str(insert_rls(statement, table, condition)).strip() == expected.strip()
+    add_table_name(condition, table)
+
+    # pylint: disable=unused-argument
+    def get_rls_for_table(
+        candidate: Token, database_id: int, default_schema: str
+    ) -> TokenList:

Review comment:
       nit:
   ```suggestion
       ) -> Optional[TokenList]:
   ```




-- 
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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #19454:
URL: https://github.com/apache/superset/pull/19454#issuecomment-1084946384


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19454?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 [#19454](https://codecov.io/gh/apache/superset/pull/19454?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b83a64f) into [master](https://codecov.io/gh/apache/superset/commit/8e29ec5a6685867ffc035d20999c54c2abe36fb1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e29ec5) will **decrease** coverage by `0.02%`.
   > The diff coverage is `93.02%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #19454      +/-   ##
   ==========================================
   - Coverage   66.57%   66.55%   -0.03%     
   ==========================================
     Files        1675     1675              
     Lines       64092    64107      +15     
     Branches     6519     6519              
   ==========================================
   - Hits        42672    42665       -7     
   - Misses      19729    19751      +22     
     Partials     1691     1691              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `52.69% <44.18%> (-0.01%)` | :arrow_down: |
   | mysql | `81.90% <93.02%> (-0.01%)` | :arrow_down: |
   | postgres | `81.94% <93.02%> (-0.02%)` | :arrow_down: |
   | presto | `52.54% <44.18%> (-0.01%)` | :arrow_down: |
   | python | `82.33% <93.02%> (-0.07%)` | :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/19454?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/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `89.01% <88.88%> (-5.38%)` | :arrow_down: |
   | [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `97.38% <92.30%> (-0.92%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/19454/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.32% <100.00%> (+<0.01%)` | :arrow_up: |
   | [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/19454/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/reports/commands/log\_prune.py](https://codecov.io/gh/apache/superset/pull/19454/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/19454/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/result\_set.py](https://codecov.io/gh/apache/superset/pull/19454/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/19454/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/commands/importers/v1/utils.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `92.20% <0.00%> (-1.30%)` | :arrow_down: |
   | ... and [7 more](https://codecov.io/gh/apache/superset/pull/19454/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/19454?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/19454?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 [8e29ec5...b83a64f](https://codecov.io/gh/apache/superset/pull/19454?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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #19454:
URL: https://github.com/apache/superset/pull/19454#issuecomment-1084946384


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19454?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 [#19454](https://codecov.io/gh/apache/superset/pull/19454?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b83a64f) into [master](https://codecov.io/gh/apache/superset/commit/8e29ec5a6685867ffc035d20999c54c2abe36fb1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e29ec5) will **decrease** coverage by `0.02%`.
   > The diff coverage is `93.02%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #19454      +/-   ##
   ==========================================
   - Coverage   66.57%   66.55%   -0.03%     
   ==========================================
     Files        1675     1675              
     Lines       64092    64107      +15     
     Branches     6519     6519              
   ==========================================
   - Hits        42672    42669       -3     
   - Misses      19729    19747      +18     
     Partials     1691     1691              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `52.69% <44.18%> (-0.01%)` | :arrow_down: |
   | mysql | `81.90% <93.02%> (-0.01%)` | :arrow_down: |
   | postgres | `81.95% <93.02%> (-0.01%)` | :arrow_down: |
   | presto | `52.54% <44.18%> (-0.01%)` | :arrow_down: |
   | python | `82.34% <93.02%> (-0.05%)` | :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/19454?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/connectors/sqla/utils.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `89.01% <88.88%> (-5.38%)` | :arrow_down: |
   | [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/19454/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) | `97.38% <92.30%> (-0.92%)` | :arrow_down: |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/19454/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `89.32% <100.00%> (+<0.01%)` | :arrow_up: |
   | [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/superset/pull/19454/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/utils/celery.py](https://codecov.io/gh/apache/superset/pull/19454/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/result\_set.py](https://codecov.io/gh/apache/superset/pull/19454/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/19454/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/19454/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/models/slice.py](https://codecov.io/gh/apache/superset/pull/19454/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-c3VwZXJzZXQvbW9kZWxzL3NsaWNlLnB5) | `84.95% <0.00%> (-0.49%)` | :arrow_down: |
   | ... and [3 more](https://codecov.io/gh/apache/superset/pull/19454/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/19454?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/19454?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 [8e29ec5...b83a64f](https://codecov.io/gh/apache/superset/pull/19454?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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19454:
URL: https://github.com/apache/superset/pull/19454#discussion_r839169850



##########
File path: superset/connectors/sqla/models.py
##########
@@ -1420,7 +1441,6 @@ def get_sqla_query(  # pylint: disable=too-many-arguments,too-many-locals,too-ma
                 and db_engine_spec.allows_hidden_cc_in_orderby
                 and col.name in [select_col.name for select_col in select_exprs]
             ):
-                validate_adhoc_subquery(str(col.expression))

Review comment:
       Moved this [up](https://github.com/apache/superset/pull/19454/files#diff-af5e15a55ff9c81f441eb6b9b10dae13b2ee17fec614d3e3ad0057ed0dc814aeR1155-R1160), where it's still a string.




-- 
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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19454:
URL: https://github.com/apache/superset/pull/19454#discussion_r839169616



##########
File path: superset/connectors/sqla/models.py
##########
@@ -984,15 +992,14 @@ def is_alias_used_in_orderby(col: ColumnElement) -> bool:
 
     def _get_sqla_row_level_filters(
         self, template_processor: BaseTemplateProcessor
-    ) -> List[str]:
+    ) -> List[TextClause]:

Review comment:
       Not sure why `mypy` didn't catch 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 #19454: feat: improve adhoc SQL validation

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #19454:
URL: https://github.com/apache/superset/pull/19454#discussion_r839825075



##########
File path: superset/sql_parse.py
##########
@@ -539,45 +540,67 @@ def add_table_name(rls: TokenList, table: str) -> None:
             tokens.extend(token.tokens)
 
 
-def matches_table_name(candidate: Token, table: str) -> bool:
+def get_rls_for_table(
+    candidate: Token,
+    database_id: int,
+    default_schema: Optional[str],
+) -> Optional[TokenList]:
     """
-    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.
+    Given a table name, return any associated RLS predicates.
     """
+    # pylint: disable=import-outside-toplevel
+    from superset import db
+    from superset.connectors.sqla.models import SqlaTable
+
     if not isinstance(candidate, Identifier):
         candidate = Identifier([Token(Name, candidate.value)])
 
-    target = sqlparse.parse(table)[0].tokens[0]
-    if not isinstance(target, Identifier):
-        target = Identifier([Token(Name, target.value)])
+    table = ParsedQuery._get_table(candidate)  # pylint: disable=protected-access
+    if not table:
+        return None
 
-    # match from right to left, splitting on the period, eg, schema.table == table
-    for left, right in zip(candidate.tokens[::-1], target.tokens[::-1]):
-        if left.value != right.value:
-            return False
+    dataset = (
+        db.session.query(SqlaTable)
+        .filter(
+            and_(
+                SqlaTable.database_id == database_id,
+                SqlaTable.schema == (table.schema or default_schema),
+                SqlaTable.table_name == table.table,
+            )
+        )
+        .one_or_none()
+    )
+    if not dataset:
+        return None
 
-    return True
+    template_processor = dataset.get_template_processor()
+    # pylint: disable=protected-access
+    predicate = " AND ".join(
+        str(filter_)
+        for filter_ in dataset._get_sqla_row_level_filters(template_processor)
+    )
+    rls = sqlparse.parse(predicate)[0]
+    add_table_name(rls, str(dataset))
 
+    return rls

Review comment:
       Ah, good point! Fixed 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