fix: store form_data as dict during viz type migration#36680
Conversation
Vitor-Avila
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
|
CodeAnt AI is reviewing your PR. |
codeant-ai-for-open-source
Bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| ): | ||
| try: | ||
| query_context = json.loads(slc.query_context) | ||
| form_data = query_context.get("form_data") |
There was a problem hiding this comment.
Suggestion: The code assumes that the decoded query_context is always a dict and immediately calls .get, but if the column contains valid non-object JSON (e.g. a string or list), json.loads will succeed and .get will raise an AttributeError, causing the migration to skip that slice via the broad except and leave it in a broken state; adding an explicit type check for a dict before accessing .get avoids this runtime type error and ensures only well-formed contexts are processed. [type error]
Severity Level: Minor
| form_data = query_context.get("form_data") | |
| if not isinstance(query_context, dict): | |
| logger.warning( | |
| "Unexpected query_context format for slice %s, skipping", slc.id | |
| ) | |
| continue |
Why it matters? ⭐
The current code assumes json.loads(...) returns a dict and immediately calls .get. If query_context is valid JSON but not an object (e.g. a list or string), calling .get will raise an AttributeError which is then swallowed by the broad except, skipping the row silently. Adding an explicit isinstance(query_context, dict) check is a small, correct defensive fix that prevents that runtime type error and makes the migration behavior explicit.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/migrations/versions/2025-12-16_12-00_f5b5f88d8526_fix_form_data_string_in_query_context.py
**Line:** 84:84
**Comment:**
*Type Error: The code assumes that the decoded `query_context` is always a dict and immediately calls `.get`, but if the column contains valid non-object JSON (e.g. a string or list), `json.loads` will succeed and `.get` will raise an AttributeError, causing the migration to skip that slice via the broad `except` and leave it in a broken state; adding an explicit type check for a dict before accessing `.get` avoids this runtime type error and ensures only well-formed contexts are processed.
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.| # Check if form_data is a non-empty string (the bug) | ||
| if form_data and isinstance(form_data, str): | ||
| try: | ||
| query_context["form_data"] = json.loads(form_data) |
There was a problem hiding this comment.
Suggestion: After parsing form_data from a JSON string, the code directly assigns the result back into query_context["form_data"] without verifying it is a dict, so if the stored string represents a non-object JSON value (e.g. a list or scalar), later consumers expecting a dict will see an invalid type and may fail when constructing a QueryContext; adding a type check to only persist dict-like parsed values prevents this latent runtime type error. [type error]
Severity Level: Minor
| query_context["form_data"] = json.loads(form_data) | |
| parsed_form_data = json.loads(form_data) | |
| if not isinstance(parsed_form_data, dict): | |
| logger.warning( | |
| "Unexpected form_data type for slice %s, skipping", slc.id | |
| ) | |
| continue | |
| query_context["form_data"] = parsed_form_data |
Why it matters? ⭐
The suggestion correctly prevents persisting parsed form_data that isn't a mapping (dict). If the stored string decodes to a list/scalar, downstream code expecting a dict will later fail; skipping such cases (or at least not writing invalid types back) is safer. This is a real correctness improvement for data integrity in the migration.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/migrations/versions/2025-12-16_12-00_f5b5f88d8526_fix_form_data_string_in_query_context.py
**Line:** 89:89
**Comment:**
*Type Error: After parsing `form_data` from a JSON string, the code directly assigns the result back into `query_context["form_data"]` without verifying it is a dict, so if the stored string represents a non-object JSON value (e.g. a list or scalar), later consumers expecting a dict will see an invalid type and may fail when constructing a `QueryContext`; adding a type check to only persist dict-like parsed values prevents this latent runtime type error.
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.
|
CodeAnt AI finished reviewing your PR. |
![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 #6b2fe0
Actionable Suggestions - 1
-
superset/migrations/versions/2025-12-16_12-00_f5b5f88d8526_fix_form_data_string_in_query_context.py - 1
- Blind exception catch with unused noqa · Line 99-99
Review Details
-
Files reviewed - 3 · Commit Range:
2f34292..2f34292- superset/commands/chart/importers/v1/utils.py
- superset/migrations/versions/2025-12-16_12-00_f5b5f88d8526_fix_form_data_string_in_query_context.py
- tests/unit_tests/charts/commands/importers/v1/utils_test.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 Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| logger.warning( | ||
| "Could not parse query_context for slice %s, skipping", slc.id | ||
| ) | ||
| except Exception: # noqa: S110 |
There was a problem hiding this comment.
Avoid catching blind Exception on line 99. Replace with specific exception types (e.g., ValueError, TypeError). The noqa: S110 directive is unused and should be removed.
Code suggestion
Check the AI-generated fix before applying
| except Exception: # noqa: S110 | |
| except (ValueError, TypeError, KeyError): |
Code Review Run #6b2fe0
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
(cherry picked from commit d14f502)
(cherry picked from commit d14f502)
2 participants
User description
SUMMARY
This is a follow up to #24703. During the viz type migration, we were storing the
form_dataas a string, which would then cause issues when loading a dashboard.This PR fixes the issue for new viz type migrations, and also has a DB migration for existing charts in this state.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
String.form.data.bug.before.mov
**After
String.form.data.bug.after.mov
TESTING INSTRUCTIONS
Unit test added. For manual testing:
ADDITIONAL INFORMATION
CodeAnt-AI Description
fix: store form_data as dict during viz type migration
What Changed
Impact
✅ Fewer dashboard load errors after importing legacy charts✅ Fixed corrupted imported charts for affected visualization types✅ Consistent query_context.form_data type across migrations💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.