fix(tagging): Handle rename conflicts; Better 500 handling

This fixes two issues: 
1. Renaming a tag such that it conflicts with an existing tag should result
   in a 4xx and a helpful error message. 
2. Unexpected errors while handling requests should result in a simple
   500 error response without internal details (unless in debug mode)
   and a log message with exception details. This is default Django behavior.
   However DRF returns internal exception details, which is confusing to
   users and bad security practice.  This PR restores the default Django behavior
   for the Tagging API in particular. In the future, we'd like to generalize this handler
   to work for all our DRF APIs.

Fixes: openedx/modular-learning#261

Author: jesperhodge

src/openedx_core/__init__.py

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

src/openedx_tagging/rest_api/v1/exception_handlers.py

@@ -0,0 +1,58 @@
+"""
+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)
+
+    # DRF always calls exception handlers from within an `except:` block, so we can assume
+    # that `log.exception` will automatically insert those exception details and a stack trace.
+    log.exception("Unexpected exception while handling API request")
+
+    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.