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 2021/10/04 14:18:07 UTC
[GitHub] [superset] ofekisr opened a new pull request #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
ofekisr opened a new pull request #16957:
URL: https://github.com/apache/superset/pull/16957
### SUMMARY
on creating new filterset with extra fields inside json_metadata, it failed due to parsing error
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
new test added
### ADDITIONAL INFORMATION
--
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 #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721916013
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
This is a general observation with regard to typing - where it isn't strictly enforced, over time it tends to erode. Since filter sets are currently fully restricted to the `apache/superset` repo (=no multirepo problems associated with sharing typing context between `superset-ui` and `apache/superset`), I don't see a major problem making sure that these schemas are in sync. See for instance the dashboard metadata payload schema which has not needed to be updated since active feature development of native filters stabilized: https://github.com/apache/superset/blob/5866d5ebb064648b6ac23c498cc252207f62dfd1/superset/dashboards/schemas.py#L108-L129
Expressing this differently, is there a reason why we can't just add the known extra fields to the schema in this PR, and do the same any follow-up PRs when we need to add new properties?
--
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 #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16957:
URL: https://github.com/apache/superset/pull/16957#issuecomment-933589071
# [Codecov](https://codecov.io/gh/apache/superset/pull/16957?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 [#16957](https://codecov.io/gh/apache/superset/pull/16957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (044309c) into [master](https://codecov.io/gh/apache/superset/commit/0d0c759cfed3dc72aaca029b5da1db1acfa393a9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0d0c759) will **decrease** coverage by `0.21%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/16957?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 #16957 +/- ##
==========================================
- Coverage 76.98% 76.76% -0.22%
==========================================
Files 1028 1028
Lines 55065 55074 +9
Branches 7486 7486
==========================================
- Hits 42390 42277 -113
- Misses 12427 12549 +122
Partials 248 248
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.83% <100.00%> (-0.01%)` | :arrow_down: |
| postgres | `81.89% <100.00%> (-0.01%)` | :arrow_down: |
| presto | `?` | |
| python | `81.97% <100.00%> (-0.42%)` | :arrow_down: |
| sqlite | `81.51% <100.00%> (-0.01%)` | :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/16957?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/dashboards/filter\_sets/consts.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc2V0cy9jb25zdHMucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/dashboards/filter\_sets/schemas.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc2V0cy9zY2hlbWFzLnB5) | `89.74% <100.00%> (-0.26%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/16957/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%> (-82.15%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.76% <0.00%> (-17.06%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.47% <0.00%> (-6.49%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `85.59% <0.00%> (-1.66%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `92.85% <0.00%> (-1.59%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `89.26% <0.00%> (-0.74%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `88.17% <0.00%> (-0.39%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/superset/pull/16957/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/16957?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/16957?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 [0d0c759...044309c](https://codecov.io/gh/apache/superset/pull/16957?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 #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721584290
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
@dpgaspar we've discussed similar problems earlier on on the other endpoints - do you have any thoughts on using `unknown=INCLUDE` for these schemas? My fear is that this leads to gradual decay of the schema, as it won't raise errors on unknown fields, hence as the real schema evolves, this one stays stagnant. To quote the Finnish singer Aki Sirkesalo: "Annoin pikkusormen, se vei koko käden" (I gave my little finger, it took the whole hand).
--
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] ofekisr commented on a change in pull request #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
ofekisr commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721620784
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
It depends on the context and the situation.
right now the backend does nothing with that metadata so it's ok.
I don't understand why do you fear... if logic will be added, schema validation is probably required, and that is the reviewer's responsibility to insure it. if you don't trust others they will enforce it, this is a different problem
--
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 #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #16957:
URL: https://github.com/apache/superset/pull/16957#issuecomment-933589071
# [Codecov](https://codecov.io/gh/apache/superset/pull/16957?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 [#16957](https://codecov.io/gh/apache/superset/pull/16957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (044309c) into [master](https://codecov.io/gh/apache/superset/commit/0d0c759cfed3dc72aaca029b5da1db1acfa393a9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0d0c759) will **decrease** coverage by `0.21%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/superset/pull/16957?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 #16957 +/- ##
==========================================
- Coverage 76.98% 76.76% -0.22%
==========================================
Files 1028 1028
Lines 55065 55074 +9
Branches 7486 7486
==========================================
- Hits 42390 42277 -113
- Misses 12427 12549 +122
Partials 248 248
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.83% <100.00%> (-0.01%)` | :arrow_down: |
| postgres | `81.89% <100.00%> (-0.01%)` | :arrow_down: |
| presto | `?` | |
| python | `81.97% <100.00%> (-0.42%)` | :arrow_down: |
| sqlite | `81.51% <100.00%> (-0.01%)` | :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/16957?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/dashboards/filter\_sets/consts.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc2V0cy9jb25zdHMucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/dashboards/filter\_sets/schemas.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc2V0cy9zY2hlbWFzLnB5) | `89.74% <100.00%> (-0.26%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/16957/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%> (-82.15%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.76% <0.00%> (-17.06%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.47% <0.00%> (-6.49%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `85.59% <0.00%> (-1.66%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `92.85% <0.00%> (-1.59%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `89.26% <0.00%> (-0.74%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `88.17% <0.00%> (-0.39%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/superset/pull/16957/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/16957?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/16957?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 [0d0c759...044309c](https://codecov.io/gh/apache/superset/pull/16957?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] ofekisr closed pull request #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
ofekisr closed pull request #16957:
URL: https://github.com/apache/superset/pull/16957
--
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 #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #16957:
URL: https://github.com/apache/superset/pull/16957#issuecomment-933589071
# [Codecov](https://codecov.io/gh/apache/superset/pull/16957?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 [#16957](https://codecov.io/gh/apache/superset/pull/16957?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d03d317) into [master](https://codecov.io/gh/apache/superset/commit/0d0c759cfed3dc72aaca029b5da1db1acfa393a9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0d0c759) will **decrease** coverage by `0.21%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head d03d317 differs from pull request most recent head 044309c. Consider uploading reports for the commit 044309c to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/16957?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 #16957 +/- ##
==========================================
- Coverage 76.98% 76.76% -0.22%
==========================================
Files 1028 1028
Lines 55065 55074 +9
Branches 7486 7486
==========================================
- Hits 42390 42277 -113
- Misses 12427 12549 +122
Partials 248 248
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `81.83% <100.00%> (-0.01%)` | :arrow_down: |
| postgres | `81.89% <100.00%> (-0.01%)` | :arrow_down: |
| presto | `?` | |
| python | `81.97% <100.00%> (-0.42%)` | :arrow_down: |
| sqlite | `81.51% <100.00%> (-0.01%)` | :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/16957?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/dashboards/filter\_sets/consts.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc2V0cy9jb25zdHMucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/dashboards/filter\_sets/schemas.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJfc2V0cy9zY2hlbWFzLnB5) | `89.74% <100.00%> (-0.26%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/16957/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%> (-82.15%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.76% <0.00%> (-17.06%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.47% <0.00%> (-6.49%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `85.59% <0.00%> (-1.66%)` | :arrow_down: |
| [superset/common/query\_object.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | `92.85% <0.00%> (-1.59%)` | :arrow_down: |
| [superset/models/core.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `89.26% <0.00%> (-0.74%)` | :arrow_down: |
| [superset/db\_engine\_specs/base.py](https://codecov.io/gh/apache/superset/pull/16957/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-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | `88.17% <0.00%> (-0.39%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/apache/superset/pull/16957/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/16957?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/16957?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 [0d0c759...044309c](https://codecov.io/gh/apache/superset/pull/16957?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 #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721516528
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
We've hit these kinds of problems on the previous schema definitions before, and I wonder if we should just go with a raw schema for these JSON blobs for now? Having a defined schema but allowing for `unknown=INCLUDE` doesn't feel totally right - on one hand we're enforcing schema, but in practice not really. Thoughts?
--
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] ofekisr commented on a change in pull request #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
ofekisr commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721540788
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
it enforces the required field and helps what should be inside but still, it is ok to let other fields who used only by the front.
--
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] ofekisr commented on a change in pull request #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
ofekisr commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721620784
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
It depends on the context and the situation.
right now the backend does nothing with that metadata so it's ok.
I don't understand why do you fear... if logic will be added, schema validation is probably required, and that is the reviewer's responsibility. if you don't trust others they will enforce it, this is different problem
--
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] ofekisr commented on a change in pull request #16957: fix(filterset): failed to parse json_metadata json on creating new filterset
Posted by GitBox <gi...@apache.org>.
ofekisr commented on a change in pull request #16957:
URL: https://github.com/apache/superset/pull/16957#discussion_r721956316
##########
File path: superset/dashboards/filter_sets/schemas.py
##########
@@ -34,7 +34,7 @@ class JsonMetadataSchema(Schema):
class FilterSetSchema(Schema):
- json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema()
+ json_metadata_schema: JsonMetadataSchema = JsonMetadataSchema(unknown=INCLUDE)
Review comment:
@villebro
Ok, you convinced me, so we don't need that PR and we will ensure no extra fields will be sent
I'm closing 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