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 2020/04/06 15:53:53 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #9479:
[query] Migrate api v1 query to new location
dpgaspar opened a new pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] robdiciuccio commented on issue #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
robdiciuccio commented on issue #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#issuecomment-610561750
`query` is ambiguous in this endpoint name, and conflicts with top-level `Query` objects in Superset. I propose this new endpoint be renamed to one of the following:
```
POST /api/v1/visualization
POST /api/v1/slice
POST /api/v1/chart-data
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404887332
##########
File path: superset/views/api.py
##########
@@ -44,7 +44,7 @@ def query(self):
security_manager.assert_query_context_permission(query_context)
payload_json = query_context.get_payload()
return json.dumps(
- payload_json, default=utils.json_int_dttm_ser, ignore_nan=True
+ payload_json, default=utils.json_int_dttm_ser, allow_nan=False
)
Review comment:
reverted, this change should not exist
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404760899
##########
File path: tests/queries/api_tests.py
##########
@@ -245,3 +261,25 @@ def test_get_queries_no_data_access(self):
# rollback changes
db.session.delete(query)
db.session.commit()
+
+ def test_query_exec(self):
+ """
+ Query API: Test exec query
+ """
+ self.login(username="admin")
+ qc_dict = self._get_query_context_dict()
Review comment:
nti: I would prefer `query_context` as the variable name.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404750114
##########
File path: superset/queries/api.py
##########
@@ -70,3 +77,112 @@ class QueryRestApi(BaseSupersetModelRestApi):
base_order = ("changed_on", "desc")
openapi_spec_tag = "Queries"
+
+ @expose("/exec", methods=["POST"])
+ @event_logger.log_this
+ @protect()
+ @safe
+ def exec(self) -> Response:
+ """
+ Takes a query_obj constructed in the client and returns payload data response
+ for the given query_obj.
+ ---
+ post:
+ description: >-
+ Takes a query_obj constructed in the client and returns payload data
+ response for the given query_obj
+ requestBody:
+ description: Query object schema
Review comment:
Same here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#issuecomment-610664177
I'm agnostic as to naming. If there's consensus around `chart data` as the preferred name, that's fine by me. I agree that the term `query` is overloaded in Superset right now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] kristw commented on issue #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
kristw commented on issue #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#issuecomment-610671378
I could live with `chart-data`. `visualization` is too specific.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on issue #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#issuecomment-610526704
Can we rename it to `/v1/chartData`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404860359
##########
File path: superset/views/api.py
##########
@@ -44,7 +44,7 @@ def query(self):
security_manager.assert_query_context_permission(query_context)
payload_json = query_context.get_payload()
return json.dumps(
- payload_json, default=utils.json_int_dttm_ser, ignore_nan=True
+ payload_json, default=utils.json_int_dttm_ser, allow_nan=False
)
Review comment:
I actually made the switch from simplejson to json since it's the more current stdlib. Yet json does not implement `ignore_nan` just `allow_nan`. Was taking the chance to update
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404860359
##########
File path: superset/views/api.py
##########
@@ -44,7 +44,7 @@ def query(self):
security_manager.assert_query_context_permission(query_context)
payload_json = query_context.get_payload()
return json.dumps(
- payload_json, default=utils.json_int_dttm_ser, ignore_nan=True
+ payload_json, default=utils.json_int_dttm_ser, allow_nan=False
)
Review comment:
I actually made the switch from simplejson to json since it's the more current stdlib. Yet json does not implement `ignore_nan` just `allow_nan`. Was taking the chance to update
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#issuecomment-610778697
I vote for `chart-data`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404759397
##########
File path: tests/queries/api_tests.py
##########
@@ -67,6 +67,22 @@ def insert_query(
db.session.commit()
return query
+ def _get_query_context_dict(self) -> Dict[str, Any]:
Review comment:
nit: perhaps remove dict from the method name, as the return type is specified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404749605
##########
File path: superset/queries/api.py
##########
@@ -70,3 +77,112 @@ class QueryRestApi(BaseSupersetModelRestApi):
base_order = ("changed_on", "desc")
openapi_spec_tag = "Queries"
+
+ @expose("/exec", methods=["POST"])
+ @event_logger.log_this
+ @protect()
+ @safe
+ def exec(self) -> Response:
+ """
+ Takes a query_obj constructed in the client and returns payload data response
Review comment:
Nit: it is in fact a query context (the query objects are in the `queries` array)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9479: [query]
Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#issuecomment-610861905
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9479?src=pr&el=h1) Report
> Merging [#9479](https://codecov.io/gh/apache/incubator-superset/pull/9479?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4485800e2118f8bc1424bdf67d2200eb5f0056ac&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9479?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9479 +/- ##
=======================================
Coverage 58.76% 58.76%
=======================================
Files 385 385
Lines 12237 12237
Branches 3020 3020
=======================================
Hits 7191 7191
Misses 4862 4862
Partials 184 184
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9479?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9479?src=pr&el=footer). Last update [4485800...bb2149f](https://codecov.io/gh/apache/incubator-superset/pull/9479?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404757857
##########
File path: superset/views/api.py
##########
@@ -44,7 +44,7 @@ def query(self):
security_manager.assert_query_context_permission(query_context)
payload_json = query_context.get_payload()
return json.dumps(
- payload_json, default=utils.json_int_dttm_ser, ignore_nan=True
+ payload_json, default=utils.json_int_dttm_ser, allow_nan=False
)
Review comment:
Perhaps we should leave this as-is, and only implement `allow_nan` in the new EP to correspond with vanilla `json.dumps`, just in case. Looking at the source code, it seems `ignore_nan` is supposed to take precedence over `allow_nan` in `simplejson`: https://github.com/simplejson/simplejson/blob/288e4e005c39a2eb855b5225c5dc8ebcb82eee72/simplejson/__init__.py#L373-L376
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404761006
##########
File path: tests/queries/api_tests.py
##########
@@ -245,3 +261,25 @@ def test_get_queries_no_data_access(self):
# rollback changes
db.session.delete(query)
db.session.commit()
+
+ def test_query_exec(self):
+ """
+ Query API: Test exec query
+ """
+ self.login(username="admin")
+ qc_dict = self._get_query_context_dict()
+ uri = "api/v1/query/exec"
+ rv = self.client.post(uri, json=qc_dict)
+ self.assertEqual(rv.status_code, 200)
+ data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(data[0]["rowcount"], 100)
+
+ def test_query_exec_not_alloed(self):
+ """
+ Query API: Test exec query not allowed
+ """
+ self.login(username="gamma")
+ qc_dict = self._get_query_context_dict()
Review comment:
Same here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404896690
##########
File path: tests/queries/api_tests.py
##########
@@ -245,3 +261,25 @@ def test_get_queries_no_data_access(self):
# rollback changes
db.session.delete(query)
db.session.commit()
+
+ def test_query_exec(self):
+ """
+ Query API: Test exec query
+ """
+ self.login(username="admin")
+ query_context = self._get_query_context()
+ uri = "api/v1/query/exec"
+ rv = self.client.post(uri, json=query_context)
+ self.assertEqual(rv.status_code, 200)
+ data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(data[0]["rowcount"], 100)
+
+ def test_query_exec_not_alloed(self):
Review comment:
```suggestion
def test_query_exec_not_allowed(self):
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9479: [query] Migrate api v1 query to new location
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9479: [query] Migrate api v1 query to new location
URL: https://github.com/apache/incubator-superset/pull/9479#discussion_r404896087
##########
File path: superset/queries/api.py
##########
@@ -70,3 +77,112 @@ class QueryRestApi(BaseSupersetModelRestApi):
base_order = ("changed_on", "desc")
openapi_spec_tag = "Queries"
+
+ @expose("/exec", methods=["POST"])
+ @event_logger.log_this
+ @protect()
+ @safe
+ def exec(self) -> Response:
+ """
+ Takes a query context constructed in the client and returns payload
+ data response for the given query.
+ ---
+ post:
+ description: >-
+ Takes a query context constructed in the client and returns payload data
+ response for the given query.
+ requestBody:
+ description: Query object schema
Review comment:
```suggestion
description: Query context schema
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org