[dashboards] New, API for Bulk delete (#8972)

* [dashboards] New, API for Bulk delete

* [dashboards] Tests

* [dashboards] Fix not found on multiple delete

* [dashboards] Bulk delete partial deletes on not owned dashes

* [dashboards] Improve OpenAPI spec and tests

* [dashboards] Test for bad request

* [dashboards] i18n

* [dashboards] black

* [dashboard] make bulk all or nothing

* [dashboard] Log on sqlalchemy error

* [dashboard] Log on sqlalchemy error

superset/views/dashboard/api.py

@@ -15,24 +15,30 @@
 # specific language governing permissions and limitations
 # under the License.
 import json
+import logging
 import re
 from typing import Dict, List
 
-from flask import current_app, make_response
+from flask import current_app, g, make_response
 from flask_appbuilder.api import expose, protect, rison, safe
 from flask_appbuilder.models.sqla.interface import SQLAInterface
+from flask_babel import lazy_gettext as _, ngettext
 from marshmallow import fields, post_load, pre_load, Schema, ValidationError
 from marshmallow.validate import Length
+from sqlalchemy.exc import SQLAlchemyError
 
-from superset.exceptions import SupersetException
+from superset.exceptions import SupersetException, SupersetSecurityException
 from superset.models.dashboard import Dashboard
 from superset.utils import core as utils
-from superset.views.base import generate_download_headers
+from superset.views.base import check_ownership, generate_download_headers
 from superset.views.base_api import BaseOwnedModelRestApi
 from superset.views.base_schemas import BaseOwnedSchema, validate_owner
 
 from .mixin import DashboardMixin
 
+logger = logging.getLogger(__name__)
+get_delete_ids_schema = {"type": "array", "items": {"type": "integer"}}
+
 
 class DashboardJSONMetadataSchema(Schema):
     timed_refresh_immune_slices = fields.List(fields.Integer())
@@ -136,6 +142,7 @@ class DashboardRestApi(DashboardMixin, BaseOwnedModelRestApi):
         "post": "add",
         "put": "edit",
         "delete": "delete",
+        "bulk_delete": "delete",
         "info": "list",
         "related": "list",
     }
@@ -172,6 +179,93 @@ class DashboardRestApi(DashboardMixin, BaseOwnedModelRestApi):
     }
     filter_rel_fields_field = {"owners": "first_name", "slices": "slice_name"}
 
+    @expose("/", methods=["DELETE"])
+    @protect()
+    @safe
+    @rison(get_delete_ids_schema)
+    def bulk_delete(self, **kwargs):  # pylint: disable=arguments-differ
+        """Delete bulk Dashboards
+        ---
+        delete:
+          parameters:
+          - in: query
+            name: q
+            content:
+              application/json:
+                schema:
+                  type: array
+                  items:
+                    type: integer
+          responses:
+            200:
+              description: Dashboard bulk delete
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      message:
+                        type: string
+            401:
+              $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/401'
+            404:
+              $ref: '#/components/responses/404'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        item_ids = kwargs["rison"]
+        query = self.datamodel.session.query(Dashboard).filter(
+            Dashboard.id.in_(item_ids)
+        )
+        items = self._base_filters.apply_all(query).all()
+        if not items:
+            return self.response_404()
+        # Check user ownership over the items
+        for item in items:
+            try:
+                check_ownership(item)
+            except SupersetSecurityException as e:
+                logger.warning(
+                    f"Dashboard {item} was not deleted, "
+                    f"because the user ({g.user}) does not own it"
+                )
+                return self.response(403, message=_("No dashboards deleted"))
+            except SQLAlchemyError as e:
+                logger.error(f"Error checking dashboard ownership {e}")
+                return self.response_422(message=str(e))
+        # bulk delete, first delete related data
+        for item in items:
+            try:
+                item.slices = []
+                item.owners = []
+                self.datamodel.session.merge(item)
+            except SQLAlchemyError as e:
+                logger.error(f"Error bulk deleting related data on dashboards {e}")
+                self.datamodel.session.rollback()
+                return self.response_422(message=str(e))
+        # bulk delete itself
+        try:
+            self.datamodel.session.query(Dashboard).filter(
+                Dashboard.id.in_(item_ids)
+            ).delete(synchronize_session="fetch")
+        except SQLAlchemyError as e:
+            logger.error(f"Error bulk deleting dashboards {e}")
+            self.datamodel.session.rollback()
+            return self.response_422(message=str(e))
+        self.datamodel.session.commit()
+        return self.response(
+            200,
+            message=ngettext(
+                f"Deleted %(num)d dashboard",
+                f"Deleted %(num)d dashboards",
+                num=len(items),
+            ),
+        )
+
     @expose("/export/", methods=["GET"])
     @protect()
     @safe

tests/dashboard_api_tests.py

@@ -78,6 +78,44 @@ class DashboardApiTests(SupersetTestCase, ApiOwnersTestCaseMixin):
         model = db.session.query(models.Dashboard).get(dashboard_id)
         self.assertEqual(model, None)
 
+    def test_delete_bulk_dashboards(self):
+        """
+            Dashboard API: Test delete bulk
+        """
+        admin_id = self.get_user("admin").id
+        dashboard_count = 4
+        dashboard_ids = list()
+        for dashboard_name_index in range(dashboard_count):
+            dashboard_ids.append(
+                self.insert_dashboard(
+                    f"title{dashboard_name_index}",
+                    f"slug{dashboard_name_index}",
+                    [admin_id],
+                ).id
+            )
+        self.login(username="admin")
+        argument = dashboard_ids
+        uri = f"api/v1/dashboard/?q={prison.dumps(argument)}"
+        rv = self.client.delete(uri)
+        self.assertEqual(rv.status_code, 200)
+        response = json.loads(rv.data.decode("utf-8"))
+        expected_response = {"message": f"Deleted {dashboard_count} dashboards"}
+        self.assertEqual(response, expected_response)
+        for dashboard_id in dashboard_ids:
+            model = db.session.query(models.Dashboard).get(dashboard_id)
+            self.assertEqual(model, None)
+
+    def test_delete_bulk_dashboards_bad_request(self):
+        """
+            Dashboard API: Test delete bulk bad request
+        """
+        dashboard_ids = [1, "a"]
+        self.login(username="admin")
+        argument = dashboard_ids
+        uri = f"api/v1/dashboard/?q={prison.dumps(argument)}"
+        rv = self.client.delete(uri)
+        self.assertEqual(rv.status_code, 400)
+
     def test_delete_not_found_dashboard(self):
         """
             Dashboard API: Test not found delete
@@ -88,6 +126,17 @@ class DashboardApiTests(SupersetTestCase, ApiOwnersTestCaseMixin):
         rv = self.client.delete(uri)
         self.assertEqual(rv.status_code, 404)
 
+    def test_delete_bulk_dashboards_not_found(self):
+        """
+            Dashboard API: Test delete bulk not found
+        """
+        dashboard_ids = [1001, 1002]
+        self.login(username="admin")
+        argument = dashboard_ids
+        uri = f"api/v1/dashboard/?q={prison.dumps(argument)}"
+        rv = self.client.delete(uri)
+        self.assertEqual(rv.status_code, 404)
+
     def test_delete_dashboard_admin_not_owned(self):
         """
             Dashboard API: Test admin delete not owned
@@ -102,6 +151,35 @@ class DashboardApiTests(SupersetTestCase, ApiOwnersTestCaseMixin):
         model = db.session.query(models.Dashboard).get(dashboard_id)
         self.assertEqual(model, None)
 
+    def test_delete_bulk_dashboard_admin_not_owned(self):
+        """
+            Dashboard API: Test admin delete bulk not owned
+        """
+        gamma_id = self.get_user("gamma").id
+        dashboard_count = 4
+        dashboard_ids = list()
+        for dashboard_name_index in range(dashboard_count):
+            dashboard_ids.append(
+                self.insert_dashboard(
+                    f"title{dashboard_name_index}",
+                    f"slug{dashboard_name_index}",
+                    [gamma_id],
+                ).id
+            )
+
+        self.login(username="admin")
+        argument = dashboard_ids
+        uri = f"api/v1/dashboard/?q={prison.dumps(argument)}"
+        rv = self.client.delete(uri)
+        response = json.loads(rv.data.decode("utf-8"))
+        self.assertEqual(rv.status_code, 200)
+        expected_response = {"message": f"Deleted {dashboard_count} dashboards"}
+        self.assertEqual(response, expected_response)
+
+        for dashboard_id in dashboard_ids:
+            model = db.session.query(models.Dashboard).get(dashboard_id)
+            self.assertEqual(model, None)
+
     def test_delete_dashboard_not_owned(self):
         """
             Dashboard API: Test delete try not owned
@@ -127,6 +205,68 @@ class DashboardApiTests(SupersetTestCase, ApiOwnersTestCaseMixin):
         db.session.delete(user_alpha2)
         db.session.commit()
 
+    def test_delete_bulk_dashboard_not_owned(self):
+        """
+            Dashboard API: Test delete bulk try not owned
+        """
+        user_alpha1 = self.create_user(
+            "alpha1", "password", "Alpha", email="alpha1@superset.org"
+        )
+        user_alpha2 = self.create_user(
+            "alpha2", "password", "Alpha", email="alpha2@superset.org"
+        )
+        existing_slice = (
+            db.session.query(Slice).filter_by(slice_name="Girl Name Cloud").first()
+        )
+
+        dashboard_count = 4
+        dashboards = list()
+        for dashboard_name_index in range(dashboard_count):
+            dashboards.append(
+                self.insert_dashboard(
+                    f"title{dashboard_name_index}",
+                    f"slug{dashboard_name_index}",
+                    [user_alpha1.id],
+                    slices=[existing_slice],
+                    published=True,
+                )
+            )
+
+        owned_dashboard = self.insert_dashboard(
+            "title_owned",
+            "slug_owned",
+            [user_alpha2.id],
+            slices=[existing_slice],
+            published=True,
+        )
+
+        self.login(username="alpha2", password="password")
+
+        # verify we can't delete not owned dashboards
+        arguments = [dashboard.id for dashboard in dashboards]
+        uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}"
+        rv = self.client.delete(uri)
+        self.assertEqual(rv.status_code, 403)
+        response = json.loads(rv.data.decode("utf-8"))
+        expected_response = {"message": "No dashboards deleted"}
+        self.assertEqual(response, expected_response)
+
+        # nothing is delete in bulk with a list of owned and not owned dashboards
+        arguments = [dashboard.id for dashboard in dashboards] + [owned_dashboard.id]
+        uri = f"api/v1/dashboard/?q={prison.dumps(arguments)}"
+        rv = self.client.delete(uri)
+        self.assertEqual(rv.status_code, 403)
+        response = json.loads(rv.data.decode("utf-8"))
+        expected_response = {"message": "No dashboards deleted"}
+        self.assertEqual(response, expected_response)
+
+        for dashboard in dashboards:
+            db.session.delete(dashboard)
+        db.session.delete(owned_dashboard)
+        db.session.delete(user_alpha1)
+        db.session.delete(user_alpha2)
+        db.session.commit()
+
     def test_create_dashboard(self):
         """
             Dashboard API: Test create dashboard