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): 500 error caused by data_for_slices API #16053

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Aug 3, 2021

SUMMARY

Fix a 500 error on the /dashboard/<dashboardId>/datasets API caused by an incorrect nested list comprehension introduced by #15993 .

Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/flask_appbuilder/api/__init__.py", line 85, in wraps
    return f(self, *args, **kwargs)
  File "superset/views/base_api.py", line 85, in wraps
    raise ex
  File "superset/views/base_api.py", line 82, in wraps
    duration, response = time_function(f, self, *args, **kwargs)
  File "superset/utils/core.py", line 1367, in time_function
    response = func(*args, **kwargs)
  File "superset/utils/log.py", line 241, in wrapper
    value = f(*args, **kwargs)
  File "superset/dashboards/api.py", line 334, in get_datasets
    datasets = DashboardDAO.get_datasets_for_dashboard(id_or_slug)
  File "superset/dashboards/dao.py", line 52, in get_datasets_for_dashboard
    return dashboard.datasets_trimmed_for_slices()
  File "/usr/local/lib/python3.8/dist-packages/flask_caching/__init__.py", line 905, in decorated_function
    return f(*args, **kwargs)
  File "superset/models/dashboard.py", line 280, in datasets_trimmed_for_slices
    result.append(datasource.data_for_slices(slices))
  File "superset/connectors/base/models.py", line 309, in data_for_slices
    column_names.update(
TypeError: unhashable type: 'dict'

Updated the test case to cover this case.

In dashboards where a slice has the y metric set (e.g. a NVD3 timeseries chart), the /datasets API will return 500. However, most of the time if will not break the dashboard, since this API is almost only used in FilterBox and filter indicators.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

  1. Test adding a NVD3 Timeseries chart to the dashboard
  2. You may see 500 error in base but not error after the fix

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
  • Introduces new feature or API
  • Removes existing feature or API

@ktmud ktmud added the v1.3 label Aug 3, 2021
for column in utils.get_iterable(form_data.get(param) or [])
for param in COLUMN_FORM_DATA_PARAMS
for column_param in COLUMN_FORM_DATA_PARAMS
for column in utils.get_iterable(form_data.get(column_param) or [])
Copy link
Member Author

Choose a reason for hiding this comment

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

The order of the for loop is incorrect. Normally it will throw an undefined variable error but since param was used in other loops above, the code will still run. Just another example of why too many local variables is not a good thing.

if column_name:
columns.append(column_name)
return columns
return [col for col in map(get_column_name_from_metric, metrics) if col]
Copy link
Member Author

Choose a reason for hiding this comment

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

Bycatch refactor. No real code logic changes.

@ktmud ktmud force-pushed the fix-datasource-column-names branch from a7e5eef to fd3b68b Compare August 3, 2021 20:02
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #16053 (3839452) into master (69c5cd7) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

❗ Current head 3839452 differs from pull request most recent head f0fd52c. Consider uploading reports for the commit f0fd52c to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16053      +/-   ##
==========================================
- Coverage   76.81%   76.66%   -0.15%     
==========================================
  Files         995      995              
  Lines       52869    52864       -5     
  Branches     6712     6712              
==========================================
- Hits        40610    40528      -82     
- Misses      12033    12110      +77     
  Partials      226      226              
Flag Coverage Δ
hive ?
mysql 81.59% <100.00%> (-0.01%) ⬇️
postgres 81.62% <100.00%> (+0.03%) ⬆️
python 81.71% <100.00%> (-0.29%) ⬇️
sqlite 81.22% <100.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
superset/viz.py 56.57% <ø> (ø)
superset/connectors/base/models.py 88.06% <100.00%> (ø)
superset/utils/core.py 88.09% <100.00%> (-0.21%) ⬇️
superset/db_engines/hive.py 0.00% <0.00%> (-82.15%) ⬇️
superset/db_engine_specs/hive.py 69.80% <0.00%> (-16.87%) ⬇️
superset/views/database/mixins.py 81.03% <0.00%> (-1.73%) ⬇️
superset/db_engine_specs/presto.py 83.47% <0.00%> (-1.05%) ⬇️
superset/db_engine_specs/base.py 87.98% <0.00%> (-0.39%) ⬇️
superset/connectors/sqla/models.py 88.08% <0.00%> (-0.24%) ⬇️

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 69c5cd7...f0fd52c. Read the comment docs.

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for quick fix!

@ktmud ktmud force-pushed the fix-datasource-column-names branch from fd3b68b to f0fd52c Compare August 3, 2021 20:37
@ktmud ktmud merged commit 490890d into apache:master Aug 4, 2021
villebro pushed a commit that referenced this pull request Aug 16, 2021
opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@john-bodley john-bodley deleted the fix-datasource-column-names branch June 8, 2022 05:26
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🍒 1.3.0 🍒 1.3.1 🍒 1.3.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 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 v1.3 🍒 1.3.0 🍒 1.3.1 🍒 1.3.2 🚢 1.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants