codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3645754090
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -185,20 +199,35 @@ def upgrade_slice(cls, slc: Slice) -> None:
def downgrade_slice(cls, slc: Slice) -> None:
try:
form_data = try_load_json(slc.params)
- if "viz_type" in (
+ if "viz_type" not in (
form_data_bak := form_data.get(FORM_DATA_BAK_FIELD_NAME, {})
):
- slc.params = json.dumps(form_data_bak)
- slc.viz_type = form_data_bak.get("viz_type")
- query_context = try_load_json(slc.query_context)
- queries_bak = form_data.get(QUERIES_BAK_FIELD_NAME, {})
- if queries_bak:
- query_context["queries"] = queries_bak
- if "form_data" in query_context:
- query_context["form_data"] = form_data_bak
- slc.query_context = json.dumps(query_context)
- else:
- slc.query_context = None
+ return
+
+ new_params = json.dumps(form_data_bak)
+ new_viz_type = form_data_bak.get("viz_type")
+
+ # Sentinel so a "leave query_context untouched" branch below is
+ # distinguishable from "explicitly set it to None".
+ unchanged = object()
+ new_query_context: Any = unchanged
+
+ query_context = try_load_json(slc.query_context)
+ queries_bak = form_data.get(QUERIES_BAK_FIELD_NAME, {})
+ if queries_bak:
+ query_context["queries"] = queries_bak
+ if "form_data" in query_context:
+ query_context["form_data"] = form_data_bak
+ new_query_context = json.dumps(query_context)
Review Comment:
**Suggestion:** When backup queries exist but `query_context` has no
`form_data` key, the code updates `query_context["queries"]` in memory but
never serializes it back, so the restored queries are silently dropped.
Serialize and assign `new_query_context` whenever queries are restored,
regardless of whether `form_data` is present. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Restored queries not persisted when form_data missing in context.
- ⚠️ Downgraded charts use migrated queries, not original ones.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A slice row in `slices` has a `query_context` JSON that contains a
`"queries"` field
but no `"form_data"` key (older records or external modifications), and its
`viz_type`
matches a migration class's `source_viz_type`, so it is processed by
`MigrateViz.upgrade_slice` at
`superset/migrations/shared/migrate_viz/base.py:150-183`.
2. During upgrade, at lines 172-178, `query_context` is loaded and
`queries_bak =
copy.deepcopy(query_context["queries"])` captures the existing queries into
the backup.
Because `"form_data"` is not in `query_context`, the branch at line 173 does
not add it,
so the upgraded `query_context` still lacks `"form_data"` when serialized
and stored in
`slc.query_context`.
3. On downgrade, `MigrateViz.downgrade_slice`
(`superset/migrations/shared/migrate_viz/base.py:199-231`) is invoked for
that slice. At
line 216, `queries_bak = form_data.get(QUERIES_BAK_FIELD_NAME, {})` reads
the saved backup
list (truthy). At line 217, `if queries_bak:` passes, and line 218 executes
`query_context["queries"] = queries_bak`, correctly restoring the in-memory
`query_context["queries"]` field.
4. Because `"form_data"` is absent, the `if "form_data" in query_context:`
guard at line
219 fails, and `new_query_context` remains the sentinel `unchanged` from
line 212. The
subsequent check at lines 229-230 (`if new_query_context is not unchanged:`)
prevents
writing back to `slc.query_context`, so the restored `queries` are never
persisted; the
slice keeps the migrated (wrong) `queries` despite having a valid backup.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4e9dece06cdf4944b98c6e86723ab7fb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4e9dece06cdf4944b98c6e86723ab7fb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/shared/migrate_viz/base.py
**Line:** 217:221
**Comment:**
*Logic Error: When backup queries exist but `query_context` has no
`form_data` key, the code updates `query_context["queries"]` in memory but
never serializes it back, so the restored queries are silently dropped.
Serialize and assign `new_query_context` whenever queries are restored,
regardless of whether `form_data` is present.
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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=0612dc5f6a42c92957ca996794e27f5b761e64d9f91d9c320f8152529208461f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=0612dc5f6a42c92957ca996794e27f5b761e64d9f91d9c320f8152529208461f&reaction=dislike'>👎</a>
--
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]
