eschutho opened a new pull request, #44577:
URL: https://github.com/apache/superset/pull/44577
### SUMMARY
`GET /api/v1/dashboard/<id>` returns a 500 (`AttributeError: 'str' object
has no attribute 'values'`) for a dashboard whose `position_json` is valid JSON
but not an object, for example a JSON-encoded string.
**Root cause.** `Dashboard.position` returns whatever
`json.loads(position_json)` produces, even though it is annotated `dict[str,
Any]`. #43575 already fixed this for `Dashboard.tabs`: it checks
`isinstance(self.position, dict)`, logs `"layout is not a mapping"` and returns
no tabs. `superset/dashboards/filter_scope.py` came from #43252, which was
merged before that fix, so it never got the same guard. It reads
`dashboard.position` through `derive_metadata_scopes()` → `derive_scopes()` →
`build_chart_layout_items()`, and the last of those calls
`position_data.values()` without checking the type.
`derive_json_metadata()` already passes non-object `json_metadata` through
unchanged ("reading a dashboard is not the place to start rejecting documents
that have always been served as-is"). It just didn't apply the same tolerance
to the layout.
**Fix.** One guard in `build_chart_layout_items()`, the function that
crashes. A layout that isn't a mapping places no charts, so the function logs a
warning and returns an empty layout-items map. Every production path reaches
`dashboard.position` through this function:
- `DashboardRestApi.get` → `derive_json_metadata` → `derive_metadata_scopes`
→ `derive_scopes` → `build_chart_layout_items`
- `DashboardDAO.get_native_filter_configuration` → `derive_metadata_scopes`
→ …
- `DashboardDAO.update_native_filters_config` → `derive_metadata_scopes` → …
So all three are covered. The function has no dashboard in scope, so the
warning doesn't include a dashboard id. Adding one would mean passing it
through `derive_scopes`, and I kept the diff to a single guard.
### Tradeoffs
- A dashboard with malformed `position_json` now gets **empty** derived
`chartsInScope` / `tabsInScope` caches instead of a 500, and
`chart_configuration` entries for charts are dropped because no chart is on the
layout. That matches what the layout describes (no charts), and it is the same
graceful-degradation choice `Dashboard.tabs` made in #43575 for this case. The
dashboard client can't render a non-mapping layout either, so no working view
loses anything.
- The warning doesn't name the dashboard (see above).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (backend only)
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/dashboards/filter_scope_test.py
```
The two new tests cover `build_chart_layout_items` / `derive_scopes` with
string, list and `None` layouts, and the `derive_json_metadata` path used by
the GET handler. Without the fix both fail with `AttributeError: 'str' object
has no attribute 'values'` at `filter_scope.py:62`. With the fix:
```
tests/unit_tests/dashboards/filter_scope_test.py 11 passed
tests/unit_tests/dao/dashboard_test.py
tests/unit_tests/models/dashboard_test.py 26 passed
```
`pre-commit run` passes on the changed files (ruff, ruff-format, mypy,
pylint).
Manual: set a dashboard's `position_json` to `"\"not a layout\""` in the
metadata DB and call `GET /api/v1/dashboard/<id>`. Before the fix it returns
500; after, it returns 200 with empty derived scopes.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
Possible follow-up: `Dashboard.position` itself could normalize non-mapping
layouts to `{}` so every consumer is covered. That would change a widely used
property, so it's left out of this fix.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
