Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/openedx_core/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
"""

# The version for the entire repository
__version__ = "0.39.0"
__version__ = "0.39.1"
58 changes: 58 additions & 0 deletions src/openedx_tagging/rest_api/v1/exception_handlers.py
Original file line number Diff line number Diff line change
@@ -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
61 changes: 60 additions & 1 deletion src/openedx_tagging/rest_api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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]

Expand Down Expand Up @@ -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'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @jesperhodge , I only just thought of this after merging:

Do these validation strings appear in the end user UI? If so, they need to be internationalized.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me an example of how we do that?
As far as I see, we are not internationalizing a single error response in the whole repository.

I think since this is ubiquitous that would call for a new issue that has a bit wider scope and would include internationalizing other error messages like this. What do you think?

Is there any prior art in edx-platform?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I had a quick look at edx-platform.

It seems like that in edx-platform, ValidationError messages forwarded to the frontend on edx-platform are also not internationalized, for example https://github.com/openedx/openedx-platform/blob/77293cdf8e97eddcb3db32624b690dff8fe5d0df/lms/djangoapps/badges/models.py#L15

Copy link
Copy Markdown
Member

@kdmccormick kdmccormick Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jesperhodge

As far as I see, we are not internationalizing a single error response in the whole repository.

Ack, you're right, and it's worse than that--nothing in this repo is internationalized. And I guess we've noticed that before, too: #483 . No need for you to do anything here--we'll do a big sweep of internationalization at some point in the future. Thanks for checking on that.

would include internationalizing other error messages like this. What do you think?

I think the rule of thumb is "If the user will see it, it must be internationalized" rather than a blanket "all exceptions strings do/don't need to be internationalized".

Is there any prior art in edx-platform?

Yes, we use the standard gettext function for all translation. Usually we alias it to _:

from django.utils.translation import gettext as _

my_message = _("translate me")
my_template = _("hello {name}, translate me")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like that in edx-platform, ValidationError messages forwarded to the frontend on edx-platform are also not internationalized, for example https://github.com/openedx/openedx-platform/blob/77293cdf8e97eddcb3db32624b690dff8fe5d0df/lms/djangoapps/badges/models.py#L15

That message is internationalized. The _ function returns the localized string, if it available.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That message is internationalized. The _ function returns the localized string, if it available.

Oh! That's good

)

# 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.
Expand All @@ -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.
Expand Down Expand Up @@ -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
"""
Expand All @@ -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
"""
Expand All @@ -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
"""
Expand Down
16 changes: 9 additions & 7 deletions src/openedx_tagging/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.

Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -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")
Expand All @@ -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
Expand All @@ -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")
Expand All @@ -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
Expand All @@ -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")
Expand Down
4 changes: 3 additions & 1 deletion src/openedx_tagging/rest_api/v1/views_import.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
Loading