feat(hooks): add configurable post-creation hook for dashboards - #42836
feat(hooks): add configurable post-creation hook for dashboards#42836alexandrusoare wants to merge 3 commits into
Conversation
Code Review Agent Run #0b9552Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
| if after_create := current_app.config.get("AFTER_ASSET_CREATE"): | ||
| after_create(new_dashboard, "dashboard") | ||
| db.session.commit() |
There was a problem hiding this comment.
Suggestion: The dashboard is committed before AFTER_ASSET_CREATE runs. If the hook raises an exception, the request fails but the dashboard remains permanently persisted, so the creation is not atomic and cannot be rolled back. Invoke the hook before the transaction's commit, or explicitly roll back and remove the dashboard when the hook fails. [logic error]
Severity Level: Major ⚠️
- ❌ Dashboard creation requests fail when configured hooks raise exceptions.
- ⚠️ Failed requests leave orphaned untitled dashboards persisted.
- ⚠️ Downstream integrations can make UI creation appear unsuccessful.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/dashboard/views.py
**Line:** 98:100
**Comment:**
*Logic Error: The dashboard is committed before `AFTER_ASSET_CREATE` runs. If the hook raises an exception, the request fails but the dashboard remains permanently persisted, so the creation is not atomic and cannot be rolled back. Invoke the hook before the transaction's commit, or explicitly roll back and remove the dashboard when the hook fails.
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|
The flagged issue is correct. Committing the transaction before executing the hook means that if the hook fails, the dashboard remains in the database, violating atomicity. To resolve this, you should move the hook execution before the initial commit or wrap the operations in a try-except block to roll back the session if the hook fails. Here is a concise fix: db.session.add(new_dashboard)
try:
db.session.commit()
if after_create := current_app.config.get("AFTER_ASSET_CREATE"):
after_create(new_dashboard, "dashboard")
db.session.commit()
except Exception:
db.session.rollback()
raiseThere are no other comments in this PR. Would you like me to review any other parts of the code? superset/views/dashboard/views.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42836 +/- ##
==========================================
+ Coverage 56.18% 65.73% +9.54%
==========================================
Files 2843 2843
Lines 162653 162656 +3
Branches 37239 37240 +1
==========================================
+ Hits 91380 106914 +15534
+ Misses 70430 53649 -16781
- Partials 843 2093 +1250
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SUMMARY
Adds an
AFTER_ASSET_CREATEconfig callback that fires after a new dashboard is persisted. This allows downstream integrations (e.g. auto-categorization, audit logging, notification triggers) to react to asset creation without modifying core views. The hook receives the model instance and the asset type string ("dashboard")BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION