fix: handle unexpected error and duplicates

Author: jesperhodge

src/openedx_core/__init__.py

@@ -6,4 +6,4 @@
 """
 
 # The version for the entire repository
-__version__ = "0.38.3"
+__version__ = "0.38.4"

src/openedx_tagging/rest_api/v1/exception_handlers.py

@@ -0,0 +1,57 @@
+"""
+Tagging REST API exception handling utilities.
+"""
+
+import logging
+import traceback
+
+from django.conf import settings
+from django.http import Http404
+from rest_framework import status
+from rest_framework.exceptions import APIException, PermissionDenied
+from rest_framework.response import Response
+from rest_framework.views import exception_handler
+
+log = logging.getLogger(__name__)
+
+
+def _custom_exception_handler(exc: Exception, context: dict) -> Response | None:
+    """
+    Return standard DRF errors for APIException and a generic 500 otherwise.
+    This exception handler should eventually be replaced by a more top-level
+    exception handler in the openedx-platform repo after the ADR for it is accepted:
+    https://github.com/openedx/openedx-platform/pull/38246
+    """
+    # For exceptions expected by DRF return the standard DRF error response:
+    # Instances of APIException, subclasses of APIException, Django's Http404 exception,
+    # and Django's PermissionDenied exception.
+    is_expected_exception = isinstance(
+        exc, (APIException, Http404, PermissionDenied)
+    )
+    if is_expected_exception:
+        return exception_handler(exc, context)
+
+    # Making really sure that any unexpected exception gets logged
+    log.error(exc, stack_info=True, exc_info=True)
+
+    if settings.DEBUG:
+        description_with_traceback = f"{exc.__class__.__name__}: {str(exc)}\n\nTraceback:\n{traceback.format_exc()}"
+
+        return Response(
+            {"detail": description_with_traceback},
+            status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+        )
+
+    return Response(
+        {"detail": "An unexpected error occurred while processing the request."},
+        status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+    )
+
+
+class TaggingExceptionHandlerMixin:
+    """
+    Scope custom exception handling to tagging API views only.
+    """
+
+    def get_exception_handler(self):
+        return _custom_exception_handler

src/openedx_tagging/rest_api/v1/serializers.py

@@ -13,6 +13,7 @@
 from openedx_tagging.data import TagData
 from openedx_tagging.import_export.parsers import ParserFormat
 from openedx_tagging.models import ObjectTag, Tag, TagImportTask, Taxonomy
+from openedx_tagging.models.utils import RESERVED_TAG_CHARS
 from openedx_tagging.rules import ObjectTagPermissionItem
 
 from ..utils import UserPermissionsHelper
@@ -59,7 +60,7 @@ def _model(self) -> Type:
     @property
     def _request(self) -> Request:
         """
-        Returns the current request from the serialize context.
+        Returns the current request from the serializer context.
         """
         return self.context.get('request')  # type: ignore[attr-defined]
 
@@ -217,6 +218,27 @@ class ObjectTagUpdateBodySerializer(serializers.Serializer):  # pylint: disable=
     tagsData = serializers.ListField(child=ObjectTagUpdateByTaxonomySerializer(), required=True)
 
 
+def validate_tag_value(value, context):
+    """
+    Validate this tag value is unique within the current taxonomy context and
+    does not contain forbidden characters.
+    """
+    taxonomy_id = context.get("taxonomy_id")
+    if taxonomy_id is not None:
+        # Check if tag value already exists within this taxonomy. If so, raise a validation error.
+        queryset = Tag.objects.filter(taxonomy_id=taxonomy_id, value=value)
+        if queryset.exists():
+            raise serializers.ValidationError(
+                f'Tag value "{value}" already exists in this taxonomy.', code='unique'
+            )
+
+    # validator checks there are no forbidden characters ">" or ";":
+    for char in value:
+        if char in RESERVED_TAG_CHARS:
+            raise serializers.ValidationError('Tag values cannot contain "\t" or ">" or ";" characters.')
+    return value
+
+
 class TagDataSerializer(UserPermissionsSerializerMixin, serializers.Serializer):  # pylint: disable=abstract-method
     """
     Serializer for TagData dicts. Also can serialize Tag instances.
@@ -237,6 +259,12 @@ class TagDataSerializer(UserPermissionsSerializerMixin, serializers.Serializer):
     can_change_tag = serializers.SerializerMethodField()
     can_delete_tag = serializers.SerializerMethodField()
 
+    def validate_value(self, value):
+        """
+        Runs validations for the tag value.
+        """
+        return validate_tag_value(value, self.context)
+
     def get_sub_tags_url(self, obj: TagData | Tag):
         """
         Returns URL for the list of child tags of the current tag.
@@ -303,6 +331,12 @@ class TaxonomyTagCreateBodySerializer(serializers.Serializer):  # pylint: disabl
     parent_tag_value = serializers.CharField(required=False)
     external_id = serializers.CharField(required=False)
 
+    def validate_tag(self, value):
+        """
+        Run validations for the tag value.
+        """
+        return validate_tag_value(value, self.context)
+
 
 class TaxonomyTagUpdateBodySerializer(serializers.Serializer):  # pylint: disable=abstract-method
     """
@@ -312,6 +346,12 @@ class TaxonomyTagUpdateBodySerializer(serializers.Serializer):  # pylint: disabl
     tag = serializers.CharField(required=True)
     updated_tag_value = serializers.CharField(required=True)
 
+    def validate_updated_tag_value(self, value):
+        """
+        Run validations for the updated tag value.
+        """
+        return validate_tag_value(value, self.context)
+
 
 class TaxonomyTagDeleteBodySerializer(serializers.Serializer):  # pylint: disable=abstract-method
     """
@@ -323,6 +363,25 @@ class TaxonomyTagDeleteBodySerializer(serializers.Serializer):  # pylint: disabl
     )
     with_subtags = serializers.BooleanField(required=False)
 
+    def validate_tags(self, tags_list):
+        """
+        Make sure all tags are valid and exist before attempting deletion, to avoid partial deletes.
+        """
+        # Iterate through the list and make one bulk request that checks whether every tag.value exists
+        taxonomy_id = self.context.get("taxonomy_id")
+        existing_tags = set(
+            Tag.objects.filter(taxonomy_id=taxonomy_id, value__in=tags_list)
+            .values_list("value", flat=True)
+        )
+        missing_tags = set(tags_list) - existing_tags
+
+        if missing_tags:
+            raise serializers.ValidationError(
+                f"Deletion aborted. The following tags do not exist and cannot be deleted:"
+                f" {', '.join(missing_tags)}"
+            )
+        return tags_list
+
 
 class TaxonomyImportBodySerializer(serializers.Serializer):  # pylint: disable=abstract-method
     """

src/openedx_tagging/rest_api/v1/views.py

@@ -34,6 +34,7 @@
 from ...rules import ObjectTagPermissionItem
 from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination, TaxonomyPagination
 from ..utils import view_auth_classes
+from .exception_handlers import TaggingExceptionHandlerMixin
 from .permissions import ObjectTagObjectPermissions, TaxonomyObjectPermissions, TaxonomyTagsObjectPermissions
 from .serializers import (
     ObjectTagListQueryParamsSerializer,
@@ -55,7 +56,7 @@
 
 
 @view_auth_classes
-class TaxonomyView(ModelViewSet):
+class TaxonomyView(TaggingExceptionHandlerMixin, ModelViewSet):
     """
     View to list, create, retrieve, update, delete, export or import Taxonomies.
 
@@ -640,7 +641,7 @@ def retrieve(self, request, *args, **kwargs) -> Response:
 
 
 @view_auth_classes
-class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView):
+class TaxonomyTagsView(TaggingExceptionHandlerMixin, ListAPIView, RetrieveUpdateDestroyAPIView):
     """
     View to list/create/update/delete tags of a taxonomy.
 
@@ -865,7 +866,8 @@ def post(self, request, *args, **kwargs):
         """
         taxonomy = self.get_taxonomy()
 
-        body = TaxonomyTagCreateBodySerializer(data=request.data)
+        serializer_context = self.get_serializer_context()
+        body = TaxonomyTagCreateBodySerializer(data=request.data, context=serializer_context)
         body.is_valid(raise_exception=True)
 
         tag = body.data.get("tag")
@@ -881,7 +883,6 @@ def post(self, request, *args, **kwargs):
         except ValueError as e:
             raise ValidationError(e) from e
 
-        serializer_context = self.get_serializer_context()
         return Response(
             self.serializer_class(new_tag, context=serializer_context).data,
             status=status.HTTP_201_CREATED
@@ -894,7 +895,8 @@ def update(self, request, *args, **kwargs):
         """
         taxonomy = self.get_taxonomy()
 
-        body = TaxonomyTagUpdateBodySerializer(data=request.data)
+        serializer_context = self.get_serializer_context()
+        body = TaxonomyTagUpdateBodySerializer(data=request.data, context=serializer_context)
         body.is_valid(raise_exception=True)
 
         tag = body.data.get("tag")
@@ -907,7 +909,6 @@ def update(self, request, *args, **kwargs):
         except ValueError as e:
             raise ValidationError(e) from e
 
-        serializer_context = self.get_serializer_context()
         return Response(
             self.serializer_class(updated_tag, context=serializer_context).data,
             status=status.HTTP_200_OK
@@ -921,7 +922,8 @@ def delete(self, request, *args, **kwargs):
         """
         taxonomy = self.get_taxonomy()
 
-        body = TaxonomyTagDeleteBodySerializer(data=request.data)
+        serializer_context = self.get_serializer_context()
+        body = TaxonomyTagDeleteBodySerializer(data=request.data, context=serializer_context)
         body.is_valid(raise_exception=True)
 
         tags = body.data.get("tags")

src/openedx_tagging/rest_api/v1/views_import.py

@@ -9,8 +9,10 @@
 from rest_framework.request import Request
 from rest_framework.views import APIView
 
+from .exception_handlers import TaggingExceptionHandlerMixin
 
-class TemplateView(APIView):
+
+class TemplateView(TaggingExceptionHandlerMixin, APIView):
     """
     View which serves the static Taxonomy Import template files.