Skip to content

fix(dashboard): label colors included in explore url#16621

Merged
kgabryje merged 1 commit into
apache:masterfrom
kgabryje:fix/dashboard-explore-url
Sep 8, 2021
Merged

fix(dashboard): label colors included in explore url#16621
kgabryje merged 1 commit into
apache:masterfrom
kgabryje:fix/dashboard-explore-url

Conversation

@kgabryje
Copy link
Copy Markdown
Member

@kgabryje kgabryje commented Sep 7, 2021

SUMMARY

Due to changes from #16444, label_colors from dashboard were being included in chart's URL when user visited explore from dashboard. This PR partially reverts the changes from #16444, while still preserving the perf improvement introduced in that PR

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before - example link of Proportion of Monthly Revenue by Product Line chart from Sales Dashboard (2142 characters):
http://localhost:9000/superset/explore/?form_data=%7B%22viz_type%22%3A%22area%22%2C%22datasource%22%3A%2220__table%22%2C%22slice_id%22%3A96%2C%22url_params%22%3A%7B%7D%2C%22time_range_endpoints%22%3A%5B%22inclusive%22%2C%22exclusive%22%5D%2C%22granularity_sqla%22%3A%22order_date%22%2C%22time_grain_sqla%22%3A%22P1M%22%2C%22time_range%22%3A%22No+filter%22%2C%22metrics%22%3A%5B%7B%22aggregate%22%3A%22SUM%22%2C%22column%22%3A%7B%22column_name%22%3A%22sales%22%2C%22description%22%3Anull%2C%22expression%22%3Anull%2C%22filterable%22%3Atrue%2C%22groupby%22%3Atrue%2C%22id%22%3A917%2C%22is_dttm%22%3Afalse%2C%22optionName%22%3A%22_col_Sales%22%2C%22python_date_format%22%3Anull%2C%22type%22%3A%22DOUBLE+PRECISION%22%2C%22verbose_name%22%3Anull%7D%2C%22expressionType%22%3A%22SIMPLE%22%2C%22hasCustomLabel%22%3Afalse%2C%22isNew%22%3Afalse%2C%22label%22%3A%22%28Sales%29%22%2C%22optionName%22%3A%22metric_3is69ofceho_6d0ezok7ry6%22%2C%22sqlExpression%22%3Anull%7D%5D%2C%22adhoc_filters%22%3A%5B%5D%2C%22groupby%22%3A%5B%22product_line%22%5D%2C%22limit%22%3A100%2C%22order_desc%22%3Atrue%2C%22contribution%22%3Atrue%2C%22row_limit%22%3Anull%2C%22show_brush%22%3A%22auto%22%2C%22show_legend%22%3Atrue%2C%22line_interpolation%22%3A%22linear%22%2C%22stacked_style%22%3A%22stack%22%2C%22color_scheme%22%3A%22bnbColors%22%2C%22label_colors%22%3A%7B%22SUM%28Sales%29%22%3A%22%23ff5a5f%22%2C%22Medium%22%3A%22%237b0051%22%2C%22Small%22%3A%22%23007A87%22%2C%22Large%22%3A%22%2300d1c1%22%2C%22SUM%28SALES%29%22%3A%22%238ce071%22%2C%22Classic+Cars%22%3A%22%23ffb400%22%2C%22Vintage+Cars%22%3A%22%23b4a76c%22%2C%22Motorcycles%22%3A%22%23ff8083%22%2C%22Trucks+and+Buses%22%3A%22%23cc0086%22%2C%22Planes%22%3A%22%2300a1b3%22%2C%22Ships%22%3A%22%2300ffeb%22%2C%22Trains%22%3A%22%23bbedab%22%7D%2C%22rich_tooltip%22%3Atrue%2C%22bottom_margin%22%3A%22auto%22%2C%22x_ticks_layout%22%3A%22auto%22%2C%22x_axis_format%22%3A%22smart_date%22%2C%22y_axis_format%22%3A%22SMART_NUMBER%22%2C%22y_axis_bounds%22%3A%5Bnull%2Cnull%5D%2C%22rolling_type%22%3A%22None%22%2C%22comparison_type%22%3A%22values%22%2C%22annotation_layers%22%3A%5B%5D%2C%22extra_form_data%22%3A%7B%7D%7D

After (1709 characters):
http://localhost:9000/superset/explore/?form_data=%7B%22viz_type%22%3A%22area%22%2C%22datasource%22%3A%2220__table%22%2C%22slice_id%22%3A96%2C%22url_params%22%3A%7B%7D%2C%22time_range_endpoints%22%3A%5B%22inclusive%22%2C%22exclusive%22%5D%2C%22granularity_sqla%22%3A%22order_date%22%2C%22time_grain_sqla%22%3A%22P1M%22%2C%22time_range%22%3A%22No+filter%22%2C%22metrics%22%3A%5B%7B%22aggregate%22%3A%22SUM%22%2C%22column%22%3A%7B%22column_name%22%3A%22sales%22%2C%22description%22%3Anull%2C%22expression%22%3Anull%2C%22filterable%22%3Atrue%2C%22groupby%22%3Atrue%2C%22id%22%3A917%2C%22is_dttm%22%3Afalse%2C%22optionName%22%3A%22_col_Sales%22%2C%22python_date_format%22%3Anull%2C%22type%22%3A%22DOUBLE+PRECISION%22%2C%22verbose_name%22%3Anull%7D%2C%22expressionType%22%3A%22SIMPLE%22%2C%22hasCustomLabel%22%3Afalse%2C%22isNew%22%3Afalse%2C%22label%22%3A%22%28Sales%29%22%2C%22optionName%22%3A%22metric_3is69ofceho_6d0ezok7ry6%22%2C%22sqlExpression%22%3Anull%7D%5D%2C%22adhoc_filters%22%3A%5B%5D%2C%22groupby%22%3A%5B%22product_line%22%5D%2C%22limit%22%3A100%2C%22order_desc%22%3Atrue%2C%22contribution%22%3Atrue%2C%22row_limit%22%3Anull%2C%22show_brush%22%3A%22auto%22%2C%22show_legend%22%3Atrue%2C%22line_interpolation%22%3A%22linear%22%2C%22stacked_style%22%3A%22stack%22%2C%22color_scheme%22%3A%22bnbColors%22%2C%22label_colors%22%3A%7B%7D%2C%22rich_tooltip%22%3Atrue%2C%22bottom_margin%22%3A%22auto%22%2C%22x_ticks_layout%22%3A%22auto%22%2C%22x_axis_format%22%3A%22smart_date%22%2C%22y_axis_format%22%3A%22SMART_NUMBER%22%2C%22y_axis_bounds%22%3A%5Bnull%2Cnull%5D%2C%22rolling_type%22%3A%22None%22%2C%22comparison_type%22%3A%22values%22%2C%22annotation_layers%22%3A%5B%5D%2C%22extra_form_data%22%3A%7B%7D%7D

TESTING INSTRUCTIONS

  1. Go to a dashboard
  2. Click "View chart in Explore"
  3. Verify that label colors are not included in url

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

CC @junlincc @serenajiang

@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 7, 2021

Codecov Report

Merging #16621 (8559380) into master (effcf3b) will decrease coverage by 0.00%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16621      +/-   ##
==========================================
- Coverage   76.69%   76.69%   -0.01%     
==========================================
  Files        1003     1003              
  Lines       53950    53961      +11     
  Branches     7324     7332       +8     
==========================================
+ Hits        41379    41386       +7     
- Misses      12332    12336       +4     
  Partials      239      239              
Flag Coverage Δ
javascript 71.08% <33.33%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset-frontend/src/dashboard/actions/hydrate.js 1.69% <0.00%> (-0.03%) ⬇️
...shboard/util/charts/getFormDataWithExtraFilters.ts 83.87% <50.00%> (ø)
superset-frontend/src/components/Select/Select.tsx 72.65% <0.00%> (+0.19%) ⬆️

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 effcf3b...8559380. Read the comment docs.

Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM - I think we need to do some hacking on related logic to make sure native filters/filter sets don't cause similar blow-ups. So let's try to come up with good conventions for what goes in the URL, what doesn't and how to truncate stuff when it starts getting crowded.

@kgabryje kgabryje merged commit 788c0c3 into apache:master Sep 8, 2021
@graceguo-supercat
Copy link
Copy Markdown

graceguo-supercat commented Sep 8, 2021

The issue for "explore URL" is not really caused by #16444. This line

 <a href={this.props.exploreUrl} rel="noopener noreferrer">

will generate a GET request when user click on it, but browser (and many companies using nginx) has limitation for length of GET request. In airbnb we had a lot of charts with very long parameters, these charts couldn't be open by GET request. #16444 added label colors into url makes things worse, but it is no the root cause. In airbnb user got an error "414 Request-URI Too Large".

Note, originally, there was an onClick event handler for this View chart in explore menu item, but it was lost since #11554. In the event handler, it generate a POST request to open the chart.

Ideally we should avoid opening chart by GET request.

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 First shipped in 1.4.0 labels Mar 13, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
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/S 🚢 1.4.0 First shipped in 1.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants