[sql_json] allow not specifying client_id #5730

mistercrunch · 2018-08-25T22:15:39Z

We're opening the sql_json endpoint at Lyft to other apps leveraging
Superset as a data-access layer that enforces authentication and our data
access policy.

Currently sql_json requires the client to pass a client_id parameter
that uniquely identifies the query, that can then be used for polling
when in async mode. This PR makes it such that you don't have to define
a client_id anymore. It just gets generated when not passed.

We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed.

codecov-io · 2018-08-26T03:47:01Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cae0704). Click here to learn what that means.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##             master    #5730   +/-   ##
=========================================
  Coverage          ?   63.38%           
=========================================
  Files             ?      361           
  Lines             ?    22998           
  Branches          ?     2559           
=========================================
  Hits              ?    14577           
  Misses            ?     8406           
  Partials          ?       15

Impacted Files	Coverage Δ
superset/views/core.py	`73.95% <100%> (ø)`
superset/utils.py	`88.98% <50%> (ø)`

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 cae0704...862075f. Read the comment docs.

betodealmeida · 2018-08-27T23:32:18Z

superset/views/core.py

@@ -2406,8 +2408,8 @@ def sql_json(self):
            status=QueryStatus.PENDING if async_ else QueryStatus.RUNNING,
            sql_editor_id=request.form.get('sql_editor_id'),
            tmp_table_name=tmp_table_name,
-            user_id=int(g.user.get_id()),
-            client_id=request.form.get('client_id'),
+            user_id=int(g.user.get_id()) if g.user and g.user.get_id() else None,


I know that right now admin has user id 1, but in the future it could change to be 0 and it would break this code. I think it's better to be more explicit here an unravel the one liner:

client_id = request.form.get('client_id') or utils.shortid() try: user_id = int(g.user.get_id()) except Exception: user_id = None query = Query( ... user_id=user_id, client_id=client_id, )

What I'm fixing here is that g.user.get_id() returns None when using the token auth mechanism on our fork. int(None) raises.

I can change this to g.user.get_id() is not None

With the try block, int(None) would raise and user_id would be set to None.

I'm not even sure that the int casting is necessary here, let me check

Turns out it isn't necessary as far as I understand.

* [sql_json] allow not specifying client_id We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed. * adressing comments (cherry picked from commit 5616d7b) (cherry picked from commit bb0c888)

* [sql_json] allow not specifying client_id We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed. * adressing comments (cherry picked from commit 5616d7b)

* [sql_json] allow not specifying client_id We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed. * adressing comments (cherry picked from commit 5616d7b) (cherry picked from commit bb0c888)

* [sql_json] allow not specifying client_id We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed. * adressing comments

* [sql_json] allow not specifying client_id We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed. * adressing comments (cherry picked from commit 5616d7b) (cherry picked from commit bb0c888)

* [sql_json] allow not specifying client_id We're opening the sql_json endpoint at Lyft to other apps leveraging Superset as a data-access layer that enforces authentication and our data access policy. Currently sql_json requires the client to pass a `client_id` parameter that uniquely identifies the query, that can then be used for polling when in async mode. This PR makes it such that you don't have to define a client_id anymore. It just gets generated when not passed. * adressing comments

mistercrunch force-pushed the autoshortid branch from 47cffab to 862075f Compare August 26, 2018 03:31

betodealmeida reviewed Aug 27, 2018

View reviewed changes

adressing comments

6ae0485

mistercrunch merged commit 5616d7b into apache:master Sep 6, 2018

mistercrunch deleted the autoshortid branch September 6, 2018 05:34

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

[sql_json] allow not specifying client_id #5730

[sql_json] allow not specifying client_id #5730

mistercrunch commented Aug 25, 2018

codecov-io commented Aug 26, 2018 •

edited

Loading

betodealmeida Aug 27, 2018

mistercrunch Aug 28, 2018

betodealmeida Aug 28, 2018

mistercrunch Aug 28, 2018

mistercrunch Aug 28, 2018

[sql_json] allow not specifying client_id #5730

[sql_json] allow not specifying client_id #5730

Conversation

mistercrunch commented Aug 25, 2018

codecov-io commented Aug 26, 2018 • edited Loading

Codecov Report

betodealmeida Aug 27, 2018

Choose a reason for hiding this comment

mistercrunch Aug 28, 2018

Choose a reason for hiding this comment

betodealmeida Aug 28, 2018

Choose a reason for hiding this comment

mistercrunch Aug 28, 2018

Choose a reason for hiding this comment

mistercrunch Aug 28, 2018

Choose a reason for hiding this comment

codecov-io commented Aug 26, 2018 •

edited

Loading