-
Notifications
You must be signed in to change notification settings - Fork 14.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: SavedQuery REST API for bulk delete and new API fields #10793
Changes from all commits
aa81ec2
6252b89
877129e
1f7c299
009bcff
ad0d942
2871e26
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import logging | ||
from typing import Any | ||
|
||
from flask import g, Response | ||
from flask_appbuilder.api import expose, protect, rison, safe | ||
from flask_appbuilder.models.sqla.interface import SQLAInterface | ||
from flask_babel import ngettext | ||
|
||
from superset.constants import RouteMethod | ||
from superset.databases.filters import DatabaseFilter | ||
from superset.models.sql_lab import SavedQuery | ||
from superset.queries.saved_queries.commands.bulk_delete import ( | ||
BulkDeleteSavedQueryCommand, | ||
) | ||
from superset.queries.saved_queries.commands.exceptions import ( | ||
SavedQueryBulkDeleteFailedError, | ||
SavedQueryNotFoundError, | ||
) | ||
from superset.queries.saved_queries.filters import SavedQueryFilter | ||
from superset.queries.saved_queries.schemas import ( | ||
get_delete_ids_schema, | ||
openapi_spec_methods_override, | ||
) | ||
from superset.views.base_api import BaseSupersetModelRestApi, statsd_metrics | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class SavedQueryRestApi(BaseSupersetModelRestApi): | ||
datamodel = SQLAInterface(SavedQuery) | ||
|
||
include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | { | ||
RouteMethod.RELATED, | ||
RouteMethod.DISTINCT, | ||
"bulk_delete", # not using RouteMethod since locally defined | ||
} | ||
class_permission_name = "SavedQueryView" | ||
resource_name = "saved_query" | ||
allow_browser_login = True | ||
|
||
base_filters = [["id", SavedQueryFilter, lambda: []]] | ||
|
||
show_columns = [ | ||
"created_by.first_name", | ||
"created_by.id", | ||
"created_by.last_name", | ||
"database.database_name", | ||
"database.id", | ||
"description", | ||
"id", | ||
"label", | ||
"schema", | ||
"sql", | ||
"sql_tables", | ||
] | ||
list_columns = [ | ||
"created_by.first_name", | ||
"created_by.id", | ||
"created_by.last_name", | ||
"database.database_name", | ||
"database.id", | ||
"db_id", | ||
"description", | ||
"label", | ||
"schema", | ||
"sql", | ||
"sql_tables", | ||
] | ||
add_columns = ["db_id", "description", "label", "schema", "sql"] | ||
edit_columns = add_columns | ||
order_columns = [ | ||
"schema", | ||
"label", | ||
"description", | ||
"sql", | ||
"created_by.first_name", | ||
"database.database_name", | ||
] | ||
|
||
apispec_parameter_schemas = { | ||
"get_delete_ids_schema": get_delete_ids_schema, | ||
} | ||
openapi_spec_tag = "Queries" | ||
openapi_spec_methods = openapi_spec_methods_override | ||
|
||
related_field_filters = { | ||
"database": "database_name", | ||
} | ||
filter_rel_fields = {"database": [["id", DatabaseFilter, lambda: []]]} | ||
allowed_rel_fields = {"database"} | ||
allowed_distinct_fields = {"schema"} | ||
|
||
def pre_add(self, item: SavedQuery) -> None: | ||
item.user = g.user | ||
|
||
def pre_update(self, item: SavedQuery) -> None: | ||
self.pre_add(item) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 . Perhaps there should be an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually there is a |
||
|
||
@expose("/", methods=["DELETE"]) | ||
@protect() | ||
@safe | ||
@statsd_metrics | ||
@rison(get_delete_ids_schema) | ||
def bulk_delete( | ||
self, **kwargs: Any | ||
) -> Response: # pylint: disable=arguments-differ | ||
"""Delete bulk Saved Queries | ||
--- | ||
delete: | ||
description: >- | ||
Deletes multiple saved queries in a bulk operation. | ||
parameters: | ||
- in: query | ||
name: q | ||
content: | ||
application/json: | ||
schema: | ||
$ref: '#/components/schemas/get_delete_ids_schema' | ||
responses: | ||
200: | ||
description: Saved queries bulk delete | ||
content: | ||
application/json: | ||
schema: | ||
type: object | ||
properties: | ||
message: | ||
type: string | ||
401: | ||
$ref: '#/components/responses/401' | ||
404: | ||
$ref: '#/components/responses/404' | ||
422: | ||
$ref: '#/components/responses/422' | ||
500: | ||
$ref: '#/components/responses/500' | ||
""" | ||
item_ids = kwargs["rison"] | ||
try: | ||
BulkDeleteSavedQueryCommand(g.user, item_ids).run() | ||
return self.response( | ||
200, | ||
message=ngettext( | ||
"Deleted %(num)d saved query", | ||
"Deleted %(num)d saved queries", | ||
num=len(item_ids), | ||
), | ||
) | ||
except SavedQueryNotFoundError: | ||
return self.response_404() | ||
except SavedQueryBulkDeleteFailedError as ex: | ||
return self.response_422(message=str(ex)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import logging | ||
from typing import List, Optional | ||
|
||
from flask_appbuilder.security.sqla.models import User | ||
|
||
from superset.commands.base import BaseCommand | ||
from superset.dao.exceptions import DAODeleteFailedError | ||
from superset.models.dashboard import Dashboard | ||
from superset.queries.saved_queries.commands.exceptions import ( | ||
SavedQueryBulkDeleteFailedError, | ||
SavedQueryNotFoundError, | ||
) | ||
from superset.queries.saved_queries.dao import SavedQueryDAO | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class BulkDeleteSavedQueryCommand(BaseCommand): | ||
def __init__(self, user: User, model_ids: List[int]): | ||
self._actor = user | ||
self._model_ids = model_ids | ||
self._models: Optional[List[Dashboard]] = None | ||
|
||
def run(self) -> None: | ||
self.validate() | ||
try: | ||
SavedQueryDAO.bulk_delete(self._models) | ||
return None | ||
except DAODeleteFailedError as ex: | ||
logger.exception(ex.exception) | ||
raise SavedQueryBulkDeleteFailedError() | ||
|
||
def validate(self) -> None: | ||
# Validate/populate model exists | ||
self._models = SavedQueryDAO.find_by_ids(self._model_ids) | ||
if not self._models or len(self._models) != len(self._model_ids): | ||
raise SavedQueryNotFoundError() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
from flask_babel import lazy_gettext as _ | ||
|
||
from superset.commands.exceptions import CommandException, DeleteFailedError | ||
|
||
|
||
class SavedQueryBulkDeleteFailedError(DeleteFailedError): | ||
message = _("Saved queries could not be deleted.") | ||
|
||
|
||
class SavedQueryNotFoundError(CommandException): | ||
message = _("Saved query not found.") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import logging | ||
from typing import List, Optional | ||
|
||
from sqlalchemy.exc import SQLAlchemyError | ||
|
||
from superset.dao.base import BaseDAO | ||
from superset.dao.exceptions import DAODeleteFailedError | ||
from superset.extensions import db | ||
from superset.models.sql_lab import SavedQuery | ||
from superset.queries.saved_queries.filters import SavedQueryFilter | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class SavedQueryDAO(BaseDAO): | ||
model_cls = SavedQuery | ||
base_filter = SavedQueryFilter | ||
|
||
@staticmethod | ||
def bulk_delete(models: Optional[List[SavedQuery]], commit: bool = True) -> None: | ||
item_ids = [model.id for model in models] if models else [] | ||
try: | ||
db.session.query(SavedQuery).filter(SavedQuery.id.in_(item_ids)).delete( | ||
synchronize_session="fetch" | ||
villebro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
if commit: | ||
db.session.commit() | ||
except SQLAlchemyError: | ||
if commit: | ||
db.session.rollback() | ||
raise DAODeleteFailedError() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
from typing import Any | ||
|
||
from flask import g | ||
from flask_sqlalchemy import BaseQuery | ||
|
||
from superset.models.sql_lab import SavedQuery | ||
from superset.views.base import BaseFilter | ||
|
||
|
||
class SavedQueryFilter(BaseFilter): # pylint: disable=too-few-public-methods | ||
def apply(self, query: BaseQuery, value: Any) -> BaseQuery: | ||
""" | ||
Filter saved queries to only those created by current user. | ||
|
||
:returns: flask-sqlalchemy query | ||
""" | ||
return query.filter( | ||
SavedQuery.created_by == g.user # pylint: disable=comparison-with-callable | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems premature, and unclear if calling a caching backend would perform better than doing the actual parsing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to the equivalent of Ruby's:
which caches the result in-memory for the lifetime of the object :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually reverted this because of
mypy
python/mypy#1362
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, like I said it's not a blocker :)