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/01/24 16:54:28 UTC
[GitHub] [superset] michael-s-molina opened a new pull request #18151: refactor: Moves the Explore form_data endpoint
michael-s-molina opened a new pull request #18151:
URL: https://github.com/apache/superset/pull/18151
### SUMMARY
The objective of this PR is to move the Explore `form_data` endpoint from `/charts` to `/explore`. This change became clear when analyzing the type of data contained in `form_data` and the security rules that should be applied to the endpoint. The `/explore` endpoint is done under the `v1` API and all security checks were kept the same. We still check for dataset and chart access independently of each other.
I also implemented some suggestions made by @ktmud during the review process of https://github.com/apache/superset/pull/17882:
- Enhanced the `Entry` structure to also store the `dataset_id` and `chart_id` which will allow us to only keep the key in the URL
- Removed the hacky `chart_id == 0` in the URL by creating a specific endpoint
Follow-up of https://github.com/apache/superset/pull/17882
### TESTING INSTRUCTIONS
1 - Execute all tests
2 - All tests should pass
### ADDITIONAL INFORMATION
- [ ] 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
- [x] Introduces new feature or API
- [x] 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] villebro commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791461686
##########
File path: superset/config.py
##########
@@ -585,7 +585,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
}
# Cache for chart form data
-CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = {
+EXPLORE_FORM_DATA_CACHE_CONFIG: CacheConfig = {
Review comment:
This is a breaking change that needs to be flagged in `UPDATING.md`. Related to this, I've been thinking we should add a config which makes it possible to restrict the cache types that can be used. By restricting to e.g. Redis, we could ensure that the service doesn't start up if the admin has forgotten to add a recently added cache config.
##########
File path: superset/explore/form_data/commands/create.py
##########
@@ -0,0 +1,58 @@
+# 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:
nit: while these are not identical with the old `charts/form_data/command` packages, it would have been nice to move these to their new locations first so the change in logic could have been more transparently recorded in the PR diff.
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791647995
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
+ try:
+ item = self.add_model_schema.load(request.json)
+ args = CommandParameters(
+ actor=g.user,
+ dataset_id=item["dataset_id"],
+ chart_id=item.get("chart_id"),
+ value=item["value"],
+ )
+ key = CreateFormDataCommand(args).run()
+ return self.response(201, key=key)
+ except ValidationError as ex:
+ return self.response(400, message=ex.messages)
+ except (
+ ChartAccessDeniedError,
+ DatasetAccessDeniedError,
+ KeyValueAccessDeniedError,
+ ) as ex:
+ return self.response(403, message=str(ex))
+ except (ChartNotFoundError, DatasetNotFoundError) as ex:
+ return self.response(404, message=str(ex))
Review comment:
Great idea. Will do it in another PR to apply it to all endpoints.
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
Review comment:
Great idea. Will do it in another PR to apply it to all endpoints.
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791647746
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
Review comment:
Great idea. Will do in another PR to apply it to all endpoints.
--
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 #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18151:
URL: https://github.com/apache/superset/pull/18151#issuecomment-1020430708
# [Codecov](https://codecov.io/gh/apache/superset/pull/18151?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 [#18151](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d239710) into [master](https://codecov.io/gh/apache/superset/commit/e632193eb00803594a1bbc20c2f6cb6fb29deb1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e632193) will **decrease** coverage by `0.15%`.
> The diff coverage is `91.05%`.
> :exclamation: Current head d239710 differs from pull request most recent head c6e09c7. Consider uploading reports for the commit c6e09c7 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18151 +/- ##
==========================================
- Coverage 65.94% 65.79% -0.16%
==========================================
Files 1584 1587 +3
Lines 62055 62166 +111
Branches 6273 6273
==========================================
- Hits 40925 40901 -24
- Misses 19509 19644 +135
Partials 1621 1621
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.15% <91.05%> (-0.03%)` | :arrow_down: |
| postgres | `81.20% <91.05%> (-0.03%)` | :arrow_down: |
| presto | `?` | |
| python | `81.29% <91.05%> (-0.38%)` | :arrow_down: |
| sqlite | `80.89% <91.05%> (-0.03%)` | :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/18151?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-frontend/src/setup/setupColors.ts](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLnRz) | `76.92% <ø> (ø)` | |
| [superset/explore/form\_data/commands/update.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvdXBkYXRlLnB5) | `85.29% <85.29%> (ø)` | |
| [superset/explore/form\_data/commands/delete.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZGVsZXRlLnB5) | `86.20% <86.20%> (ø)` | |
| [superset/explore/form\_data/commands/get.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZ2V0LnB5) | `87.87% <87.87%> (ø)` | |
| [superset/explore/form\_data/commands/create.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvY3JlYXRlLnB5) | `90.00% <90.00%> (ø)` | |
| [superset/explore/form\_data/api.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvYXBpLnB5) | `93.75% <92.59%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvY29uZmlnLnB5) | `91.74% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/entry.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZW50cnkucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/parameters.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvcGFyYW1ldGVycy5weQ==) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/schemas.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvc2NoZW1hcy5weQ==) | `100.00% <100.00%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/superset/pull/18151/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e632193...c6e09c7](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov[bot] edited a comment on pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18151:
URL: https://github.com/apache/superset/pull/18151#issuecomment-1020430708
# [Codecov](https://codecov.io/gh/apache/superset/pull/18151?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 [#18151](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (be7a13b) into [master](https://codecov.io/gh/apache/superset/commit/e632193eb00803594a1bbc20c2f6cb6fb29deb1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e632193) will **decrease** coverage by `0.16%`.
> The diff coverage is `92.02%`.
> :exclamation: Current head be7a13b differs from pull request most recent head a2daa48. Consider uploading reports for the commit a2daa48 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18151 +/- ##
==========================================
- Coverage 65.94% 65.78% -0.17%
==========================================
Files 1584 1587 +3
Lines 62055 62175 +120
Branches 6273 6273
==========================================
- Hits 40925 40903 -22
- Misses 19509 19651 +142
Partials 1621 1621
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `52.15% <56.15%> (+<0.01%)` | :arrow_up: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.00% <56.15%> (+<0.01%)` | :arrow_up: |
| python | `81.27% <92.02%> (-0.40%)` | :arrow_down: |
| sqlite | `80.90% <92.02%> (-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/18151?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-frontend/src/setup/setupColors.ts](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLnRz) | `83.33% <ø> (+6.41%)` | :arrow_up: |
| [superset/models/helpers.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvbW9kZWxzL2hlbHBlcnMucHk=) | `90.66% <ø> (ø)` | |
| [superset/explore/form\_data/commands/update.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvdXBkYXRlLnB5) | `85.29% <85.29%> (ø)` | |
| [superset/explore/form\_data/commands/delete.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZGVsZXRlLnB5) | `86.20% <86.20%> (ø)` | |
| [superset/explore/form\_data/commands/get.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZ2V0LnB5) | `87.87% <87.87%> (ø)` | |
| [superset/explore/form\_data/commands/create.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvY3JlYXRlLnB5) | `90.00% <90.00%> (ø)` | |
| [superset/explore/form\_data/api.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvYXBpLnB5) | `93.75% <93.75%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvY29uZmlnLnB5) | `91.77% <100.00%> (+0.02%)` | :arrow_up: |
| [superset/connectors/base/models.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvY29ubmVjdG9ycy9iYXNlL21vZGVscy5weQ==) | `88.27% <100.00%> (+0.07%)` | :arrow_up: |
| [superset/explore/form\_data/commands/parameters.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvcGFyYW1ldGVycy5weQ==) | `100.00% <100.00%> (ø)` | |
| ... and [31 more](https://codecov.io/gh/apache/superset/pull/18151/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e632193...a2daa48](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov[bot] edited a comment on pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18151:
URL: https://github.com/apache/superset/pull/18151#issuecomment-1020430708
# [Codecov](https://codecov.io/gh/apache/superset/pull/18151?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 [#18151](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d239710) into [master](https://codecov.io/gh/apache/superset/commit/e632193eb00803594a1bbc20c2f6cb6fb29deb1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e632193) will **decrease** coverage by `0.15%`.
> The diff coverage is `91.05%`.
> :exclamation: Current head d239710 differs from pull request most recent head a2daa48. Consider uploading reports for the commit a2daa48 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18151 +/- ##
==========================================
- Coverage 65.94% 65.79% -0.16%
==========================================
Files 1584 1587 +3
Lines 62055 62166 +111
Branches 6273 6273
==========================================
- Hits 40925 40901 -24
- Misses 19509 19644 +135
Partials 1621 1621
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.15% <91.05%> (-0.03%)` | :arrow_down: |
| postgres | `81.20% <91.05%> (-0.03%)` | :arrow_down: |
| presto | `?` | |
| python | `81.29% <91.05%> (-0.38%)` | :arrow_down: |
| sqlite | `80.89% <91.05%> (-0.03%)` | :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/18151?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-frontend/src/setup/setupColors.ts](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLnRz) | `76.92% <ø> (ø)` | |
| [superset/explore/form\_data/commands/update.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvdXBkYXRlLnB5) | `85.29% <85.29%> (ø)` | |
| [superset/explore/form\_data/commands/delete.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZGVsZXRlLnB5) | `86.20% <86.20%> (ø)` | |
| [superset/explore/form\_data/commands/get.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZ2V0LnB5) | `87.87% <87.87%> (ø)` | |
| [superset/explore/form\_data/commands/create.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvY3JlYXRlLnB5) | `90.00% <90.00%> (ø)` | |
| [superset/explore/form\_data/api.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvYXBpLnB5) | `93.75% <92.59%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvY29uZmlnLnB5) | `91.74% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/entry.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZW50cnkucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/parameters.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvcGFyYW1ldGVycy5weQ==) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/schemas.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvc2NoZW1hcy5weQ==) | `100.00% <100.00%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/superset/pull/18151/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e632193...a2daa48](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] codecov[bot] edited a comment on pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18151:
URL: https://github.com/apache/superset/pull/18151#issuecomment-1020430708
# [Codecov](https://codecov.io/gh/apache/superset/pull/18151?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 [#18151](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d239710) into [master](https://codecov.io/gh/apache/superset/commit/e632193eb00803594a1bbc20c2f6cb6fb29deb1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e632193) will **decrease** coverage by `0.15%`.
> The diff coverage is `91.05%`.
> :exclamation: Current head d239710 differs from pull request most recent head c6e09c7. Consider uploading reports for the commit c6e09c7 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18151 +/- ##
==========================================
- Coverage 65.94% 65.79% -0.16%
==========================================
Files 1584 1587 +3
Lines 62055 62166 +111
Branches 6273 6273
==========================================
- Hits 40925 40901 -24
- Misses 19509 19644 +135
Partials 1621 1621
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.15% <91.05%> (-0.03%)` | :arrow_down: |
| postgres | `81.20% <91.05%> (-0.03%)` | :arrow_down: |
| presto | `?` | |
| python | `81.29% <91.05%> (-0.38%)` | :arrow_down: |
| sqlite | `80.89% <91.05%> (-0.03%)` | :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/18151?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-frontend/src/setup/setupColors.ts](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLnRz) | `76.92% <ø> (ø)` | |
| [superset/explore/form\_data/commands/update.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvdXBkYXRlLnB5) | `85.29% <85.29%> (ø)` | |
| [superset/explore/form\_data/commands/delete.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZGVsZXRlLnB5) | `86.20% <86.20%> (ø)` | |
| [superset/explore/form\_data/commands/get.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZ2V0LnB5) | `87.87% <87.87%> (ø)` | |
| [superset/explore/form\_data/commands/create.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvY3JlYXRlLnB5) | `90.00% <90.00%> (ø)` | |
| [superset/explore/form\_data/api.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvYXBpLnB5) | `93.75% <92.59%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvY29uZmlnLnB5) | `91.74% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/entry.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZW50cnkucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/parameters.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvcGFyYW1ldGVycy5weQ==) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/schemas.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvc2NoZW1hcy5weQ==) | `100.00% <100.00%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/superset/pull/18151/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e632193...c6e09c7](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] villebro commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791461686
##########
File path: superset/config.py
##########
@@ -585,7 +585,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
}
# Cache for chart form data
-CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = {
+EXPLORE_FORM_DATA_CACHE_CONFIG: CacheConfig = {
Review comment:
This is a breaking change that needs to be flagged in `UPDATING.md`. Related to this, I've been thinking we should add a config which makes it possible to restrict the cache types that can be used. By restricting to e.g. Redis, we could ensure that the service doesn't start up if the admin has forgotten to add a recently added cache config.
##########
File path: superset/explore/form_data/commands/create.py
##########
@@ -0,0 +1,58 @@
+# 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:
nit: while these are not identical with the old `charts/form_data/command` packages, it would have been nice to move these to their new locations first so the change in logic could have been more transparently recorded in the PR diff.
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791663538
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
Review comment:
The `dataset_id` is always required but the `chart_id` should be optional. Will change.
--
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] edited a comment on pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #18151:
URL: https://github.com/apache/superset/pull/18151#issuecomment-1020430708
--
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] michael-s-molina merged pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina merged pull request #18151:
URL: https://github.com/apache/superset/pull/18151
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791663726
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
+ value: str
Review comment:
Good point. Will change.
--
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] michael-s-molina merged pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina merged pull request #18151:
URL: https://github.com/apache/superset/pull/18151
--
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] ktmud commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791185706
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
Review comment:
Can we rename this to something like `TemporaryExploreState` to be more specific?
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
+ try:
+ item = self.add_model_schema.load(request.json)
+ args = CommandParameters(
+ actor=g.user,
+ dataset_id=item["dataset_id"],
+ chart_id=item.get("chart_id"),
+ value=item["value"],
+ )
+ key = CreateFormDataCommand(args).run()
+ return self.response(201, key=key)
+ except ValidationError as ex:
+ return self.response(400, message=ex.messages)
+ except (
+ ChartAccessDeniedError,
+ DatasetAccessDeniedError,
+ KeyValueAccessDeniedError,
+ ) as ex:
+ return self.response(403, message=str(ex))
+ except (ChartNotFoundError, DatasetNotFoundError) as ex:
+ return self.response(404, message=str(ex))
Review comment:
We should have a general decorator to handle common 403 and 404 error if not already.
Maybe in `superset.commands.exceptions` add a:
```python
def handle_command_exceptions():
...
```
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
Review comment:
Shouldn't `dataset_id` and `chart_id` be `Optional[int]`?
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
+ value: str
Review comment:
Since this is already a typed dict and does not need to be generic, I'd recommend renaming `value` to `form_data`.
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
Review comment:
Do we have a decorator for this? Might want to add one if not yet.
--
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 #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #18151:
URL: https://github.com/apache/superset/pull/18151#issuecomment-1020430708
# [Codecov](https://codecov.io/gh/apache/superset/pull/18151?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 [#18151](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d239710) into [master](https://codecov.io/gh/apache/superset/commit/e632193eb00803594a1bbc20c2f6cb6fb29deb1f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e632193) will **decrease** coverage by `0.15%`.
> The diff coverage is `91.05%`.
> :exclamation: Current head d239710 differs from pull request most recent head c6e09c7. Consider uploading reports for the commit c6e09c7 to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18151 +/- ##
==========================================
- Coverage 65.94% 65.79% -0.16%
==========================================
Files 1584 1587 +3
Lines 62055 62166 +111
Branches 6273 6273
==========================================
- Hits 40925 40901 -24
- Misses 19509 19644 +135
Partials 1621 1621
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.15% <91.05%> (-0.03%)` | :arrow_down: |
| postgres | `81.20% <91.05%> (-0.03%)` | :arrow_down: |
| presto | `?` | |
| python | `81.29% <91.05%> (-0.38%)` | :arrow_down: |
| sqlite | `80.89% <91.05%> (-0.03%)` | :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/18151?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-frontend/src/setup/setupColors.ts](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLnRz) | `76.92% <ø> (ø)` | |
| [superset/explore/form\_data/commands/update.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvdXBkYXRlLnB5) | `85.29% <85.29%> (ø)` | |
| [superset/explore/form\_data/commands/delete.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZGVsZXRlLnB5) | `86.20% <86.20%> (ø)` | |
| [superset/explore/form\_data/commands/get.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZ2V0LnB5) | `87.87% <87.87%> (ø)` | |
| [superset/explore/form\_data/commands/create.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvY3JlYXRlLnB5) | `90.00% <90.00%> (ø)` | |
| [superset/explore/form\_data/api.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvYXBpLnB5) | `93.75% <92.59%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvY29uZmlnLnB5) | `91.74% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/entry.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvZW50cnkucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/commands/parameters.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvY29tbWFuZHMvcGFyYW1ldGVycy5weQ==) | `100.00% <100.00%> (ø)` | |
| [superset/explore/form\_data/schemas.py](https://codecov.io/gh/apache/superset/pull/18151/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-c3VwZXJzZXQvZXhwbG9yZS9mb3JtX2RhdGEvc2NoZW1hcy5weQ==) | `100.00% <100.00%> (ø)` | |
| ... and [12 more](https://codecov.io/gh/apache/superset/pull/18151/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) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e632193...c6e09c7](https://codecov.io/gh/apache/superset/pull/18151?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?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] ktmud commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r792827207
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
+ try:
+ item = self.add_model_schema.load(request.json)
+ args = CommandParameters(
+ actor=g.user,
+ dataset_id=item["dataset_id"],
+ chart_id=item.get("chart_id"),
+ value=item["value"],
+ )
+ key = CreateFormDataCommand(args).run()
+ return self.response(201, key=key)
+ except ValidationError as ex:
+ return self.response(400, message=ex.messages)
+ except (
+ ChartAccessDeniedError,
+ DatasetAccessDeniedError,
+ KeyValueAccessDeniedError,
+ ) as ex:
+ return self.response(403, message=str(ex))
+ except (ChartNotFoundError, DatasetNotFoundError) as ex:
+ return self.response(404, message=str(ex))
Review comment:
@michael-s-molina Just realized we have a [handle_api_exception](https://github.com/apache/superset/blob/c6c4143b044a6ed19db2c3c8bc9b5abdd1cb3a78/superset/views/base.py#L194) already. We may be good by just adding some base command exceptions to this handler.
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791666764
##########
File path: superset/config.py
##########
@@ -585,7 +585,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
}
# Cache for chart form data
-CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = {
+EXPLORE_FORM_DATA_CACHE_CONFIG: CacheConfig = {
Review comment:
Great catch! I'll update the file. I liked the idea of restricting the cache types. I will do it in a separate PR where I'll also improve the cache_manager initialization.
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791647746
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
Review comment:
Great idea. Will do in another PR to apply it to all endpoints.
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
+ try:
+ item = self.add_model_schema.load(request.json)
+ args = CommandParameters(
+ actor=g.user,
+ dataset_id=item["dataset_id"],
+ chart_id=item.get("chart_id"),
+ value=item["value"],
+ )
+ key = CreateFormDataCommand(args).run()
+ return self.response(201, key=key)
+ except ValidationError as ex:
+ return self.response(400, message=ex.messages)
+ except (
+ ChartAccessDeniedError,
+ DatasetAccessDeniedError,
+ KeyValueAccessDeniedError,
+ ) as ex:
+ return self.response(403, message=str(ex))
+ except (ChartNotFoundError, DatasetNotFoundError) as ex:
+ return self.response(404, message=str(ex))
Review comment:
Great idea. Will do it in another PR to apply it to all endpoints.
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
Review comment:
Great idea. Will do it in another PR to apply it to all endpoints.
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
Review comment:
The `dataset_id` is always required but the `chart_id` should be optional. Will change.
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
+ value: str
Review comment:
Good point. Will change.
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
Review comment:
Yep
##########
File path: superset/config.py
##########
@@ -585,7 +585,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
}
# Cache for chart form data
-CHART_FORM_DATA_CACHE_CONFIG: CacheConfig = {
+EXPLORE_FORM_DATA_CACHE_CONFIG: CacheConfig = {
Review comment:
Great catch! I'll update the file. I liked the idea of restricting the cache types. I will do it in a separate PR where I'll also improve the cache_manager initialization.
--
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] ktmud commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791185706
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
Review comment:
Can we rename this to something like `TemporaryExploreState` to be more specific?
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
+ try:
+ item = self.add_model_schema.load(request.json)
+ args = CommandParameters(
+ actor=g.user,
+ dataset_id=item["dataset_id"],
+ chart_id=item.get("chart_id"),
+ value=item["value"],
+ )
+ key = CreateFormDataCommand(args).run()
+ return self.response(201, key=key)
+ except ValidationError as ex:
+ return self.response(400, message=ex.messages)
+ except (
+ ChartAccessDeniedError,
+ DatasetAccessDeniedError,
+ KeyValueAccessDeniedError,
+ ) as ex:
+ return self.response(403, message=str(ex))
+ except (ChartNotFoundError, DatasetNotFoundError) as ex:
+ return self.response(404, message=str(ex))
Review comment:
We should have a general decorator to handle common 403 and 404 error if not already.
Maybe in `superset.commands.exceptions` add a:
```python
def handle_command_exceptions():
...
```
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
Review comment:
Shouldn't `dataset_id` and `chart_id` be `Optional[int]`?
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
+ owner: int
+ dataset_id: int
+ chart_id: int
+ value: str
Review comment:
Since this is already a typed dict and does not need to be generic, I'd recommend renaming `value` to `form_data`.
##########
File path: superset/explore/form_data/api.py
##########
@@ -96,41 +98,53 @@ def post(self, pk: int) -> Response:
500:
$ref: '#/components/responses/500'
"""
- return super().post(pk)
+ if not request.is_json:
+ raise InvalidPayloadFormatError("Request is not JSON")
Review comment:
Do we have a decorator for this? Might want to add one if not yet.
--
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] michael-s-molina commented on a change in pull request #18151: refactor: Moves the Explore form_data endpoint
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #18151:
URL: https://github.com/apache/superset/pull/18151#discussion_r791663834
##########
File path: superset/explore/form_data/commands/entry.py
##########
@@ -0,0 +1,24 @@
+# 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.
+from typing_extensions import TypedDict
+
+
+class Entry(TypedDict):
Review comment:
Yep
--
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