You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/06/26 22:22:38 UTC
[GitHub] [superset] justinpark opened a new pull request, #24520: fix(native filter): undefined filter dependencies
justinpark opened a new pull request, #24520:
URL: https://github.com/apache/superset/pull/24520
### SUMMARY
`useFilterDependencies` can include the `undefined` value when `filter.cascadeParentIds` value is corrupted because it always appends from `cascadeParentIds` set.
This commit adds the nullish check point for useFilterDependencies to avoid the unexpected error.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
No error thrown
Before:
<img width="468" alt="Screenshot 2023-06-26 at 3 13 00 PM" src="https://github.com/apache/superset/assets/1392866/3668ecd3-b640-4788-b435-9664cbf736d1">
<img width="434" alt="Screenshot 2023-06-26 at 3 18 50 PM" src="https://github.com/apache/superset/assets/1392866/33cd587d-f00a-4766-a05f-004e0bb9d894">
### TESTING INSTRUCTIONS
generate `cascadeParentIds` metadata with nonexistence filterId
### 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
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on pull request #24520: fix(native filter): undefined filter dependencies
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24520:
URL: https://github.com/apache/superset/pull/24520#issuecomment-1611375127
@justinpark Is there a way to fix the corrupted state via migration? I'm worried that this fix won't resolve the problem as `cascadeParentIds` is used in many places that don't use this hook such as `FiltersConfigForm` and `FiltersConfigModal`.
--
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] justinpark commented on pull request #24520: fix(native filter): undefined filter dependencies
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24520:
URL: https://github.com/apache/superset/pull/24520#issuecomment-1610232604
> I think we need to investigate why cascadeParentIds has a filter id that is not present in the Redux state anymore. If we find the root cause, this check you added won't be necessary and we'll probably fix other parts of the code that are being affected by this issue.
@john-bodley @michael-s-molina
The metadata already included the corrupted (non-existence) filter id value which means it happened during migration script.
Whether it's corrupted by the migration script or other bug, this means `useFilterDependencies` contains the potential risk by the corrupted data so this nullish check is valuable for UX
--
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 #24520: fix(native filter): undefined filter dependencies
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24520:
URL: https://github.com/apache/superset/pull/24520#issuecomment-1608468076
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24520](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (cc227d6) into [master](https://app.codecov.io/gh/apache/superset/commit/e20b69587f351056f5f997c19f438b12302dcf0b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e20b695) will **increase** coverage by `0.01%`.
> The diff coverage is `90.90%`.
> :exclamation: Current head cc227d6 differs from pull request most recent head 6d764d9. Consider uploading reports for the commit 6d764d9 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24520 +/- ##
==========================================
+ Coverage 69.06% 69.07% +0.01%
==========================================
Files 1901 1902 +1
Lines 74019 74025 +6
Branches 8116 8118 +2
==========================================
+ Hits 51121 51134 +13
+ Misses 20787 20779 -8
- Partials 2111 2112 +1
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `55.73% <90.90%> (+0.02%)` | :arrow_up: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/fixtures.ts](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9maXh0dXJlcy50cw==) | `100.00% <ø> (ø)` | |
| [.../nativeFilters/FilterCard/useFilterDependencies.ts](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyQ2FyZC91c2VGaWx0ZXJEZXBlbmRlbmNpZXMudHM=) | `88.88% <50.00%> (-11.12%)` | :arrow_down: |
| [...core/src/time-format/utils/denormalizeTimestamp.ts](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdGltZS1mb3JtYXQvdXRpbHMvZGVub3JtYWxpemVUaW1lc3RhbXAudHM=) | `100.00% <100.00%> (ø)` | |
| [...i-core/src/time-format/utils/normalizeTimestamp.ts](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdGltZS1mb3JtYXQvdXRpbHMvbm9ybWFsaXplVGltZXN0YW1wLnRz) | `100.00% <100.00%> (ø)` | |
| [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://app.codecov.io/gh/apache/superset/pull/24520?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `48.25% <100.00%> (+4.98%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] justinpark closed pull request #24520: fix(native filter): undefined filter dependencies
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark closed pull request #24520: fix(native filter): undefined filter dependencies
URL: https://github.com/apache/superset/pull/24520
--
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] john-bodley commented on pull request #24520: fix(native filter): undefined filter dependencies
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24520:
URL: https://github.com/apache/superset/pull/24520#issuecomment-1608455870
Thanks @justinpark for the fix. Do we know why/how the data becomes corrupted?
--
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 pull request #24520: fix(native filter): undefined filter dependencies
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24520:
URL: https://github.com/apache/superset/pull/24520#issuecomment-1609332457
@justinpark I think we need to investigate why `cascadeParentIds` has a filter id that is not present in the Redux state anymore. If we find the root cause, this check you added won't be necessary and we'll probably fix other parts of the code that are being affected by this issue.
--
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