Skip to content

fix(chart): load viz_type from request params when creating chart via API #40536

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.

Sign up for GitHub

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

Merged
hainenber merged 5 commits into from
May 31, 2026
+8 −0
Merged

fix(chart): load viz_type from request params when creating chart via API #40536

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ffde065
Fixing the API, pulling viz_type out of params properly
hdahme Jan 20, 2025
f3c4f1a
changing remotes"
hdahme Jan 20, 2025
8ca1d4e
Linting and fixing tests
hdahme Jan 21, 2025
8328023
fix(chart): clean up viz_type extraction from params in CreateChartCo…
rusackas May 30, 2026
bae380f
fix(chart): prefer explicit viz_type over params-derived value
rusackas May 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions superset/commands/chart/create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from superset.commands.utils import get_datasource_by_id
from superset.daos.chart import ChartDAO
from superset.daos.dashboard import DashboardDAO
from superset.utils import json
from superset.utils.decorators import on_error, transaction

logger = logging.getLogger(__name__)
Expand All @@ -43,6 +44,13 @@ class CreateChartCommand(CreateMixin, BaseCommand):
def __init__(self, data: dict[str, Any]):
self._properties = data.copy()

if params_str := self._properties.get("params"):
params = json.loads(params_str)
Comment on lines +47 to +48
Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot May 30, 2026

Choose a reason for hiding this comment

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

Uncaught JSON parse exception

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

Copy link
Copy Markdown
Member Author

@rusackas rusackas May 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot May 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

@rusackas rusackas May 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot May 30, 2026

Choose a reason for hiding this comment

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

This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.

Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot May 31, 2026

Choose a reason for hiding this comment

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

The /resolve command is not supported on inline comments. Please use it in a top-level comment instead.

Copy link
Copy Markdown
Member Author

@rusackas rusackas May 31, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot May 31, 2026

Choose a reason for hiding this comment

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

This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.

Copy link
Copy Markdown
Member Author

@rusackas rusackas May 31, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review Bot May 31, 2026

Choose a reason for hiding this comment

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

This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.

if isinstance(params, dict) and "viz_type" in params:
# Only fall back to params when no top-level viz_type was supplied;
# an explicit top-level field takes precedence.
self._properties.setdefault("viz_type", params["viz_type"])

@transaction(on_error=partial(on_error, reraise=ChartCreateFailedError))
def run(self) -> Model:
self.validate()
Expand Down
Loading