Repository navigation
fix(dashboard): stop list-valued order_by_cols from 500-ing the datasets API - #45700
Dmitry-Kucher wants to merge 3 commits into
Conversation
…ets API
GET /api/v1/dashboard/<id>/datasets fails for any dashboard holding a chart
whose order_by_cols contains a raw list and no usable query_context, which
leaves every widget on that dashboard without datasource metadata:
File "superset/connectors/sqla/models.py", in data_for_slices
column_names.update(_columns)
TypeError: unhashable type: 'list'
order_by_cols holds [column, ascending] pairs. They are canonically stored as
JSON strings ('["col", false]'), but older writers persisted raw lists. The
form_data fallback in data_for_slices passed anything non-adhoc straight
through, so a raw list reached set.update() and raised.
superset.common.form_data_query_context already tolerates both encodings when
building orderby, which is why chart data still renders and only the
dashboard-level datasets call fails. Apply the same tolerance through a
_column_param_name() helper that accepts either encoding, takes the column half
of the pair, and skips anything unrecognised instead of failing the request.
This also fixes the canonical encoding: a JSON string used to be added to
column_names verbatim, so it never matched a real column and the ordering
column was dropped from the trimmed metadata. It now resolves to the column.
Co-Authored-By: AIST code-assistant https://virtuman.com
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45700 +/- ##
=======================================
Coverage 85.03% 85.03%
=======================================
Files 3080 3080
Lines 209437 209451 +14
Branches 49385 49389 +4
=======================================
+ Hits 178095 178111 +16
+ Misses 27960 27959 -1
+ Partials 3382 3381 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
LGTM. Verified the raw-list order_by_cols case fails on master and is fixed here, the JSON-string encoding now resolves to the sort column, and the other column params behave the same as before. Thanks for the fix!
Bring in upstream CI fixes and rerun checks after the previous GitHub API rate-limit failures. SQLA models suite: 123 passed. Pre-commit passed for both PR files.
|
@gabotorresruiz This PR has approval from rebenitez1802 and is updated with master. CI is green, and all 123 SQLA model unit tests pass locally. Could you take a look when you have a chance? Thanks! |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @Dmitry-Kucher, and sorry for the wait. I ran models_test.py at 4de1947 (123 of 123), then put the old comprehension back: 6 of the 7 new cases fail, and the seventh (malformed JSON) is already tolerated on master. I also checked that bracketed names such as [2024] and [Revenue] in groupby still reach the trimmed metadata. CI is green. LGTM!
SUMMARY
GET /api/v1/dashboard/<id>/datasetsfails for a dashboard that holds a chart whoseorder_by_colscontains a raw list, when that chart has no usablequery_context. Every widget on the dashboard is then left without datasource metadata:order_by_colsholds[column, ascending]pairs. They are canonically stored as JSON strings ('["col", false]'), but older writers persisted raw lists, and such charts exist in real deployments. Theform_datafallback indata_for_slicespassed every non-adhoc entry straight through, so a raw list reachedset.update().superset.common.form_data_query_contextalready accepts both encodings when it buildsorderby, which is why chart data still renders and only the dashboard datasets call fails. This PR applies the same tolerance through a_column_param_name()helper: it accepts either encoding, takes the column half of the pair (which may itself be an adhoc column), and skips anything unrecognised instead of failing the request. A string that isn't valid JSON is still treated as a plain column name, since bracketed names such as[Revenue]are legal.It also fixes the canonical encoding. A JSON string used to be added to
column_namesverbatim, so it never matched a real column, and the ordering column was dropped from the trimmed metadata. It now resolves to the column name.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend).
TESTING INSTRUCTIONS
The parametrized test covers both encodings, an adhoc column as the pair head, non-ASCII column names, and junk entries (non-string head, empty pair, malformed JSON). Most cases fail without the fix.
Manual: pick a table chart with "Sort by" set, set its
params.order_by_colsto a raw list (e.g.[["some_col", false]]) and clear itsquery_context, then open a dashboard containing it. Before: the datasets request fails and the charts don't load. After: the dashboard loads.ADDITIONAL INFORMATION