feat: SavedQuery REST API for bulk delete and new API fields #10793
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10793 +/- ##
==========================================
+ Coverage 61.22% 61.58% +0.35%
==========================================
Files 802 429 -373
Lines 37814 13969 -23845
Branches 3555 3561 +6
==========================================
- Hits 23153 8603 -14550
+ Misses 14475 5180 -9295
Partials 186 186
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
dpgaspar
changed the title
| @@ -203,6 +203,10 @@ def sqlalchemy_uri(self) -> URL: | |||
| def url(self) -> str: | |||
| return "/superset/sqllab?savedQueryId={0}".format(self.id) | |||
|
|
|||
| @property | |||
| def sql_tables(self) -> List[Table]: | |||
| return list(ParsedQuery(self.sql).tables) | |||
There was a problem hiding this comment.
It's too bad there isn't a great pattern to memoise this on the object that I'm aware of. Parsing queries gets expensive and I'd hate to see this called multiple times for the same object in a web request.
There was a problem hiding this comment.
Seems premature, and unclear if calling a caching backend would perform better than doing the actual parsing...
There was a problem hiding this comment.
I was referring to the equivalent of Ruby's:
@parsed ||= ParsedQuery.new(self.sql)
return parsed.tableswhich caches the result in-memory for the lifetime of the object :)
There was a problem hiding this comment.
Added an lru cache, not a big win since we will only cache the parsed query on an object instance itself. But will avoid possible repeated property get's during the instance lifetime.
There was a problem hiding this comment.
Actually reverted this because of mypy
python/mypy#1362
There was a problem hiding this comment.
That's fine, like I said it's not a blocker :)
| item.user = g.user | ||
|
|
||
| def pre_update(self, item: SavedQuery) -> None: | ||
| self.pre_add(item) |
There was a problem hiding this comment.
This seems questionable - if an admin alters a saved query belonging to another user, should the ownership change?
There was a problem hiding this comment.
+1, I wouldn't make the updater the owner, even though in the current model people should not see other's people saved queries, but this could change in the future.
There was a problem hiding this comment.
+1 . Perhaps there should be an updated_by?
There was a problem hiding this comment.
Actually there is a created_by and changed_by and I'm filtering the queries by created_by. This is here to maintain functionality for tags and fav, there is a SQLA listener that calls: https://github.com/apache/incubator-superset/blob/fd2d1c58c566d9312d6cfc5641a06ac2b03e753a/superset/models/tags.py#L225
| item.user = g.user | ||
|
|
||
| def pre_update(self, item: SavedQuery) -> None: | ||
| self.pre_add(item) |
There was a problem hiding this comment.
+1 . Perhaps there should be an updated_by?
This reverts commit ad0d942
…10793) * feat: SavedQuery REST API for bulk delete * fix, singular msg and test * remove 403 from OpenAPI spec * filter by current user using created_by add sql_tables field * fixes for new filter, add user field on pre_update, pre_add * add lru cache to property * Revert "add lru cache to property" This reverts commit ad0d942
mistercrunch
added
🏷️ bot
SUMMARY
This PR introduces:
created_byuser_idfor fav and tagsBulk delete swagger:
ADDITIONAL INFORMATION