codeant-ai-for-open-source[bot] commented on code in PR #41176:
URL: https://github.com/apache/superset/pull/41176#discussion_r3500174614
##########
superset/commands/importers/v1/assets.py:
##########
@@ -165,23 +164,33 @@ def _import( # noqa: C901
dashboard = import_dashboard(config, overwrite=overwrite)
# set ref in the dashboard_slices table
- dashboard_chart_ids: list[dict[str, int]] = []
+ # Use ORM-level reassignment instead of Core
+ # delete()/insert() so SQLAlchemy-Continuum's M2M tracker
+ # sees per-row changes through the ORM. Bulk DML via Core
+ # would emit a malformed INSERT into
+ # ``dashboard_slices_version`` (missing the composite-PK
+ # columns) — see the parallel rewrite in
+ # ``DatasetDAO.update_columns`` and the test-factory's
+ # ``delete_dashboard_slices_associations`` for the same
+ # reason.
+ slice_ids: list[int] = []
for uuid in find_chart_uuids(config["position"]):
if uuid not in chart_ids:
break
- chart_id = chart_ids[uuid]
- dashboard_chart_id = {
- "dashboard_id": dashboard.id,
- "slice_id": chart_id,
- }
- dashboard_chart_ids.append(dashboard_chart_id)
+ slice_ids.append(chart_ids[uuid])
- db.session.execute(
- delete(dashboard_slices).where(
- dashboard_slices.c.dashboard_id == dashboard.id
- )
+ dashboard.slices = (
+
db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()
+ if slice_ids
+ else []
)
Review Comment:
**Suggestion:** The chart-membership rebuild aborts on the first missing
chart UUID, then replaces the whole dashboard relationship with only the IDs
collected so far. Because the UUID source is a set, iteration order is
non-deterministic, so dashboards can end up with randomly incomplete chart
links across runs. Skip unknown UUIDs instead of terminating the loop before
assigning `dashboard.slices`. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard imports lose charts with unresolved UUIDs.
- ⚠️ Dashboard-chart links depend on set iteration ordering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the assets import API endpoint `POST /api/v1/assets/import/`
implemented by
`ImportExportRestApi.import_` in `superset/importexport/api.py:95-104`,
uploading a ZIP
bundle in the `bundle` form field.
2. In that ZIP, include a dashboard YAML `dashboards/multi_chart.yaml` whose
`position`
JSON defines at least two `CHART` nodes with `meta.uuid` values `UUID_A` and
`UUID_B`, but
only `UUID_A` has a corresponding chart file under `charts/*.yaml`; `UUID_B`
refers to a
chart that is not in the bundle, so it is absent from the `chart_ids` map
built while
importing charts in `ImportAssetsCommand._import` at
`superset/commands/importers/v1/assets.py:143-152`.
3. When `_import` processes dashboards in
`superset/commands/importers/v1/assets.py:160-186`, it calls
`update_id_refs` then
`import_dashboard`, then iterates `for uuid in
find_chart_uuids(config["position"]):`
(lines 176-180), where `find_chart_uuids` in
`superset/commands/dashboard/importers/v1/utils.py:9-10` returns a
`set[str]` of UUIDs; on
the first UUID not present in `chart_ids` the loop executes `break` at line
179, stopping
collection of `slice_ids`.
4. Because iteration over the `set` from `find_chart_uuids` is not ordered
by the
dashboard layout, the unknown UUID may be yielded first or interleaved; the
`break` at
`assets.py:179` can leave `slice_ids` empty or missing some valid chart IDs
before
`dashboard.slices` is assigned from `slice_ids` at `assets.py:182-186`,
resulting in
imported dashboards with incomplete or empty chart memberships even though
the HTTP
response is 200 OK.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ff2046933dd945f38637b1edefa7e367&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=ff2046933dd945f38637b1edefa7e367&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/commands/importers/v1/assets.py
**Line:** 176:186
**Comment:**
*Incorrect Condition Logic: The chart-membership rebuild aborts on the
first missing chart UUID, then replaces the whole dashboard relationship with
only the IDs collected so far. Because the UUID source is a set, iteration
order is non-deterministic, so dashboards can end up with randomly incomplete
chart links across runs. Skip unknown UUIDs instead of terminating the loop
before assigning `dashboard.slices`.
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%2F41176&comment_hash=17f76271b192752807efc0529b90aa01dc99d24e8f4ebc90589ede0e025b9274&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=17f76271b192752807efc0529b90aa01dc99d24e8f4ebc90589ede0e025b9274&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]
