You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/09/12 00:00:11 UTC
[PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
mikebridge opened a new pull request, #44185:
URL: https://github.com/apache/superset/pull/44185
### SUMMARY
`charts_attached_to_dashboard` returns `[attach, detach)` attachment **windows** in transaction-id space β one entry per attachment episode, for every chart that has **ever** been on the dashboard β not present membership. The name read as a membership query, and that misreading is the common root of three M2M-shadow bugs (sc-119770, sc-119907, sc-120007).
This renames it to `chart_attachment_windows_for_dashboard` across all 17 references (`superset/versioning/` + tests) and rewrites the docstring so the summary line states the history-not-membership contract outright. Mechanical rename; **no behavior change** β no call site's logic is touched.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A β internal rename, no UI change.
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/versioning/ # 152 passed
grep -rn "charts_attached_to_dashboard" superset/ tests/ # empty
```
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
π€ Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01C7U5oFfrqUyM6hJTZiW2uf
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44185:
URL: https://github.com/apache/superset/pull/44185#discussion_r3994322878
##########
superset/versioning/restore.py:
##########
@@ -213,21 +213,21 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
# pylint: disable=import-outside-toplevel
# Local imports: models.slice transitively imports models.core, which needs
# the initialised app β a module-top import would recreate the bootstrap
- # cycle documented in changes/listener.py. charts_attached_to_dashboard is
+ # cycle documented in changes/listener.py. chart_attachment_windows_for_dashboard is
# imported lazily for the same reason: it pulls the window helpers, whose
# package transitively imports the versioning.changes listener graph, so a
# module-top import here would re-enter that same bootstrap cycle.
from superset.models.slice import Slice
- from superset.versioning.membership import charts_attached_to_dashboard
+ from superset.versioning.membership import chart_attachment_windows_for_dashboard
- # charts_attached_to_dashboard owns the association-shadow read and the
+ # chart_attachment_windows_for_dashboard owns the association-shadow read and the
# attach/detach window pairing (the single place that must never filter the
# M2M shadow by end_transaction_id β Continuum never closes it). A slice was
# a member at transaction_id iff one of its windows contains it (sc-119907).
member_ids = sorted(
{
slice_id
- for slice_id, window in charts_attached_to_dashboard(dashboard.id)
+ for slice_id, window in chart_attachment_windows_for_dashboard(dashboard.id)
Review Comment:
**Suggestion:** This reads attachment history while the revert is only in memory, so a concurrent membership edit can commit before assignment and then be overwritten by the restore flush. [race condition]
**Assessment:** π `Major` Β· π `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b31c8d95a9f4894ae7dc8c4fe68fe95&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=8b31c8d95a9f4894ae7dc8c4fe68fe95&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/versioning/restore.py
**Line:** 230:230
**Comment:**
*Race Condition: This reads attachment history while the revert is only in memory, so a concurrent membership edit can commit before assignment and then be overwritten by the restore flush.
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%2F44185&comment_hash=aa1f4c9a97049d81a6744e6d04fd55fbdc45bf77d53ac4833c0cf95d38af53dd&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44185&comment_hash=aa1f4c9a97049d81a6744e6d04fd55fbdc45bf77d53ac4833c0cf95d38af53dd&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on code in PR #44185:
URL: https://github.com/apache/superset/pull/44185#discussion_r3995217282
##########
superset/versioning/restore.py:
##########
@@ -213,21 +213,21 @@ def _restore_dashboard_membership(dashboard: Any, transaction_id: int) -> list[i
# pylint: disable=import-outside-toplevel
# Local imports: models.slice transitively imports models.core, which needs
# the initialised app β a module-top import would recreate the bootstrap
- # cycle documented in changes/listener.py. charts_attached_to_dashboard is
+ # cycle documented in changes/listener.py. chart_attachment_windows_for_dashboard is
# imported lazily for the same reason: it pulls the window helpers, whose
# package transitively imports the versioning.changes listener graph, so a
# module-top import here would re-enter that same bootstrap cycle.
from superset.models.slice import Slice
- from superset.versioning.membership import charts_attached_to_dashboard
+ from superset.versioning.membership import chart_attachment_windows_for_dashboard
- # charts_attached_to_dashboard owns the association-shadow read and the
+ # chart_attachment_windows_for_dashboard owns the association-shadow read and the
# attach/detach window pairing (the single place that must never filter the
# M2M shadow by end_transaction_id β Continuum never closes it). A slice was
# a member at transaction_id iff one of its windows contains it (sc-119907).
member_ids = sorted(
{
slice_id
- for slice_id, window in charts_attached_to_dashboard(dashboard.id)
+ for slice_id, window in chart_attachment_windows_for_dashboard(dashboard.id)
Review Comment:
Pre-existing on master, not introduced here β this PR is a pure rename with zero logic changes (the read at this line is byte-identical to master modulo the function name). The race you describe is real and is exactly what #44015 (sc-115423) addresses by row-locking the live entity for the duration of the restore; that fix composes with this rename with no interaction.
*Posted by Claude (AI) on behalf of @mikebridge.*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44185:
URL: https://github.com/apache/superset/pull/44185#issuecomment-5641997499
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44185:
URL: https://github.com/apache/superset/pull/44185#issuecomment-5642031437
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44185?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.45%. Comparing base ([`36361d9`](https://app.codecov.io/gh/apache/superset/commit/36361d9d0462b34114d4be0a87ca78d7a576f387?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`95494c2`](https://app.codecov.io/gh/apache/superset/commit/95494c2eac84aee50d3693bbbf275e527a2fce84?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44185?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/versioning/activity/impact.py](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&filepath=superset%2Fversioning%2Factivity%2Fimpact.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmVyc2lvbmluZy9hY3Rpdml0eS9pbXBhY3QucHk=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/versioning/activity/scope.py](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&filepath=superset%2Fversioning%2Factivity%2Fscope.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmVyc2lvbmluZy9hY3Rpdml0eS9zY29wZS5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/versioning/membership.py](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&filepath=superset%2Fversioning%2Fmembership.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmVyc2lvbmluZy9tZW1iZXJzaGlwLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/versioning/restore.py](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&filepath=superset%2Fversioning%2Frestore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmVyc2lvbmluZy9yZXN0b3JlLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44185?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44185 +/- ##
==========================================
- Coverage 66.18% 57.45% -8.74%
==========================================
Files 2925 2925
Lines 172563 172563
Branches 40058 40058
==========================================
- Hits 114212 99142 -15070
- Misses 55970 72489 +16519
+ Partials 2381 932 -1449
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44185/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44185/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `37.34% <0.00%> (ΓΈ)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44185/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.22% <0.00%> (ΓΈ)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/44185/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.27% <0.00%> (-17.45%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44185/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44185?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44185:
URL: https://github.com/apache/superset/pull/44185#issuecomment-5642028424
<!-- Bito Reply -->
The flagged issue correctly identifies a potential race condition in `superset/versioning/restore.py`. Because `chart_attachment_windows_for_dashboard` reads the association history from the database, a concurrent membership change could commit after the read but before the restore flush, leading to an inconsistent state.
To resolve this, you should wrap the membership check and the subsequent restore flush in a database transaction that ensures isolation, or use a `SELECT ... FOR UPDATE` lock on the relevant dashboard or membership records if the database backend supports it. Given the context of `superset/versioning/restore.py`, ensuring the operation is atomic within the current session's transaction is the recommended approach.
Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well?
**superset/versioning/restore.py**
```
# Wrap in a transaction to ensure atomicity
with db.session.begin_nested():
member_ids = sorted(
{
slice_id
for slice_id, window in chart_attachment_windows_for_dashboard(dashboard.id)
if window.contains(transaction_id)
}
)
# ... proceed with restore flush ...
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "richardfogaca (via GitHub)" <gi...@apache.org>.
richardfogaca merged PR #44185:
URL: https://github.com/apache/superset/pull/44185
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(versioning): rename charts_attached_to_dashboard to chart_attachment_windows_for_dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44185:
URL: https://github.com/apache/superset/pull/44185#issuecomment-5688903760
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org