[fix] Adding check for invalid filter columns #7888

john-bodley · 2019-07-17T22:34:26Z

SUMMARY

This PR fixes an issue where if you specified an invalid column (achievable in the URL) in a simple filter, i.e., not custom SQL, then the filter was simply ignored and the the query would execute sans the filter with no clear indication to the user that the filter wasn't being applied.

This PR applies the same logic as invalid metrics and raises an exception if the column is invalid. Note I added unit tests to cover both the invalid metric and column use cases.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

to: @michellethomas @mistercrunch @williaster

john-bodley · 2019-07-17T22:35:54Z

superset/connectors/sqla/models.py

-                    elif op == "IS NOT NULL":
-                        where_clause_and.append(col_obj.get_sqla_col() != None)  # noqa
+            col_obj = cols[col]
+            is_list_target = op in ("in", "not in")


Note this block is just dedented as the if col_obj check is not required given the addition of the new check.

williaster

Thanks for looking at this and adding tests, I think this is a pretty important check for "data correctness" 💯

Had a couple small suggestions re the error messages.

williaster · 2019-07-17T22:39:01Z

superset/connectors/sqla/models.py

@@ -721,43 +721,46 @@ def get_sqla_query(  # sqla
            if not all([flt.get(s) for s in ["col", "op"]]):
                continue
            col = flt["col"]
+
+            if col not in cols:
+                raise Exception(_(f"Column '{col}' is not valid"))


maybe we could update the language to be a little more precise here? For example Column '{col}' does not exist / ... not found?

@williaster I think the wording here can be difficult as it's in reference to Superset's data model as opposed to the schema of the underlying table. I think "does not exist" is more precise/clearer than "is not valid" though.

williaster · 2019-07-17T22:40:59Z

superset/connectors/sqla/models.py

@@ -649,7 +649,7 @@ def get_sqla_query(  # sqla
            elif m in metrics_dict:
                metrics_exprs.append(metrics_dict.get(m).get_sqla_col())
            else:
-                raise Exception(_("Metric '{}' is not valid".format(m)))
+                raise Exception(_(f"Metric '{m}' is not valid"))


Ditto re the error message here? Metric '{m}' does not exist / ... not found?

codecov-io · 2019-07-17T22:47:06Z

Codecov Report

Merging #7888 into master will increase coverage by <.01%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##           master    #7888      +/-   ##
==========================================
+ Coverage   65.78%   65.79%   +<.01%     
==========================================
  Files         461      461              
  Lines       22187    22188       +1     
  Branches     2425     2425              
==========================================
+ Hits        14596    14598       +2     
+ Misses       7470     7469       -1     
  Partials      121      121

Impacted Files	Coverage Δ
superset/connectors/sqla/models.py	`82.67% <36.36%> (+0.2%)`	⬆️

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 174a48a...06d26bd. Read the comment docs.

john-bodley · 2019-07-17T23:12:42Z

@williaster I addressed your comments.

mistercrunch · 2019-07-18T05:11:19Z

superset/connectors/sqla/models.py

@@ -649,7 +649,7 @@ def get_sqla_query(  # sqla
            elif m in metrics_dict:
                metrics_exprs.append(metrics_dict.get(m).get_sqla_col())
            else:
-                raise Exception(_("Metric '{}' is not valid".format(m)))
+                raise Exception(_(f"Metric '{m}' does not exist"))


It was wrong before, but variable formatting with get_text should use a different formatting scheme. Should look something like _("Metric '%(metric)s' does not exist", metric=metric)
https://pythonhosted.org/Flask-Babel/#flask.ext.babel.gettext

mistercrunch · 2019-07-18T05:13:00Z

superset/connectors/sqla/models.py

-            raise Exception(
-                _("Metric '{}' is not valid".format(timeseries_limit_metric))
-            )
+            raise Exception(_(f"Metric '{timeseries_limit_metric}' does not exist"))


Same as above...

john-bodley · 2019-07-18T15:20:54Z

@mistercrunch I addressed your comments.

[cherry] Picking apache#7888

pull-request-size bot added the size/L label Jul 17, 2019

john-bodley commented Jul 17, 2019

View reviewed changes

williaster approved these changes Jul 17, 2019

View reviewed changes

john-bodley force-pushed the john-bodley--fix-invalid-column branch 2 times, most recently from 651d674 to 17c2907 Compare July 17, 2019 23:12

mistercrunch reviewed Jul 18, 2019

View reviewed changes

john-bodley changed the title ~~[sqla] Adding check for invalid filter columns~~ [fix] Adding check for invalid filter columns Jul 18, 2019

[sqla] Adding check for invalid filter columns

06d26bd

john-bodley force-pushed the john-bodley--fix-invalid-column branch from 17c2907 to 06d26bd Compare July 18, 2019 15:18

john-bodley merged commit 2b3e7fe into apache:master Jul 18, 2019

john-bodley deleted the john-bodley--fix-invalid-column branch July 18, 2019 15:51

john-bodley mentioned this pull request Jul 18, 2019

[cherry] Picking #7888 airbnb/superset-fork#142

Merged

12 tasks

john-bodley pushed a commit to john-bodley/superset that referenced this pull request Jul 18, 2019

[cherry] cherry-picking apache#7888

2a1ee47

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Jul 18, 2019

Merge pull request #142 from john-bodley/john-bodley--cherry-pick-7888

9c4207c

[cherry] Picking apache#7888

john-bodley mentioned this pull request Jul 26, 2019

[revert] Partial revert of #7888 #7933

Merged

12 tasks

alex-mark pushed a commit to alex-mark/incubator-superset that referenced this pull request Jul 29, 2019

[sqla] Adding check for invalid filter columns (apache#7888)

42e50e3

john-bodley pushed a commit to john-bodley/superset that referenced this pull request Jul 29, 2019

[revert] Partial revert of apache#7888

caf7e6c

john-bodley added a commit that referenced this pull request Jul 29, 2019

[revert] Partial revert of #7888 (#7933)

af462fe

graceguo-supercat mentioned this pull request Aug 11, 2020

[SIP-45] Proposal for improved UX for dashboard filter eligibility #9935

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Adding check for invalid filter columns #7888

[fix] Adding check for invalid filter columns #7888

john-bodley commented Jul 17, 2019

john-bodley Jul 17, 2019

williaster left a comment

williaster Jul 17, 2019

john-bodley Jul 17, 2019 •

edited

Loading

williaster Jul 17, 2019

codecov-io commented Jul 17, 2019 •

edited

Loading

john-bodley commented Jul 17, 2019

mistercrunch Jul 18, 2019

mistercrunch Jul 18, 2019

john-bodley commented Jul 18, 2019

[fix] Adding check for invalid filter columns #7888

[fix] Adding check for invalid filter columns #7888

Conversation

john-bodley commented Jul 17, 2019

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

john-bodley Jul 17, 2019

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

williaster Jul 17, 2019

Choose a reason for hiding this comment

john-bodley Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

williaster Jul 17, 2019

Choose a reason for hiding this comment

codecov-io commented Jul 17, 2019 • edited Loading

Codecov Report

john-bodley commented Jul 17, 2019

mistercrunch Jul 18, 2019

Choose a reason for hiding this comment

mistercrunch Jul 18, 2019

Choose a reason for hiding this comment

john-bodley commented Jul 18, 2019

john-bodley Jul 17, 2019 •

edited

Loading

codecov-io commented Jul 17, 2019 •

edited

Loading