fix: [migration] fix dashboard json_metadata when native filter is not enabled by graceguo-supercat · Pull Request #15197 · apache/superset

graceguo-supercat · 2021-06-16T16:23:24Z

SUMMARY

This PR will scan all dashboard json_metada: if DASHBOARD_NATIVE_FILTERS feature is not enabled, remove show_native_filters and native_filter_configuration properties from json_metadata.

See #14933 (comment) for more context.

TESTING INSTRUCTIONS

CI and manual test.

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided: a few seconds up to 1 minute, depends on the number of dashboards to scan.
Introduces new feature or API
Removes existing feature or API

apache#14551) * Fix column name alignment * Fix space wrap

…trophobic (apache#14569)

* Fix menu * Fix test

Bumps [ssri](https://github.com/npm/ssri) from 6.0.1 to 6.0.2. - [Release notes](https://github.com/npm/ssri/releases) - [Changelog](https://github.com/npm/ssri/blob/v6.0.2/CHANGELOG.md) - [Commits](npm/ssri@v6.0.1...v6.0.2) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: Adding a little margin under the warning about changing datasets * feat: moves Alert spacing from a css override to an Alert prop * fix: prop needs to be optional... proptional * fix: moving the typing to a better spot, adding the new prop to storybook. * style: linting

* fix: bring back dashboard perf logger * add test

* fix dashboard side actions * lint being lint

…ache#14579) * send queryid up with setSQL * pass latest query id * update * cleanup

…pache#14420) * feat: new endpoint for validating database parameters * Rebase * Remove broken tests

* fix error message icon * lint

…#14419) * intial commit * fix test and add more icons * fix lint

* feat: add generic type to column payload * feat: add generic type to column payload * xit flaky test

* chore: bumping echarts plugin * feat: Upgrading to new treemap * bump @superset-ui/plugin-chart-echarts 0.17.47 Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com> Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>

…#14618) * Add getTooltipTitle * Fix tests

Co-authored-by: John Bodley <john.bodley@airbnb.com>

* fix popover * addd tabs default css * fix lint * fix tests * address comments * lint fix * fix test * lint

…pache#14637)

* perf: memoize db_engine_spec in sqla table classes * remove extended cypress timeouts

…pache#14120) * fix: do not render favorite favStar and filter for anonymous user * fix: prevent anonymous user to trigger the favstar view route * fix: lint over previous commit * fix: linter follow-up

Co-authored-by: John Bodley <john.bodley@airbnb.com>

…apache#15172) * fix(dashboard): Prevent rerendering View Query modal on window resize * Fix lint

* Convert TableElement to typescript * Change type names to better match naming conventions in other files * Fix import order and update tests on TableElement * Remove defaultProps * Destructure the props * Use Rest and Spread syntax to condense props destructuring * Fix TypeScript errors and add comment to explain antd props and types weirdness * Remove comment, add consistency with other files, and use method chaining to make more concise Co-authored-by: Corbin Robb <corbin@Corbins-MacBook-Pro.local>

villebro

Minor perf related suggestion, other than that LGTM!

villebro · 2021-06-16T16:30:39Z

+    bind = op.get_bind()
+    session = db.Session(bind=bind)
+
+    dashboards = session.query(Dashboard).all()


Here you could restrict the query so that you only get the relevant dashboards. Something like

dashboards = session.query(Dashboard).filter(or_( Dashboard.json_metadata.like('%"show_native_filters"%'), Dashboard.json_metadata.like('%"native_filter_configuration"%'), )).all()

betodealmeida · 2021-06-16T16:27:24Z

+    if is_feature_enabled("DASHBOARD_NATIVE_FILTERS"):
+        print(f"DASHBOARD_NATIVE_FILTERS feature is enabled. skip.")
+        return


Having migrations depend on feature flags seems brittle to me. What if I have the feature enabled, run this migration, and later decide to turn the feature off?

i can not handle this back and forth case. I assume there is very little number of users will do this.

betodealmeida · 2021-06-16T16:32:02Z

+            json_metadata.pop("show_native_filters", None)
+            json_metadata.pop("native_filter_configuration", None)


Why do we need to remove these attributes?

Can we make Superset ignore these attributes if DASHBOARD_NATIVE_FILTERS is not enabled instead?

No. when we start to convert dashboard filter_box to filter components, I rely on native_filter_configuration to decide if this dashboard needs to convert, or already converted.

Sorry, I don't have all the context here and I'm just trying to understand.

Shouldn't we do the conversion of dashboard filter box to filter components only if the feature flag is enabled?

There are 2 types of users: already used native filters, and user never used native filters. During migration period, i will run some logic in the front-end, to decide if we want to convert filter_box into filter components.

if dashboard not have native_filter_configuration: start JS convert, user will review this change, they decide save this convert or not. if there is an issue in the convert, user can report and we need to fix.

if dashboard has native_filter_configuration: this means feature is enabled, user may have mixed filter_box and filter components. In this case, DO NOT run JS convert.

In short, "JS convert", or later we will have "db migration auto convert", will only apply for dashboards without native_filter_configuration. there is no automatic convert for dashboards with mixed filter_box and filter components. For all of above cases, user needs to manually delete filter_box from their dashboard.

Please see SIP-64 for convert logic.

codecov · 2021-06-16T18:27:36Z

Codecov Report

Merging #15197 (2ffc596) into master (ffdbcbd) will decrease coverage by 0.38%.
The diff coverage is 36.50%.

❗ Current head 2ffc596 differs from pull request most recent head e94a812. Consider uploading reports for the commit e94a812 to get more accurate results

@@            Coverage Diff             @@
##           master   #15197      +/-   ##
==========================================
- Coverage   77.47%   77.09%   -0.39%     
==========================================
  Files         969      971       +2     
  Lines       50021    50236     +215     
  Branches     6432     6494      +62     
==========================================
- Hits        38756    38729      -27     
- Misses      11060    11302     +242     
  Partials      205      205

Flag	Coverage Δ
hive	`81.42% <96.29%> (+<0.01%)`	⬆️
mysql	`81.70% <96.29%> (+<0.01%)`	⬆️
postgres	`81.71% <96.29%> (+<0.01%)`	⬆️
presto	`?`
python	`82.09% <96.29%> (-0.14%)`	⬇️
sqlite	`81.34% <96.29%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...rset-frontend/src/components/Select/AntdSelect.tsx	`0.00% <0.00%> (ø)`
...rontend/src/components/Select/DeprecatedSelect.tsx	`86.73% <ø> (ø)`
superset-frontend/src/components/Select/styles.tsx	`84.72% <ø> (ø)`
superset-frontend/src/components/Select/utils.ts	`66.66% <0.00%> (-27.46%)`	⬇️
superset-frontend/src/components/index.ts	`0.00% <0.00%> (ø)`
...ashboard/components/gridComponents/ChartHolder.jsx	`74.71% <ø> (ø)`
...veFilters/FilterBar/FilterControls/FilterValue.tsx	`68.81% <ø> (-1.08%)`	⬇️
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	`25.00% <ø> (ø)`
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx	`95.29% <ø> (ø)`
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`68.21% <18.42%> (-0.63%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffdbcbd...e94a812. Read the comment docs.

robdiciuccio · 2021-06-16T21:37:06Z

I agree with @betodealmeida that conditional migrations based on feature flags may lead to unintended results.

If the DASHBOARD_NATIVE_FILTERS feature flag is disabled, what is the impact of having show_native_filters and native_filter_configuration values set? Shouldn't they just be ignored?

graceguo-supercat · 2021-06-16T21:46:22Z

I agree with @betodealmeida that conditional migrations based on feature flags may lead to unintended results.

If the DASHBOARD_NATIVE_FILTERS feature flag is disabled, what is the impact of having show_native_filters and native_filter_configuration values set? Shouldn't they just be ignored?

Please see my reply to Beto. It will impact the decision after we start migrating filter_box to filter component.

villebro · 2021-06-17T16:28:33Z

The concerns that have been raised are valid in the sense that if someone who has been using NF for a long time happens to turn off the FF during the migration, it might end up wiping out the existing filter configurations. To mitigate this and remove the dependency on the feature flag, I propose the following:

If, and only if, the following properties are set to the following:

"show_native_filters": true, 
"native_filter_configuration": [],

the properties are removed. Otherwise we don't touch them.

This won't have any effect on dashboards for envs that have the native filters feature flag enabled, nor will it impact envs that have it disabled.

graceguo-supercat · 2021-06-17T16:40:24Z

@betodealmeida @robdiciuccio Do you feel more comfortable with @villebro's suggestion?

Airbnb team also consider running this PR in airbnb's release branch, so that we won't impact any open source users. But if any corporation happened to have a deployment before #15146, they probably can't have auto convert filter_box to filter components. I assume this case is very small.

graceguo-supercat · 2021-06-17T18:51:32Z

i discussed this solution with @villebro, i decided to keep this change inside airbnb.

for open source users, if they happened to have a deployment during bad time, they can contact us to get a solution.

geido and others added 30 commits May 11, 2021 10:27

fix: Column name and icons alignment in the Datasource Panel (Explore) (

3a4536a

apache#14551) * Fix column name alignment * Fix space wrap

fix typo on CONTRIBUTING.md (apache#14536)

cd899c5

style: adding padding to the sides of badges, so they look less claus…

eaeb8f7

…trophobic (apache#14569)

fix: Menu does not appear on scroll in Dashboard (apache#14566)

b960843

* Fix menu * Fix test

fix: bring back dashboard perf logger (apache#14584)

ada3809

* fix: bring back dashboard perf logger * add test

fix: dashboard side actions (apache#14587)

bfbf767

* fix dashboard side actions * lint being lint

fix: properly keep state on queryEditorSetSql on tabstateview PUT (ap…

0d240c3

…ache#14579) * send queryid up with setSQL * pass latest query id * update * cleanup

use pre-commit instead (apache#14581)

76a06a7

feat(native-filters): add sort metric to select (apache#14590)

f1c32b9

feat: API endpoint to validate databases using separate parameters (a…

31f406a

…pache#14420) * feat: new endpoint for validating database parameters * Rebase * Remove broken tests

fix: error icon spacing in explore (apache#14597)

5f7722c

* fix error message icon * lint

refactor: change xsm Icons and dnd icons to new dynamic icons (apache…

3eef38f

…#14419) * intial commit * fix test and add more icons * fix lint

refactor: Bootstrap to AntD - Form - iteration 4 (apache#14546)

331eb10

fix: don't show busted label for unknown data types (apache#14585)

ad699e8

feat: add generic type to column payload (apache#14547)

3f6bd1e

* feat: add generic type to column payload * feat: add generic type to column payload * xit flaky test

fix(explore): Filters Tooltip is not showing the full content (apache…

1e60148

…#14618) * Add getTooltipTitle * Fix tests

change name to Basic instead of Base (apache#14625)

3a81e6a

fix: Fixes apache#12672 (apache#14525)

d31958c

refactor: Bootstrap to AntD - NavDropdown (apache#14557)

e4d2424

fix: Use simplejson for Flask JSON encoding (apache#14627)

2bd0b62

Co-authored-by: John Bodley <john.bodley@airbnb.com>

fix: fix adhocpopovers tab animate. (apache#14478)

e4e23ea

* fix popover * addd tabs default css * fix lint * fix tests * address comments * lint fix * fix test * lint

fix: Removing specific column widths, letting things flex naturally. (a…

bf90885

…pache#14637)

perf: memoize db_engine_spec in database (apache#14638)

97c9e37

* perf: memoize db_engine_spec in sqla table classes * remove extended cypress timeouts

fix: do not render favorite favStars and filters for anonymous user (a…

74473e2

…pache#14120) * fix: do not render favorite favStar and filter for anonymous user * fix: prevent anonymous user to trigger the favstar view route * fix: lint over previous commit * fix: linter follow-up

chore: Bump pip-compile-multi (apache#14633)

c55418d

Co-authored-by: John Bodley <john.bodley@airbnb.com>

Fix class name (apache#14609)

3466cb2

fix: Fixes top level tabs and automatic scroll (apache#14624)

9cb4a46

kgabryje and others added 8 commits June 15, 2021 13:16

fix(dashboard): Prevent rerendering View Query modal on window resize (…

5316dc8

…apache#15172) * fix(dashboard): Prevent rerendering View Query modal on window resize * Fix lint

fix: Presto postgres test (apache#15163)

75018bf

fix: use npm v7 in docker compose (apache#15160)

7b87eb1

add another wait for chart element (apache#15108)

7dc0cee

fix space (apache#15175)

d30f3c8

fix: ignore errors in GetLog (apache#15181)

ffdbcbd

migration: fix dashboard json_metadata when native filter is not enabled

9091403

graceguo-supercat requested a review from a team as a code owner June 16, 2021 16:23

pull-request-size Bot added the size/M label Jun 16, 2021

graceguo-supercat requested a review from villebro June 16, 2021 16:28

villebro reviewed Jun 16, 2021

View reviewed changes

betodealmeida reviewed Jun 16, 2021

View reviewed changes

graceguo-supercat requested a review from betodealmeida June 16, 2021 16:43

fix comment

0549f40

graceguo-supercat requested a review from villebro June 16, 2021 17:01

graceguo-supercat force-pushed the gg-FixNativeFilterMetadata branch from 5bc8646 to 8c93633 Compare June 16, 2021 21:17

fix ci and filename

e94a812

graceguo-supercat force-pushed the gg-FixNativeFilterMetadata branch from 8c93633 to e94a812 Compare June 16, 2021 21:44

robdiciuccio requested a review from suddjian June 16, 2021 22:49

graceguo-supercat changed the base branch from master to airbnb June 17, 2021 18:51

pull-request-size Bot added size/XXL and removed size/M labels Jun 17, 2021

graceguo-supercat closed this Jun 17, 2021

		json_metadata.pop("show_native_filters", None)
		json_metadata.pop("native_filter_configuration", None)

Conversation

graceguo-supercat commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

villebro Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

betodealmeida Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betodealmeida Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

betodealmeida Jun 16, 2021

Choose a reason for hiding this comment

Uh oh!

graceguo-supercat Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robdiciuccio commented Jun 16, 2021

Uh oh!

graceguo-supercat commented Jun 16, 2021

Uh oh!

villebro commented Jun 17, 2021

Uh oh!

graceguo-supercat commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graceguo-supercat commented Jun 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

graceguo-supercat commented Jun 16, 2021 •

edited

Loading

graceguo-supercat Jun 16, 2021 •

edited

Loading

graceguo-supercat Jun 16, 2021 •

edited

Loading

graceguo-supercat Jun 16, 2021 •

edited

Loading

codecov Bot commented Jun 16, 2021 •

edited

Loading

graceguo-supercat commented Jun 17, 2021 •

edited

Loading