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/08/07 13:24:54 UTC
[GitHub] [superset] EugeneTorap opened a new pull request, #21003: fix: Validate required fields in sql_json API
EugeneTorap opened a new pull request, #21003:
URL: https://github.com/apache/superset/pull/21003
fix #20873 issues.
### SUMMARY
sql_json API has two required fields:
- sql: string
- database_id: integer
We need to validate these fields if they are `None` and send user an understandable error if one of the fields is missing.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually 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:
- [ ] 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] codecov[bot] commented on pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21003:
URL: https://github.com/apache/superset/pull/21003#issuecomment-1207409306
# [Codecov](https://codecov.io/gh/apache/superset/pull/21003?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 [#21003](https://codecov.io/gh/apache/superset/pull/21003?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1e09035) into [master](https://codecov.io/gh/apache/superset/commit/e214e1ace616c3fdd40fcf64c501e08407feb8b3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e214e1a) will **decrease** coverage by `11.65%`.
> The diff coverage is `66.66%`.
```diff
@@ Coverage Diff @@
## master #21003 +/- ##
===========================================
- Coverage 66.34% 54.68% -11.66%
===========================================
Files 1767 1767
Lines 67312 67321 +9
Branches 7144 7144
===========================================
- Hits 44656 36817 -7839
- Misses 20828 28676 +7848
Partials 1828 1828
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.05% <66.66%> (+<0.01%)` | :arrow_up: |
| python | `57.42% <66.66%> (-24.05%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.45% <0.00%> (-0.02%)` | :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/21003?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/views/core.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `34.58% <66.66%> (-40.95%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `29.41% <0.00%> (-68.63%)` | :arrow_down: |
| [superset/datasets/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21003/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-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvaW1wb3J0ZXJzL3YwLnB5) | `24.03% <0.00%> (-67.45%)` | :arrow_down: |
| ... and [279 more](https://codecov.io/gh/apache/superset/pull/21003/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) | |
:mega: Codecov can now indicate which changes are the most critical in Pull Requests. [Learn more](https://about.codecov.io/product/feature/runtime-insights/?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] zhaoyongjie commented on a diff in pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21003:
URL: https://github.com/apache/superset/pull/21003#discussion_r943414508
##########
superset/views/sql_lab/views.py:
##########
@@ -31,7 +31,7 @@
from superset.utils import core as utils
from superset.utils.core import get_user_id
-from .base import BaseSupersetView, DeleteMixin, json_success, SupersetModelView
+from ..base import BaseSupersetView, DeleteMixin, json_success, SupersetModelView
Review Comment:
Could we use the absolute import instead of the relative import?
```python
from superset.views.base import BaseSupersetView, DeleteMixin, json_success, SupersetModelView
```
--
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] zhaoyongjie merged pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged PR #21003:
URL: https://github.com/apache/superset/pull/21003
--
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] EugeneTorap commented on pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #21003:
URL: https://github.com/apache/superset/pull/21003#issuecomment-1208585454
@hughhhh @betodealmeida Can you review 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
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21003:
URL: https://github.com/apache/superset/pull/21003#discussion_r943449016
##########
tests/integration_tests/core_tests.py:
##########
@@ -763,6 +763,52 @@ def test_extra_table_metadata(self):
f"/superset/extra_table_metadata/{example_db.id}/birth_names/{schema}/"
)
+ def test_required_params_in_sql_json(self):
+ self.login()
+ client_id = "{}".format(random.getrandbits(64))[:10]
+
+ data = {"client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {
+ "sql": ["Missing data for required field."],
+ "database_id": ["Missing data for required field."],
+ }
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
+
+ data = {"sql": "SELECT 1", "client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {"database_id": ["Missing data for required field."]}
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
+
+ data = {"database_id": 1, "client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {"sql": ["Missing data for required field."]}
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
Review Comment:
same before
##########
tests/integration_tests/core_tests.py:
##########
@@ -763,6 +763,52 @@ def test_extra_table_metadata(self):
f"/superset/extra_table_metadata/{example_db.id}/birth_names/{schema}/"
)
+ def test_required_params_in_sql_json(self):
+ self.login()
+ client_id = "{}".format(random.getrandbits(64))[:10]
+
+ data = {"client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {
+ "sql": ["Missing data for required field."],
+ "database_id": ["Missing data for required field."],
+ }
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
+
+ data = {"sql": "SELECT 1", "client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {"database_id": ["Missing data for required field."]}
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
+
+ data = {"database_id": 1, "client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {"sql": ["Missing data for required field."]}
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
+
+ data = {"sql": "SELECT 1", "database_id": 1, "client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(resp_data.get("status"), "success")
+ self.assertEqual(rv.status_code, 200)
Review Comment:
same before
##########
tests/integration_tests/core_tests.py:
##########
@@ -763,6 +763,52 @@ def test_extra_table_metadata(self):
f"/superset/extra_table_metadata/{example_db.id}/birth_names/{schema}/"
)
+ def test_required_params_in_sql_json(self):
+ self.login()
+ client_id = "{}".format(random.getrandbits(64))[:10]
+
+ data = {"client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {
+ "sql": ["Missing data for required field."],
+ "database_id": ["Missing data for required field."],
+ }
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
Review Comment:
no blocking, nits
```suggestion
assert rv.json == failed_resp
assert rv.status_code == 400
```
##########
tests/integration_tests/core_tests.py:
##########
@@ -763,6 +763,52 @@ def test_extra_table_metadata(self):
f"/superset/extra_table_metadata/{example_db.id}/birth_names/{schema}/"
)
+ def test_required_params_in_sql_json(self):
+ self.login()
+ client_id = "{}".format(random.getrandbits(64))[:10]
+
+ data = {"client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {
+ "sql": ["Missing data for required field."],
+ "database_id": ["Missing data for required field."],
+ }
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
+
+ data = {"sql": "SELECT 1", "client_id": client_id}
+ rv = self.client.post(
+ "/superset/sql_json/",
+ json=data,
+ )
+ failed_resp = {"database_id": ["Missing data for required field."]}
+ resp_data = json.loads(rv.data.decode("utf-8"))
+ self.assertDictEqual(resp_data, failed_resp)
+ self.assertEqual(rv.status_code, 400)
Review Comment:
same before
--
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] rusackas commented on a diff in pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #21003:
URL: https://github.com/apache/superset/pull/21003#discussion_r951592498
##########
superset/views/sql_lab/__init__.py:
##########
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
Review Comment:
Is this file necessary?
--
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] EugeneTorap commented on a diff in pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on code in PR #21003:
URL: https://github.com/apache/superset/pull/21003#discussion_r943438413
##########
superset/views/sql_lab/views.py:
##########
@@ -31,7 +31,7 @@
from superset.utils import core as utils
from superset.utils.core import get_user_id
-from .base import BaseSupersetView, DeleteMixin, json_success, SupersetModelView
+from ..base import BaseSupersetView, DeleteMixin, json_success, SupersetModelView
Review Comment:
Sure
--
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] zhaoyongjie commented on pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21003:
URL: https://github.com/apache/superset/pull/21003#issuecomment-1209374831
Hi @EugeneTorap, Thanks for adding validation on the endpoint. Do you mind following the [Mashmallow](https://marshmallow.readthedocs.io/en/stable/) validation schema for the requesting validation? Here are some [examples](https://github.com/apache/superset/blob/665007622845cf4303b50e57d11549a162dbbb1b/superset/views/datasource/views.py#L190-L195).
--
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] EugeneTorap commented on pull request #21003: fix: Validate required fields in sql_json API
Posted by GitBox <gi...@apache.org>.
EugeneTorap commented on PR #21003:
URL: https://github.com/apache/superset/pull/21003#issuecomment-1209384447
> Hi @EugeneTorap, Thanks for adding validation on the endpoint. Do you mind following the [Mashmallow](https://marshmallow.readthedocs.io/en/stable/) validation schema for the requesting validation? Here are some [examples](https://github.com/apache/superset/blob/665007622845cf4303b50e57d11549a162dbbb1b/superset/views/datasource/views.py#L190-L195).
Thanks, will do 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