fix(Fave): Charts and Dashboards fave/unfave do not commit transactions

superset/charts/api.py

@@ -66,7 +66,9 @@
     DashboardsForbiddenError,
 )
 from superset.commands.chart.export import ExportChartsCommand
+from superset.commands.chart.fave import AddFavoriteChartCommand
 from superset.commands.chart.importers.dispatcher import ImportChartsCommand
+from superset.commands.chart.unfave import DelFavoriteChartCommand
 from superset.commands.chart.update import UpdateChartCommand
 from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand
 from superset.commands.exceptions import CommandException, TagForbiddenError
@@ -898,11 +900,13 @@ def add_favorite(self, pk: int) -> Response:
             500:
               $ref: '#/components/responses/500'
         """
-        chart = ChartDAO.find_by_id(pk)
-        if not chart:
+        try:
+            AddFavoriteChartCommand(pk).run()
+        except ChartNotFoundError:
             return self.response_404()
+        except ChartForbiddenError:
+            return self.response_403()
 
-        ChartDAO.add_favorite(chart)
         return self.response(200, result="OK")
 
     @expose("/<pk>/favorites/", methods=("DELETE",))
@@ -941,11 +945,13 @@ def remove_favorite(self, pk: int) -> Response:
             500:
               $ref: '#/components/responses/500'
         """
-        chart = ChartDAO.find_by_id(pk)
-        if not chart:
-            return self.response_404()
+        try:
+            DelFavoriteChartCommand(pk).run()
+        except ChartNotFoundError:
+            self.response_404()
+        except ChartForbiddenError:
+            self.response_403()
 
-        ChartDAO.remove_favorite(chart)
         return self.response(200, result="OK")
 
     @expose("/warm_up_cache", methods=("PUT",))

superset/commands/chart/exceptions.py

@@ -154,3 +154,11 @@ class DashboardsForbiddenError(ForbiddenError):
 class WarmUpCacheChartNotFoundError(CommandException):
     status = 404
     message = _("Chart not found")
+
+
+class ChartFaveError(CommandException):
+    message = _("Error faving chart")
+
+
+class ChartUnfaveError(CommandException):
+    message = _("Error unfaving chart")

superset/commands/chart/fave.py

@@ -0,0 +1,57 @@
+# 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 functools import partial
+
+from requests_cache import Optional
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.chart.exceptions import (
+    ChartFaveError,
+    ChartForbiddenError,
+    ChartNotFoundError,
+)
+from superset.daos.chart import ChartDAO
+from superset.exceptions import SupersetSecurityException
+from superset.models.slice import Slice
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class AddFavoriteChartCommand(BaseCommand):
+    def __init__(self, chart_id: int) -> None:
+        self._chart_id = chart_id
+        self._chart: Optional[Slice] = None
+
+    @transaction(on_error=partial(on_error, reraise=ChartFaveError))
+    def run(self) -> None:
+        self.validate()
+        return ChartDAO.add_favorite(self._chart)
+
+    def validate(self) -> None:
+        chart = ChartDAO.find_by_id(self._chart_id)
+        if not chart:
+            raise ChartNotFoundError()
+
+        try:
+            security_manager.raise_for_ownership(chart)
+        except SupersetSecurityException as ex:
+            raise ChartForbiddenError() from ex
+
+        self._chart = chart

superset/commands/chart/unfave.py

@@ -0,0 +1,57 @@
+# 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 functools import partial
+
+from requests_cache import Optional
+
+from superset import security_manager
+from superset.commands.base import BaseCommand
+from superset.commands.chart.exceptions import (
+    ChartForbiddenError,
+    ChartNotFoundError,
+    ChartUnfaveError,
+)
+from superset.daos.chart import ChartDAO
+from superset.exceptions import SupersetSecurityException
+from superset.models.slice import Slice
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class DelFavoriteChartCommand(BaseCommand):
+    def __init__(self, chart_id: int) -> None:
+        self._chart_id = chart_id
+        self._chart: Optional[Slice] = None
+
+    @transaction(on_error=partial(on_error, reraise=ChartUnfaveError))
+    def run(self) -> None:
+        self.validate()
+        return ChartDAO.remove_favorite(self._chart)
+
+    def validate(self) -> None:
+        chart = ChartDAO.find_by_id(self._chart_id)
+        if not chart:
+            raise ChartNotFoundError()
+
+        try:
+            security_manager.raise_for_ownership(chart)
+        except SupersetSecurityException as ex:
+            raise ChartForbiddenError() from ex
+
+        self._chart = chart

superset/commands/dashboard/exceptions.py

@@ -84,3 +84,11 @@ class DashboardAccessDeniedError(ForbiddenError):
 
 class DashboardCopyError(CommandInvalidError):
     message = _("Dashboard cannot be copied due to invalid parameters.")
+
+
+class DashboardFaveError(CommandInvalidError):
+    message = _("Dashboard cannot be favorited.")
+
+
+class DashboardUnfaveError(CommandInvalidError):
+    message = _("Dashboard cannot be unfavorited.")

superset/commands/dashboard/fave.py

@@ -0,0 +1,46 @@
+# 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 functools import partial
+
+from requests_cache import Optional
+
+from superset.commands.base import BaseCommand
+from superset.commands.dashboard.exceptions import (
+    DashboardFaveError,
+)
+from superset.daos.dashboard import DashboardDAO
+from superset.models.dashboard import Dashboard
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class AddFavoriteDashboardCommand(BaseCommand):
+    def __init__(self, dashboard_id: int) -> None:
+        self._dashboard_id = dashboard_id
+        self._dashboard: Optional[Dashboard] = None
+
+    @transaction(on_error=partial(on_error, reraise=DashboardFaveError))
+    def run(self) -> None:
+        self.validate()
+        return DashboardDAO.add_favorite(self._dashboard)
+
+    def validate(self) -> None:
+        # Raises DashboardNotFoundError or DashboardAccessDeniedError
+        dashboard = DashboardDAO.get_by_id_or_slug(self._dashboard_id)
+        self._dashboard = dashboard

superset/commands/dashboard/unfave.py

@@ -0,0 +1,46 @@
+# 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 functools import partial
+
+from requests_cache import Optional
+
+from superset.commands.base import BaseCommand
+from superset.commands.dashboard.exceptions import (
+    DashboardUnfaveError,
+)
+from superset.daos.dashboard import DashboardDAO
+from superset.models.dashboard import Dashboard
+from superset.utils.decorators import on_error, transaction
+
+logger = logging.getLogger(__name__)
+
+
+class DelFavoriteDashboardCommand(BaseCommand):
+    def __init__(self, dashboard_id: int) -> None:
+        self._dashboard_id = dashboard_id
+        self._dashboard: Optional[Dashboard] = None
+
+    @transaction(on_error=partial(on_error, reraise=DashboardUnfaveError))
+    def run(self) -> None:
+        self.validate()
+        return DashboardDAO.remove_favorite(self._dashboard)
+
+    def validate(self) -> None:
+        # Raises DashboardNotFoundError or DashboardAccessDeniedError
+        dashboard = DashboardDAO.get_by_id_or_slug(self._dashboard_id)
+        self._dashboard = dashboard

superset/dashboards/api.py

@@ -51,8 +51,10 @@
     DashboardUpdateFailedError,
 )
 from superset.commands.dashboard.export import ExportDashboardsCommand
+from superset.commands.dashboard.fave import AddFavoriteDashboardCommand
 from superset.commands.dashboard.importers.dispatcher import ImportDashboardsCommand
 from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand
+from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand
 from superset.commands.dashboard.update import UpdateDashboardCommand
 from superset.commands.exceptions import TagForbiddenError
 from superset.commands.importers.exceptions import NoValidFilesFoundError
@@ -1212,11 +1214,14 @@ def add_favorite(self, pk: int) -> Response:
             500:
               $ref: '#/components/responses/500'
         """
-        dashboard = DashboardDAO.find_by_id(pk)
-        if not dashboard:
+        try:
+            AddFavoriteDashboardCommand(pk).run()
+
+        except DashboardNotFoundError:
             return self.response_404()
+        except DashboardAccessDeniedError:
+            return self.response_403()
 
-        DashboardDAO.add_favorite(dashboard)
         return self.response(200, result="OK")
 
     @expose("/<pk>/favorites/", methods=("DELETE",))
@@ -1255,11 +1260,13 @@ def remove_favorite(self, pk: int) -> Response:
             500:
               $ref: '#/components/responses/500'
         """
-        dashboard = DashboardDAO.find_by_id(pk)
-        if not dashboard:
+        try:
+            DelFavoriteDashboardCommand(pk).run()
+        except DashboardNotFoundError:
             return self.response_404()
+        except DashboardAccessDeniedError:
+            return self.response_403()
 
-        DashboardDAO.remove_favorite(dashboard)
         return self.response(200, result="OK")
 
     @expose("/import/", methods=("POST",))