[sqlparse] fix sqlparse bug #5703

timifasubaa · 2018-08-22T21:47:38Z

superset's sql_parse implementation fails when there are more than two tables in a join.
This causes problems when we use this method to extract tables to determine if a user can access all the tables in a sqllab query.

This PR addresses the issue by preventing it from stopping early. I also added a test.

@john-bodley @mistercrunch

codecov-io · 2018-08-22T23:07:12Z

Codecov Report

Merging #5703 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5703      +/-   ##
==========================================
+ Coverage   63.45%   63.45%   +<.01%     
==========================================
  Files         361      361              
  Lines       22972    22973       +1     
  Branches     2557     2557              
==========================================
+ Hits        14576    14577       +1     
  Misses       8381     8381              
  Partials       15       15

Impacted Files	Coverage Δ
superset/sql_parse.py	`99.17% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8992755...fcc1c17. Read the comment docs.

john-bodley

Should this logic belong in a function other than __extract_from_token?

john-bodley · 2018-08-22T23:21:15Z

superset/sql_parse.py

@@ -128,7 +129,8 @@ def __extract_from_token(self, token):
                continue

            if item.ttype in Keyword:
-                if self.__is_result_operation(item.value):
+                if (self.__is_result_operation(item.value) or
+                        item.value.upper() == ON_KEYWORD):


It seems like line #134 is (and was) obsolete and that this check simply ensures one doesn't break. Would it make more sense if one simply removed the break logic? According to line #136 (# FROM clause is over) one would speculate that this portion of the code simply processes a FROM clause rather than a JOIN clause.

I tested using continue instead of the break statement and it broke in some cases because it included the group by column in the table names.

mistercrunch · 2018-08-23T01:06:36Z

tests/sql_parse_tests.py

@@ -310,3 +310,23 @@ def test_explain(self):
        self.assertEquals(True, sql.is_explain())
        self.assertEquals(False, sql.is_select())
        self.assertEquals(True, sql.is_readonly())
+
+    def test_complex_extract_tables(self):


can we add another test that does not use ON? Something like

SELECT * FROM table_a AS a, table_b AS b WHERE a.id = b.id

john-bodley · 2018-08-23T17:36:40Z

@timifasubaa pending the flake8 fix, this LGTM.

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

[sqlparse] fix sqlparse bug (apache#5703)

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

* fix sqlparse bug * add one more test case

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

* fix sqlparse bug * add one more test case

timifasubaa changed the title ~~[WIP] fix sqlparse bug~~ [sqlparse] fix sqlparse bug Aug 22, 2018

timifasubaa force-pushed the fix_sqlparse_bug branch 5 times, most recently from a5690f9 to aaf5899 Compare August 22, 2018 22:34

fix sqlparse bug

eecb864

timifasubaa force-pushed the fix_sqlparse_bug branch from aaf5899 to eecb864 Compare August 22, 2018 22:44

john-bodley reviewed Aug 22, 2018

View reviewed changes

mistercrunch reviewed Aug 23, 2018

View reviewed changes

timifasubaa force-pushed the fix_sqlparse_bug branch from 139a647 to 093a81e Compare August 23, 2018 01:16

john-bodley approved these changes Aug 23, 2018

View reviewed changes

add one more test case

fcc1c17

timifasubaa force-pushed the fix_sqlparse_bug branch from 093a81e to fcc1c17 Compare August 23, 2018 17:42

timifasubaa merged commit 5c49514 into apache:master Aug 23, 2018

john-bodley pushed a commit to john-bodley/superset that referenced this pull request Aug 23, 2018

[sqlparse] fix sqlparse bug (apache#5703)

04d5676

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Aug 23, 2018

Merge pull request #98 from john-bodley/john-bodley-cherry-pick-5703

a40656a

[sqlparse] fix sqlparse bug (apache#5703)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

ac85043

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

032b5d7

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

3f707ad

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

5614e8a

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

0872025

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

537b29c

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

3092081

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

0a75e7e

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

0ecea92

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

c1c2d65

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Sep 21, 2018

[sqlparse] fix sqlparse bug (apache#5703)

1a374a5

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 11, 2018

[sqlparse] fix sqlparse bug (apache#5703)

91157f8

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 11, 2018

[sqlparse] fix sqlparse bug (apache#5703)

f742fc0

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 11, 2018

[sqlparse] fix sqlparse bug (apache#5703)

9d468c2

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018

[sqlparse] fix sqlparse bug (apache#5703)

7ce912a

* fix sqlparse bug * add one more test case

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018

[sqlparse] fix sqlparse bug (apache#5703)

d398d43

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018

[sqlparse] fix sqlparse bug (apache#5703)

d984411

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018

[sqlparse] fix sqlparse bug (apache#5703)

dcc39ba

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018

[sqlparse] fix sqlparse bug (apache#5703)

4c0d0d9

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018

[sqlparse] fix sqlparse bug (apache#5703)

ffd84aa

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018

[sqlparse] fix sqlparse bug (apache#5703)

4ab7bfa

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Oct 17, 2018

[sqlparse] fix sqlparse bug (apache#5703)

bf629fd

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

mistercrunch pushed a commit to lyft/incubator-superset that referenced this pull request Oct 29, 2018

[sqlparse] fix sqlparse bug (apache#5703)

3e928dd

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018

[sqlparse] fix sqlparse bug (apache#5703)

bbfcaea

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018

[sqlparse] fix sqlparse bug (apache#5703)

4ae9005

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018

[sqlparse] fix sqlparse bug (apache#5703)

3414076

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018

[sqlparse] fix sqlparse bug (apache#5703)

7a05a80

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018

[sqlparse] fix sqlparse bug (apache#5703)

7eca7e4

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Nov 2, 2018

[sqlparse] fix sqlparse bug (apache#5703)

c23dd98

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

youngyjd pushed a commit to lyft/incubator-superset that referenced this pull request Nov 2, 2018

[sqlparse] fix sqlparse bug (apache#5703)

f8bc09e

* fix sqlparse bug * add one more test case (cherry picked from commit 5c49514)

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

[sqlparse] fix sqlparse bug (apache#5703)

666d2f9

* fix sqlparse bug * add one more test case

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sqlparse] fix sqlparse bug #5703

[sqlparse] fix sqlparse bug #5703

timifasubaa commented Aug 22, 2018 •

edited

Loading

codecov-io commented Aug 22, 2018 •

edited

Loading

john-bodley left a comment

john-bodley Aug 22, 2018 •

edited

Loading

timifasubaa Aug 23, 2018

mistercrunch Aug 23, 2018

timifasubaa Aug 23, 2018

john-bodley commented Aug 23, 2018 •

edited

Loading

[sqlparse] fix sqlparse bug #5703

[sqlparse] fix sqlparse bug #5703

Conversation

timifasubaa commented Aug 22, 2018 • edited Loading

codecov-io commented Aug 22, 2018 • edited Loading

Codecov Report

john-bodley left a comment

Choose a reason for hiding this comment

john-bodley Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

timifasubaa Aug 23, 2018

Choose a reason for hiding this comment

mistercrunch Aug 23, 2018

Choose a reason for hiding this comment

timifasubaa Aug 23, 2018

Choose a reason for hiding this comment

john-bodley commented Aug 23, 2018 • edited Loading

timifasubaa commented Aug 22, 2018 •

edited

Loading

codecov-io commented Aug 22, 2018 •

edited

Loading

john-bodley Aug 22, 2018 •

edited

Loading

john-bodley commented Aug 23, 2018 •

edited

Loading