Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dashboard): increase json_metadata field #24510

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

cnabro
Copy link
Contributor

@cnabro cnabro commented Jun 26, 2023

SUMMARY

  • Data too long for column json_metadata
  • Default column size is 65535, we need to increase column size for saving dashboard.
    • ⚒️ Migration DDL required
  • position_json already changed to utils.MediumText
(MySQLdb._exceptions.DataError) (1406, "Data too long for column 'json_metadata' at row 1")
[SQL: UPDATE dashboards SET changed_on=%s, json_metadata=%s, changed_by_fk=%s WHERE dashboards.id = %s]

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ALTER TABLE dashboards MODIFY json_metadata MEDIUMTEXT;
  • After modifying the column, it works normally.

ADDITIONAL INFORMATION

  • Has associated issue: Dashboard position_json truncates if dashboard gets too large #5532
  • Required feature flags:
  • 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
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@john-bodley
Copy link
Member

john-bodley commented Jun 27, 2023

Thanks @cnabro for the PR. Would you mind also adding a database migration as without the migration this change has no impact. You can take a look at this example regarding how to write said migration—it's virtually a copy-and-paste, though you'll have to update the column names.

@cnabro
Copy link
Contributor Author

cnabro commented Jun 28, 2023

@john-bodley OK, I'll add table migration for dashboards 👍

@cnabro cnabro requested a review from a team as a code owner June 28, 2023 10:55
@cnabro
Copy link
Contributor Author

cnabro commented Jun 29, 2023

I'd fixed alembic multiple head issues.

83e1abbe777f -> bf646a0c1501 (head), json_metadata
ae58e1e58e5c -> 83e1abbe777f, drop access_request
4c5da39be729 -> ae58e1e58e5c, migrate_dual_line_to_mixed_chart
9ba2ce3086e5 -> 4c5da39be729, migrate_treemap_chart
4ea966691069 -> 9ba2ce3086e5, migrate-pivot-table-v1-to-v2

@cnabro
Copy link
Contributor Author

cnabro commented Jul 11, 2023

@john-bodley Is there anything else to do to get a review? 👀

@john-bodley
Copy link
Member

Thanks @cnabro. You'll have to bump the Revies/down_revision since the latest version has changed. It tends to be somewhat of a race to get in line.

@cnabro
Copy link
Contributor Author

cnabro commented Jul 23, 2023

@john-bodley Thank you for guiding! 👍
I'd bumped upstream master branch.

(venv)  ~/private/superset   fix/json_metadata ±  superset db history
a23c6f8b1280 -> bf646a0c1501 (head), json_metadata
863adcf72773 -> a23c6f8b1280, cleanup erroneous parent filter IDs
6d05b0a70c89 -> 863adcf72773, delete obsolete Druid NoSQL slice parameters
f92a3124dd66 -> 6d05b0a70c89, add on delete cascade for owners references
240d23c7f86f -> f92a3124dd66, drop rouge constraints and tables
8e5b0fb85b9a -> 240d23c7f86f, update_tag_model_w_description
6fbe660cac39 -> 8e5b0fb85b9a, Add custom size columns to report schedule
90139bf715e4 -> 6fbe660cac39, add on delete cascade for tables references

@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #24510 (9e909eb) into master (0631a80) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 9e909eb differs from pull request most recent head fecb457. Consider uploading reports for the commit fecb457 to get more accurate results

@@           Coverage Diff           @@
##           master   #24510   +/-   ##
=======================================
  Coverage   68.95%   68.95%           
=======================================
  Files        1902     1902           
  Lines       73939    73939           
  Branches     8176     8176           
=======================================
  Hits        50984    50984           
  Misses      20842    20842           
  Partials     2113     2113           
Flag Coverage Δ
hive 54.14% <100.00%> (ø)
mysql 79.19% <100.00%> (ø)
postgres 79.29% <100.00%> (ø)
presto 54.03% <100.00%> (ø)
python 83.35% <100.00%> (ø)
sqlite 77.86% <100.00%> (ø)
unit 54.96% <100.00%> (ø)

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

Impacted Files Coverage Δ
superset/models/dashboard.py 77.73% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michael-s-molina michael-s-molina merged commit ff7c152 into apache:master Jul 25, 2023
29 checks passed
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 25, 2023
michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023
@mistercrunch mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants