chore(tags): Handle tagging as part of asset update call

superset-frontend/src/dashboard/components/PropertiesModal/index.tsx

@@ -44,12 +44,7 @@ import ColorSchemeControlWrapper from 'src/dashboard/components/ColorSchemeContr
 import FilterScopeModal from 'src/dashboard/components/filterscope/FilterScopeModal';
 import withToasts from 'src/components/MessageToasts/withToasts';
 import TagType from 'src/types/TagType';
-import {
-  addTag,
-  deleteTaggedObjects,
-  fetchTags,
-  OBJECT_TYPES,
-} from 'src/features/tags/tags';
+import { fetchTags, OBJECT_TYPES } from 'src/features/tags/tags';
 import { loadTags } from 'src/components/Tags/utils';
 
 const StyledFormItem = styled(FormItem)`
@@ -309,41 +304,6 @@ const PropertiesModal = ({
     setColorScheme(colorScheme);
   };
 
-  const updateTags = (oldTags: TagType[], newTags: TagType[]) => {
-    // update the tags for this object
-    // add tags that are in new tags, but not in old tags
-    // eslint-disable-next-line array-callback-return
-    newTags.map((tag: TagType) => {
-      if (!oldTags.some(t => t.name === tag.name)) {
-        addTag(
-          {
-            objectType: OBJECT_TYPES.DASHBOARD,
-            objectId: dashboardId,
-            includeTypes: false,
-          },
-          tag.name,
-          () => {},
-          () => {},
-        );
-      }
-    });
-    // delete tags that are in old tags, but not in new tags
-    // eslint-disable-next-line array-callback-return
-    oldTags.map((tag: TagType) => {
-      if (!newTags.some(t => t.name === tag.name)) {
-        deleteTaggedObjects(
-          {
-            objectType: OBJECT_TYPES.DASHBOARD,
-            objectId: dashboardId,
-          },
-          tag,
-          () => {},
-          () => {},
-        );
-      }
-    });
-  };
-
   const onFinish = () => {
     const { title, slug, certifiedBy, certificationDetails } =
       form.getFieldsValue();
@@ -401,31 +361,15 @@ const PropertiesModal = ({
       updateMetadata: false,
     });
 
-    if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
-      // update tags
-      try {
-        fetchTags(
-          {
-            objectType: OBJECT_TYPES.DASHBOARD,
-            objectId: dashboardId,
-            includeTypes: false,
-          },
-          (currentTags: TagType[]) => updateTags(currentTags, tags),
-          error => {
-            handleErrorResponse(error);
-          },
-        );
-      } catch (error) {
-        handleErrorResponse(error);
-      }
-    }
-
     const moreOnSubmitProps: { roles?: Roles } = {};
-    const morePutProps: { roles?: number[] } = {};
+    const morePutProps: { roles?: number[]; tags?: string[] } = {};
     if (isFeatureEnabled(FeatureFlag.DashboardRbac)) {
       moreOnSubmitProps.roles = roles;
       morePutProps.roles = (roles || []).map(r => r.id);
     }
+    if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
+      morePutProps.tags = (tags || []).map(r => r.name);
+    }
     const onSubmitProps = {
       id: dashboardId,
       title,

superset-frontend/src/explore/components/PropertiesModal/index.tsx

@@ -34,12 +34,7 @@ import {
 import Chart, { Slice } from 'src/types/Chart';
 import withToasts from 'src/components/MessageToasts/withToasts';
 import { loadTags } from 'src/components/Tags/utils';
-import {
-  addTag,
-  deleteTaggedObjects,
-  fetchTags,
-  OBJECT_TYPES,
-} from 'src/features/tags/tags';
+import { fetchTags, OBJECT_TYPES } from 'src/features/tags/tags';
 import TagType from 'src/types/TagType';
 
 export type PropertiesModalProps = {
@@ -144,41 +139,6 @@ function PropertiesModal({
     [],
   );
 
-  const updateTags = (oldTags: TagType[], newTags: TagType[]) => {
-    // update the tags for this object
-    // add tags that are in new tags, but not in old tags
-    // eslint-disable-next-line array-callback-return
-    newTags.map((tag: TagType) => {
-      if (!oldTags.some(t => t.name === tag.name)) {
-        addTag(
-          {
-            objectType: OBJECT_TYPES.CHART,
-            objectId: slice.slice_id,
-            includeTypes: false,
-          },
-          tag.name,
-          () => {},
-          () => {},
-        );
-      }
-    });
-    // delete tags that are in old tags, but not in new tags
-    // eslint-disable-next-line array-callback-return
-    oldTags.map((tag: TagType) => {
-      if (!newTags.some(t => t.name === tag.name)) {
-        deleteTaggedObjects(
-          {
-            objectType: OBJECT_TYPES.CHART,
-            objectId: slice.slice_id,
-          },
-          tag,
-          () => {},
-          () => {},
-        );
-      }
-    });
-  };
-
   const onSubmit = async (values: {
     certified_by?: string;
     certification_details?: string;
@@ -209,22 +169,7 @@ function PropertiesModal({
       ).map(o => o.value);
     }
     if (isFeatureEnabled(FeatureFlag.TaggingSystem)) {
-      // update tags
-      try {
-        fetchTags(
-          {
-            objectType: OBJECT_TYPES.CHART,
-            objectId: slice.slice_id,
-            includeTypes: false,
-          },
-          (currentTags: TagType[]) => updateTags(currentTags, tags),
-          error => {
-            showError(error);
-          },
-        );
-      } catch (error) {
-        showError(error);
-      }
+      payload.tags = tags.map(tag => tag.name);
     }
 
     try {

superset/charts/api.py

@@ -69,7 +69,7 @@
 from superset.commands.chart.importers.dispatcher import ImportChartsCommand
 from superset.commands.chart.update import UpdateChartCommand
 from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand
-from superset.commands.exceptions import CommandException
+from superset.commands.exceptions import CommandException, TagForbiddenError
 from superset.commands.importers.exceptions import (
     IncorrectFormatError,
     NoValidFilesFoundError,
@@ -404,6 +404,8 @@ def put(self, pk: int) -> Response:
             response = self.response_404()
         except ChartForbiddenError:
             response = self.response_403()
+        except TagForbiddenError as ex:
+            response = self.response(403, message=str(ex))
         except ChartInvalidError as ex:
             response = self.response_422(message=ex.normalized_messages())
         except ChartUpdateFailedError as ex:

superset/charts/schemas.py

@@ -122,6 +122,7 @@
 owners_name_description = "Name of an owner of the chart."
 certified_by_description = "Person or group that has certified this chart"
 certification_details_description = "Details of the certification"
+tags_description = "Tags to be associated with the chart"
 
 openapi_spec_methods_override = {
     "get": {"get": {"summary": "Get a chart detail information"}},
@@ -284,7 +285,9 @@ class ChartPutSchema(Schema):
     )
     is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
     external_url = fields.String(allow_none=True)
-    tags = fields.Nested(TagSchema, many=True)
+    tags = fields.List(
+        fields.String(metadata={"description": tags_description}, allow_none=True)
+    )
 
 
 class ChartGetDatasourceObjectDataResponseSchema(Schema):

superset/commands/chart/update.py

@@ -32,12 +32,13 @@
     DashboardsNotFoundValidationError,
     DatasourceTypeUpdateRequiredValidationError,
 )
-from superset.commands.utils import get_datasource_by_id
+from superset.commands.utils import get_datasource_by_id, update_tags, validate_tags
 from superset.daos.chart import ChartDAO
 from superset.daos.dashboard import DashboardDAO
-from superset.daos.exceptions import DAOUpdateFailedError
+from superset.daos.exceptions import DAODeleteFailedError, DAOUpdateFailedError
 from superset.exceptions import SupersetSecurityException
 from superset.models.slice import Slice
+from superset.tags.models import ObjectType
 
 logger = logging.getLogger(__name__)
 
@@ -59,11 +60,16 @@ def run(self) -> Model:
         assert self._model
 
         try:
+            # Update tags
+            tags = self._properties.pop("tags", None)
+            if tags is not None:
+                update_tags(ObjectType.chart, self._model.id, self._model.tags, tags)
+
             if self._properties.get("query_context_generation") is None:
                 self._properties["last_saved_at"] = datetime.now()
                 self._properties["last_saved_by"] = g.user
             chart = ChartDAO.update(self._model, self._properties)
-        except DAOUpdateFailedError as ex:
+        except (DAOUpdateFailedError, DAODeleteFailedError) as ex:
             logger.exception(ex.exception)
             raise ChartUpdateFailedError() from ex
         return chart
@@ -72,6 +78,7 @@ def validate(self) -> None:
         exceptions: list[ValidationError] = []
         dashboard_ids = self._properties.get("dashboards")
         owner_ids: Optional[list[int]] = self._properties.get("owners")
+        tag_names: Optional[list[str]] = self._properties.get("tags")
 
         # Validate if datasource_id is provided datasource_type is required
         datasource_id = self._properties.get("datasource_id")
@@ -100,6 +107,12 @@ def validate(self) -> None:
             except ValidationError as ex:
                 exceptions.append(ex)
 
+        # validate tags
+        try:
+            validate_tags(self._model.__class__.__name__, self._model.tags, tag_names)
+        except ValidationError as ex:
+            exceptions.append(ex)
+
         # Validate/Populate datasource
         if datasource_id is not None:
             try:

superset/commands/dashboard/update.py

@@ -30,12 +30,13 @@
     DashboardSlugExistsValidationError,
     DashboardUpdateFailedError,
 )
-from superset.commands.utils import populate_roles
+from superset.commands.utils import populate_roles, update_tags, validate_tags
 from superset.daos.dashboard import DashboardDAO
-from superset.daos.exceptions import DAOUpdateFailedError
+from superset.daos.exceptions import DAODeleteFailedError, DAOUpdateFailedError
 from superset.exceptions import SupersetSecurityException
 from superset.extensions import db
 from superset.models.dashboard import Dashboard
+from superset.tags.models import ObjectType
 
 logger = logging.getLogger(__name__)
 
@@ -51,6 +52,13 @@ def run(self) -> Model:
         assert self._model
 
         try:
+            # Update tags
+            tags = self._properties.pop("tags", None)
+            if tags is not None:
+                update_tags(
+                    ObjectType.dashboard, self._model.id, self._model.tags, tags
+                )
+
             dashboard = DashboardDAO.update(self._model, self._properties, commit=False)
             if self._properties.get("json_metadata"):
                 dashboard = DashboardDAO.set_dash_metadata(
@@ -59,7 +67,7 @@ def run(self) -> Model:
                     commit=False,
                 )
             db.session.commit()
-        except DAOUpdateFailedError as ex:
+        except (DAOUpdateFailedError, DAODeleteFailedError) as ex:
             logger.exception(ex.exception)
             raise DashboardUpdateFailedError() from ex
         return dashboard
@@ -69,6 +77,7 @@ def validate(self) -> None:
         owner_ids: Optional[list[int]] = self._properties.get("owners")
         roles_ids: Optional[list[int]] = self._properties.get("roles")
         slug: Optional[str] = self._properties.get("slug")
+        tag_names: Optional[list[str]] = self._properties.get("tags")
 
         # Validate/populate model exists
         self._model = DashboardDAO.find_by_id(self._model_id)
@@ -93,8 +102,12 @@ def validate(self) -> None:
             self._properties["owners"] = owners
         except ValidationError as ex:
             exceptions.append(ex)
-        if exceptions:
-            raise DashboardInvalidError(exceptions=exceptions)
+
+        # validate tags
+        try:
+            validate_tags(self._model.__class__.__name__, self._model.tags, tag_names)
+        except ValidationError as ex:
+            exceptions.append(ex)
 
         # Validate/Populate role
         if roles_ids is None:

superset/commands/exceptions.py

@@ -140,3 +140,13 @@ class QueryNotFoundValidationError(ValidationError):
 
     def __init__(self) -> None:
         super().__init__([_("Query does not exist")], field_name="datasource_id")
+
+
+class TagNotFoundValidationError(ValidationError):
+    def __init__(self, message: str) -> None:
+        super().__init__(message, field_name="tags")
+
+
+class TagForbiddenError(ForbiddenError):
+    def __init__(self, message: str) -> None:
+        super().__init__(message)