codeant-ai-for-open-source[bot] commented on code in PR #40957:
URL: https://github.com/apache/superset/pull/40957#discussion_r3493579726
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -152,57 +188,123 @@ def _apply_field_updates(dashboard: Any, request:
UpdateDashboardRequest) -> lis
dashboard.position_json = json.dumps(request.position_json)
changed.append("position_json")
- if request.json_metadata_overrides is not None:
- dashboard.json_metadata = _merge_json_metadata(
- dashboard, request.json_metadata_overrides
- )
+ metadata_overrides: dict[str, Any] = _collect_metadata_overrides(request)
+ if metadata_overrides:
+ dashboard.json_metadata = _merge_json_metadata(dashboard,
metadata_overrides)
changed.append("json_metadata")
if request.css is not None:
dashboard.css = request.css or None
changed.append("css")
+ if request.tags is not None:
+ # Reuse the same helper the REST UpdateDashboardCommand uses so tag
+ # association semantics (custom-tag full replacement) stay identical.
+ from superset.commands.utils import update_tags
+ from superset.tags.models import ObjectType
+
+ update_tags(ObjectType.dashboard, dashboard.id, dashboard.tags,
request.tags)
+ changed.append("tags")
Review Comment:
**Suggestion:** The tool marks `tags` as changed whenever `tags` is present
in the request, even when the requested tag IDs are identical to the current
custom tags. That makes `changed_fields` inaccurate and forces the update path
to commit even though nothing changed. Compute whether the tag set actually
differs (or have `update_tags` return whether it mutated) before appending
`tags` to `changed_fields`. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- Critical MCP tool update_dashboard misreports unchanged tags as changed.
- Database commit executed even when tags remain identical.
- Clients relying on changed_fields get misleading tag updates.
- Extra commits add overhead on busy dashboard update paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Locate the MCP tool entrypoint `update_dashboard` in
`superset/mcp_service/dashboard/tool/update_dashboard.py:20-52`, which calls
`_apply_field_updates` inside the `event_logger.log_context` block at lines
64-70 to apply
changes and determine `changed_fields`.
2. Observe `_apply_field_updates` in the same file at lines 159-209: for
tags, the block
at lines 200-207 checks `if request.tags is not None`, imports `update_tags`
from
`superset.commands.utils`, calls `update_tags(ObjectType.dashboard,
dashboard.id,
dashboard.tags, request.tags)`, and unconditionally appends `"tags"` to the
local
`changed` list.
3. Inspect the `update_tags` helper in `superset/commands/utils.py` (shown
in the tool
output header “Showing lines 160 to 239”): at lines 1-33 of that snippet it
computes
`tags_to_delete` and `tag_ids_to_add`, deleting tags whose IDs are not in
`new_tag_ids`
and adding tags whose IDs are not in the current custom tag IDs; if
`new_tag_ids` is
exactly the same set as the existing custom tag IDs, both `tags_to_delete`
and
`tag_ids_to_add` are empty and no `TagDAO` operations execute, so the
database tag
relationships remain unchanged.
4. Run the MCP service and call `update_dashboard` with a dashboard whose
custom tags
already match the requested IDs (for example, existing custom tag IDs [3, 7]
and a request
payload with `"tags": [3, 7]`): `update_tags` performs no actual tag
mutation, but
`_apply_field_updates` still appends `"tags"` to `changed`; back in
`update_dashboard` at
lines 64-81, `changed_fields` is non-empty so `db.session.commit()` is
executed and the
response’s `changed_fields` includes `"tags"`, even though the tag set was
unchanged,
making the change report inaccurate and forcing a needless commit.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=529b732f39ca459ebe5c98ab28381adc&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=529b732f39ca459ebe5c98ab28381adc&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/mcp_service/dashboard/tool/update_dashboard.py
**Line:** 200:207
**Comment:**
*Incorrect Condition Logic: The tool marks `tags` as changed whenever
`tags` is present in the request, even when the requested tag IDs are identical
to the current custom tags. That makes `changed_fields` inaccurate and forces
the update path to commit even though nothing changed. Compute whether the tag
set actually differs (or have `update_tags` return whether it mutated) before
appending `tags` to `changed_fields`.
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%2F40957&comment_hash=0de43fc1f40fd07299e603e0d2afaa82efb4b0a10bcabcf07fae7451e309bbcd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=0de43fc1f40fd07299e603e0d2afaa82efb4b0a10bcabcf07fae7451e309bbcd&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]
