You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2024/06/07 22:35:57 UTC
[PR] fix: Improving handling for tag relationship when deleting assets [superset]
Vitor-Avila opened a new pull request, #29117:
URL: https://github.com/apache/superset/pull/29117
### SUMMARY
The relationship between the `tagged_objects` table and the asset table (for example, `dashboard`) is a combination of the `object_id` and `object_type`. While this relationship works properly when displaying tags/etc, when deleting an asset SQLAlchemy was trying to run `DELETE FROM tagged_object WHERE tagged_object.tag_id = ? AND tagged_object.object_id = ?` which would delete more rows then it should (it's missing another filter for `object_type`), causing the operation to fail with an error:
```
sqlalchemy.orm.exc.StaleDataError: DELETE statement on table 'tagged_object' expected to delete 3 row(s); Only 10 were matched.
```
This PR adds the `passive_deletes=True` argument to the `relationship`, so that the cascade delete is handled by the DB (based on the constraints).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
1. Make sure you have a dashboard and a chart with the same ID.
2. Assign a tag for both assets. Also add `owners` to both assets.
3. Validate that you're able to delete one of the assets.
### ADDITIONAL INFORMATION
- [ ] 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
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2159348793
Adding tests here would be complex, since it requires having assets (dashboards and charts) with the same ID and forcing the ID during the asset creation is atypical and could impact other tests.
I wanted to be extra sure the `passive_deletes` argument would handle this correctly, specially since the FK set on `TaggedObject.object_id` (which is currently mapped to multiple tables) is only applied in the SQLAlchemy level (couldn't find any migration to implement this relationship in the DB level, and AFAIK the DB doesn't support a single column being a FK to multiple tables):
``` python
object_id = Column(
Integer,
ForeignKey("dashboards.id"),
ForeignKey("slices.id"),
ForeignKey("saved_query.id"),
)
```
I did some tests running Superset with a PSQL DB with query logging enabled, and the deletion queries look correct (they're including a filter for the `object_type`):
```
2024-06-10 21:25:13 UTC:189.6.252.197(13164):preset@superset_local:[3611]:LOG: statement: DELETE FROM dashboard_slices WHERE dashboard_slices.dashboard_id = 1 AND dashboard_slices.slice_id = 1
2024-06-10 21:25:13 UTC:189.6.252.197(13164):preset@superset_local:[3611]:LOG: duration: 0.211 ms
2024-06-10 21:25:13 UTC:189.6.252.197(13164):preset@superset_local:[3611]:LOG: statement: DELETE FROM dashboards WHERE dashboards.id = 1
2024-06-10 21:25:13 UTC:189.6.252.197(13164):preset@superset_local:[3611]:LOG: duration: 0.958 ms
2024-06-10 21:25:13 UTC:189.6.252.197(13164):preset@superset_local:[3611]:LOG: statement: DELETE FROM tagged_object WHERE tagged_object.object_type = 'dashboard' AND tagged_object.object_id = 1
2024-06-10 21:40:46 UTC:189.6.252.197(13148):preset@superset_local:[4186]:LOG: statement: DELETE FROM saved_query WHERE saved_query.id = 1
2024-06-10 21:40:46 UTC:189.6.252.197(13148):preset@superset_local:[4186]:LOG: duration: 0.334 ms
2024-06-10 21:40:46 UTC:189.6.252.197(13148):preset@superset_local:[4186]:LOG: statement: DELETE FROM tagged_object WHERE tagged_object.object_type = 'query' AND tagged_object.object_id = 1
2024-06-10 21:41:07 UTC:189.6.252.197(13150):preset@superset_local:[4188]:LOG: statement: DELETE FROM slices WHERE slices.id = 1
2024-06-10 21:41:07 UTC:189.6.252.197(13150):preset@superset_local:[4188]:LOG: duration: 0.709 ms
2024-06-10 21:41:07 UTC:189.6.252.197(13150):preset@superset_local:[4188]:LOG: statement: DELETE FROM tagged_object WHERE tagged_object.object_type = 'chart' AND tagged_object.object_id = 1
```
We could also assess a bigger alternative which would be changing the `tagged_object` table to have one column per FK (`chart_id`, `dashboard_id`, `query_id`) which should make the relationship simpler and work with the SQLAlchemy default cascade behavior (but would require a DB migration).
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2161261556
@Vitor-Avila I'm not sure this is the right approach, i.e., historically errors of the form,
> sqlalchemy.orm.exc.StaleDataError: DELETE statement on table 'tagged_object' expected to delete 3 row(s); Only 10 were matched.
typically suggest there's something borked within our ORM.
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2161803360
It seemed like the right longer term solution might be to have one bridge table for each tag relationship (?)
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2163374316
@john-bodley 100% agreed! This is currently happening because the statement to delete entries from the `tagged_object` table does not include a filter for the `object_type` column, so SQLAlchemy was expecting to delete x rows, but the operation would actually delete more than that.
I worked on this as an attempt to have a fast fix in place (the issue is pretty simple to reproduce, make sure you have a dashboard and a chart with the same ID, assign tags/owners to both and try to delete one).
Since it seems this solution didn't fully solve the issue just yet, I just submitted an alternative approach that I believe is better: https://github.com/apache/superset/pull/29229
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #29117:
URL: https://github.com/apache/superset/pull/29117
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2155658448
I'm checking if I can add some tests around this flow to prevent a regression
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2155661969
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/29117?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 64.65%. Comparing base [(`76d897e`)](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`9ad1ead`)](https://app.codecov.io/gh/apache/superset/commit/9ad1ead4587cac4a28994c2a99308fc33bb38c43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 290 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #29117 +/- ##
==========================================
+ Coverage 60.48% 64.65% +4.16%
==========================================
Files 1931 518 -1413
Lines 76236 37492 -38744
Branches 8568 0 -8568
==========================================
- Hits 46114 24239 -21875
+ Misses 28017 13253 -14764
+ Partials 2105 0 -2105
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/29117/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/29117/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.92% <ø> (-0.24%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/29117/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/29117/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.53% <ø> (-0.27%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/29117/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.65% <ø> (+1.16%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/29117/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.04% <ø> (+1.42%)` | :arrow_up: |
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 Sentry](https://app.codecov.io/gh/apache/superset/pull/29117?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).
--
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] fix: Improving handling for tag relationship when deleting assets [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #29117:
URL: https://github.com/apache/superset/pull/29117#issuecomment-2159462015
Given that the DELETE statements look good, and the complexity of the alternative solutions, I think this is solid for now. Let's merge to fix the issue at hand and we can reconsider other approaches in the future.
--
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