[dashboard] Refactor API using SIP-35 #9315
Conversation
superset/dao/base.py
Outdated
| from superset.extensions import db | ||
|
|
||
|
|
||
| def generic_create(model_cls: Model, properties: Dict, commit=True) -> Optional[Model]: |
There was a problem hiding this comment.
Don't pass in the class here. Instead, the caller will have created whatever instance he wants and then should pass it in. You can then combine your create / update into a single upsert method. Also, you can call this something like def upsert(model, commit=True)
There was a problem hiding this comment.
We can do that and end up with less code. Yet I tend to think it's more readable this way (and easier to reason), DAO create methods call generic_create, update methods call generic_update, also Exceptions follow the same pattern.
There was a problem hiding this comment.
I think we shouldn't combine create and update. If I'm trying to update a pre-existing record and someone deletes it in the interim from another process, I want that to error. Upsert would hide the race condition.
Codecov Report
@@ Coverage Diff @@
## master #9315 +/- ##
=======================================
Coverage 59.08% 59.08%
=======================================
Files 374 374
Lines 12205 12205
Branches 2989 2989
=======================================
Hits 7211 7211
Misses 4815 4815
Partials 179 179
Continue to review full report at Codecov.
|
superset/dao/base.py
Outdated
| from superset.extensions import db | ||
|
|
||
|
|
||
| def generic_create(model_cls: Model, properties: Dict, commit=True) -> Optional[Model]: |
There was a problem hiding this comment.
I think we shouldn't combine create and update. If I'm trying to update a pre-existing record and someone deletes it in the interim from another process, I want that to error. Upsert would hide the race condition.
| @@ -20,10 +20,11 @@ | |||
| from typing import List, Optional | |||
There was a problem hiding this comment.
Would it be useful to move this file to tests/dashboards/api_tests.py? Following the implementation structure in the tests could help us find things in the future.
…board-sip-35 # Conflicts: # superset/views/dashboard/api.py
mistercrunch
added
🏷️ bot
CATEGORY
SUMMARY
Refactor of the Dashboard API using SIP-35
ADDITIONAL INFORMATION
REVIEWERS
@willbarrett, @craig-rueda, @villebro