fix(chart): load viz_type from request params when creating chart via API#40536
Conversation
…mmand - Move json import to module level (from superset.utils import json) - Remove redundant try/except that only re-raised the exception - Add isinstance(params, dict) guard to prevent TypeError when params JSON is not an object (e.g. a list or string) - Fix single-quote style to match project conventions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️ Please read our New Contributor Welcome & Expectations guide. We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register. |
rusackas
added
the
merge-if-green
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if isinstance(params, dict) and "viz_type" in params: | ||
| self._properties["viz_type"] = params["viz_type"] |
There was a problem hiding this comment.
Suggestion: This unconditionally overwrites a user-supplied top-level chart type whenever params contains viz_type. If both are present but differ, the explicit API field is silently ignored and the chart can be created with the wrong visualization type. Only copy from params when the top-level field is missing. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ POST /api/v1/chart persists unexpected viz_type from params.
- ⚠️ Programmatic chart creation can silently ignore explicit viz_type.
- ⚠️ Generated charts may render with inconsistent visualization type.Steps of Reproduction ✅
1. Send a POST request to the chart creation API `POST /api/v1/chart/` handled by
`ChartRestApi.post()` in `superset/charts/api.py` (around lines 359–366 per `add` method
snippet).
2. In the JSON body, provide a valid top-level `viz_type`, e.g. `"viz_type": "bar"`, and a
`params` string whose JSON contains a conflicting `viz_type`, e.g. `"params":
"{\"viz_type\": \"line\"}"`. This request payload is loaded by `ChartPostSchema` in
`superset/charts/schemas.py:28-53`, which validates the top-level `viz_type` as a string
and `params` as JSON.
3. After schema validation, `ChartRestApi.post()` calls `CreateChartCommand(item).run()`
(see `superset/charts/api.py` snippet lines 20-27). Inside `CreateChartCommand.__init__`
in `superset/commands/chart/create.py:44-50`, `self._properties` is set to the validated
data, then `params_str` is JSON-decoded and, because the decoded dict has `"viz_type":
"line"`, the unconditional assignment at lines 49-50 overwrites the already-validated
top-level `"viz_type": "bar"` with `"line"`.
4. `CreateChartCommand.run()` then calls `ChartDAO.create(attributes=self._properties)`
(create.py:52-57), which persists a `Slice` model (`superset/models/slice.py:68-82`) with
`viz_type="line"`. Fetching or rendering this chart later (e.g. via explore or GET
endpoints) shows `viz_type="line"`, demonstrating that the explicit API field was silently
ignored whenever `params` also contained `viz_type`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/chart/create.py
**Line:** 49:50
**Comment:**
*Incorrect Condition Logic: This unconditionally overwrites a user-supplied top-level chart type whenever `params` contains `viz_type`. If both are present but differ, the explicit API field is silently ignored and the chart can be created with the wrong visualization type. Only copy from `params` when the top-level field is missing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Good catch — fixed in bae380f. Switched from direct assignment to setdefault() so the top-level viz_type wins when both it and a viz_type inside params are supplied.
|
The PR comment file contains only the header row and no additional data. I cannot analyze or provide information about specific comments or suggestions in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40536 +/- ##
==========================================
- Coverage 64.00% 63.99% -0.02%
==========================================
Files 2620 2637 +17
Lines 140974 141658 +684
Branches 32527 32567 +40
==========================================
+ Hits 90236 90652 +416
- Misses 49187 49451 +264
- Partials 1551 1555 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #21ab12
Actionable Suggestions - 1
-
superset/commands/chart/create.py - 1
- Uncaught JSON parse exception · Line 47-48
Review Details
-
Files reviewed - 1 · Commit Range:
ffde065..8328023- superset/commands/chart/create.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| if params_str := self._properties.get("params"): | ||
| params = json.loads(params_str) |
There was a problem hiding this comment.
Wrap json.loads(params_str) in a try-except block to catch JSONDecodeError and convert it into a ChartInvalidError, ensuring malformed JSON is handled gracefully.
Code Review Run #21ab12
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
The marshmallow schema for chart creation (superset/charts/schemas.py) already applies validate=utils.validate_json to the params field before the request reaches CreateChartCommand.__init__, so json.loads here cannot raise JSONDecodeError in normal operation. Adding a catch would be dead code — declining this one.
There was a problem hiding this comment.
The suggestion to add a try-except block around json.loads(params_str) is valid and should be applied. The suggestion addresses a potential edge case where params_str might be malformed JSON, which could cause a runtime error. Even though the schema already validates params, adding a try-except block ensures robustness in case of unexpected input. This is a defensive programming practice that improves code reliability.
There was a problem hiding this comment.
Standing by rusackas's assessment — the Marshmallow schema in superset/charts/schemas.py applies validate=utils.validate_json to params before this command is ever constructed, so json.loads here can only fail if something bypasses the schema layer. Adding a try-except would be dead code that obscures that invariant. Declining.
There was a problem hiding this comment.
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
There was a problem hiding this comment.
The /resolve command is not supported on inline comments. Please use it in a top-level comment instead.
There was a problem hiding this comment.
Closing this thread. The Marshmallow schema in superset/charts/schemas.py applies validate=utils.validate_json to the params field before CreateChartCommand is ever constructed, so json.loads(params_str) cannot receive malformed JSON. No try/except needed. Bito's subsequent replies are loop artifacts from /resolve not being supported on inline comments.
There was a problem hiding this comment.
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
There was a problem hiding this comment.
Thread closed. The Marshmallow schema in superset/charts/schemas.py applies validate=utils.validate_json to the params field before CreateChartCommand is ever constructed, so json.loads(params_str) cannot raise JSONDecodeError here. No try/except is needed. Bito's replies are loop artifacts from /resolve not being supported on inline comments.
There was a problem hiding this comment.
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
If a caller supplies viz_type at the top level and also embeds it in the params JSON, the explicit field should take precedence. Switching from a direct assignment to setdefault() ensures the params value is only used when no top-level viz_type was provided. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #bf6f4eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
3 participants
SUMMARY
Adopts and fixes #31929 (originally by @hdahme, whose fork was subsequently archived).
When creating a chart via the API, the
viz_typefield was not being extracted from the JSON-encodedparamsstring. This meant that charts created programmatically withparamsset (but without an explicit top-levelviz_type) ended up with a blank/wrong chart type.Changes in
superset/commands/chart/create.py:CreateChartCommand.__init__, ifparamsis present, parse it and copyviz_typeinto the top-level properties so the model gets the correct chart type.import jsonto module level usingfrom superset.utils import json(project convention).try/except JSONDecodeError as ex: raise exantipattern — let the exception propagate naturally.isinstance(params, dict)guard to preventTypeErrorwhen theparamsJSON is not an object (e.g. an array or scalar).TESTING INSTRUCTIONS
pytest tests/integration_tests/charts/commands_tests.py -k "viz_type" -vThe integration test at
tests/integration_tests/charts/commands_tests.pyalready covers the expected behaviour: a chart created withparams={"viz_type": "new_viz_type"}should havechart.viz_type == "new_viz_type".ADDITIONAL INFORMATION
🤖 Generated with Claude Code