From 8d109096f58064bdee6d567d0d92ea8c0976e7f1 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Fri, 18 Oct 2024 12:50:26 -0700
Subject: [PATCH] refactor: simplify REST API handling of usage keys

---
 .../tests/test_content_libraries.py           |  4 --
 openedx/core/djangoapps/xblock/api.py         |  5 +-
 .../xblock/rest_api/url_converters.py         | 23 ++++++++
 .../core/djangoapps/xblock/rest_api/urls.py   | 16 +++---
 .../core/djangoapps/xblock/rest_api/views.py  | 53 +++++--------------
 5 files changed, 48 insertions(+), 53 deletions(-)
 create mode 100644 openedx/core/djangoapps/xblock/rest_api/url_converters.py

diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py
index 03b32e08ad36..04cf96abbf36 100644
--- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py
+++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py
@@ -1101,10 +1101,6 @@ def test_invalid_key(self, endpoint, endpoint_parameters):
             endpoint.format(**endpoint_parameters),
         )
         self.assertEqual(response.status_code, 404)
-        msg = f"XBlock {endpoint_parameters.get('block_key')} does not exist, or you don't have permission to view it."
-        self.assertEqual(response.json(), {
-            'detail': msg,
-        })
 
     def test_xblock_handler_invalid_key(self):
         """This endpoint is tested separately from the previous ones as it's not a DRF endpoint."""
diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py
index 919b4b63ec4b..773bf81edb66 100644
--- a/openedx/core/djangoapps/xblock/api.py
+++ b/openedx/core/djangoapps/xblock/api.py
@@ -279,7 +279,6 @@ def get_handler_url(
 
     This view does not check/care if the XBlock actually exists.
     """
-    usage_key_str = str(usage_key)
     site_root_url = get_xblock_app_config().get_site_root_url()
     if not user:
         raise TypeError("Cannot get handler URLs without specifying a specific user ID.")
@@ -291,10 +290,10 @@ def get_handler_url(
         raise ValueError("Invalid user value")
     # Now generate a token-secured URL for this handler, specific to this user
     # and this XBlock:
-    secure_token = get_secure_token_for_xblock_handler(user_id, usage_key_str)
+    secure_token = get_secure_token_for_xblock_handler(user_id, str(usage_key))
     # Now generate the URL to that handler:
     kwargs = {
-        'usage_key_str': usage_key_str,
+        'usage_key': usage_key,
         'user_id': user_id,
         'secure_token': secure_token,
         'handler_name': handler_name,
diff --git a/openedx/core/djangoapps/xblock/rest_api/url_converters.py b/openedx/core/djangoapps/xblock/rest_api/url_converters.py
new file mode 100644
index 000000000000..4b660fdc0bf4
--- /dev/null
+++ b/openedx/core/djangoapps/xblock/rest_api/url_converters.py
@@ -0,0 +1,23 @@
+"""
+URL pattern converters
+https://docs.djangoproject.com/en/5.1/topics/http/urls/#registering-custom-path-converters
+"""
+from opaque_keys import InvalidKeyError
+from opaque_keys.edx.keys import UsageKeyV2
+
+
+class UsageKeyV2Converter:
+    """
+    Converter that matches V2 usage keys like:
+        lb:Org:LIB:drag-and-drop-v2:91c2b1d5
+    """
+    regex = r'[\w-]+(:[\w\-.]+)+'
+
+    def to_python(self, value):
+        try:
+            return UsageKeyV2.from_string(value)
+        except InvalidKeyError as exc:
+            raise ValueError from exc
+
+    def to_url(self, value):
+        return str(value)
diff --git a/openedx/core/djangoapps/xblock/rest_api/urls.py b/openedx/core/djangoapps/xblock/rest_api/urls.py
index 7bc61db144ba..884ca9cf37b3 100644
--- a/openedx/core/djangoapps/xblock/rest_api/urls.py
+++ b/openedx/core/djangoapps/xblock/rest_api/urls.py
@@ -1,7 +1,8 @@
 """
 URL configuration for the new XBlock API
 """
-from django.urls import include, path, re_path
+from django.urls import include, path, re_path, register_converter
+from .url_converters import UsageKeyV2Converter
 from . import views
 
 # Note that the exact same API URLs are used in Studio and the LMS, but the API
@@ -9,6 +10,8 @@
 # If necessary at some point in the future, these URLs could be duplicated into
 # urls_studio and urls_lms, and/or the views could be likewise duplicated.
 app_name = 'openedx.core.djangoapps.xblock.rest_api'
+    
+register_converter(UsageKeyV2Converter, "usage_v2")
 
 block_endpoints = [
     # get metadata about an XBlock:
@@ -16,9 +19,9 @@
     # get/post full json fields of an XBlock:
     path('fields/', views.BlockFieldsView.as_view()),
     # render one of this XBlock's views (e.g. student_view)
-    re_path(r'^view/(?P<view_name>[\w\-]+)/$', views.render_block_view),
+    path('view/<str:view_name>/', views.render_block_view),
     # get the URL needed to call this XBlock's handlers
-    re_path(r'^handler_url/(?P<handler_name>[\w\-]+)/$', views.get_handler_url),
+    path('handler_url/<str:handler_name>/', views.get_handler_url),
     # call one of this block's handlers
     re_path(
         r'^handler/(?P<user_id>\w+)-(?P<secure_token>\w+)/(?P<handler_name>[\w\-]+)/(?P<suffix>.+)?$',
@@ -30,12 +33,11 @@
 
 urlpatterns = [
     path('api/xblock/v2/', include([
-        # This pattern with the version must come first so '@' isn't included in usage_key_str below
-        path(r'xblocks/<str:usage_key_str>@<str:version>/', include(block_endpoints)),
-        path(r'xblocks/<str:usage_key_str>/', include(block_endpoints)),
+        path(r'xblocks/<usage_v2:usage_key>/', include(block_endpoints)),
+        path(r'xblocks/<usage_v2:usage_key>@<str:version>/', include(block_endpoints)),
     ])),
     # Non-API views (these return HTML, not JSON):
-    path('xblocks/v2/<str:usage_key_str>/', include([
+    path('xblocks/v2/<usage_v2:usage_key>/', include([
         # render one of this XBlock's views (e.g. student_view) for embedding in an iframe
         # NOTE: this endpoint is **unstable** and subject to changes after Sumac
         path('embed/<str:view_name>/', views.embed_block_view),
diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py
index 20fa926983ac..7d29e513ddd3 100644
--- a/openedx/core/djangoapps/xblock/rest_api/views.py
+++ b/openedx/core/djangoapps/xblock/rest_api/views.py
@@ -65,7 +65,7 @@ def parse_version_request(version_str: str | None) -> LatestVersion | int:
 @api_view(['GET'])
 @view_auth_classes(is_authenticated=False)
 @permission_classes((permissions.AllowAny, ))  # Permissions are handled at a lower level, by the learning context
-def block_metadata(request, usage_key_str):
+def block_metadata(request, usage_key, version=None):
     """
     Get metadata about the specified block.
 
@@ -74,10 +74,8 @@ def block_metadata(request, usage_key_str):
     * "include": a comma-separated list of keys to include.
       Valid keys are "index_dictionary" and "student_view_data".
     """
-    try:
-        usage_key = UsageKey.from_string(usage_key_str)
-    except InvalidKeyError as e:
-        raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
+    if version:
+        raise NotImplementedError()
 
     block = load_block(usage_key, request.user)
     includes = request.GET.get("include", "").split(",")
@@ -92,14 +90,12 @@ def block_metadata(request, usage_key_str):
 @api_view(['GET'])
 @view_auth_classes(is_authenticated=False)
 @permission_classes((permissions.AllowAny, ))  # Permissions are handled at a lower level, by the learning context
-def render_block_view(request, usage_key_str, view_name):
+def render_block_view(request, usage_key, view_name, version=None):
     """
     Get the HTML, JS, and CSS needed to render the given XBlock.
     """
-    try:
-        usage_key = UsageKey.from_string(usage_key_str)
-    except InvalidKeyError as e:
-        raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
+    if version:
+        raise NotImplementedError()
 
     try:
         block = load_block(usage_key, request.user)
@@ -116,17 +112,12 @@ def render_block_view(request, usage_key_str, view_name):
 @view_auth_classes(is_authenticated=False)
 @permission_classes((permissions.AllowAny, ))  # Permissions are handled at a lower level, by the learning context
 @xframe_options_exempt
-def embed_block_view(request, usage_key_str, view_name):
+def embed_block_view(request, usage_key, view_name):
     """
     Render the given XBlock in an <iframe>
 
     Unstable - may change after Sumac
     """
-    try:
-        usage_key = UsageKey.from_string(usage_key_str)
-    except InvalidKeyError as e:
-        raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
-
     # Check if a specific version has been requested
     version = parse_version_request(request.GET.get("version"))
 
@@ -162,17 +153,15 @@ def embed_block_view(request, usage_key_str, view_name):
 
 @api_view(['GET'])
 @view_auth_classes(is_authenticated=False)
-def get_handler_url(request, usage_key_str, handler_name):
+def get_handler_url(request, usage_key, handler_name, version=None):
     """
     Get an absolute URL which can be used (without any authentication) to call
     the given XBlock handler.
 
     The URL will expire but is guaranteed to be valid for a minimum of 2 days.
     """
-    try:
-        usage_key = UsageKey.from_string(usage_key_str)
-    except InvalidKeyError as e:
-        raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
+    if version:
+        raise NotImplementedError()
 
     handler_url = _get_handler_url(usage_key, handler_name, request.user)
     return Response({"handler_url": handler_url})
@@ -184,7 +173,7 @@ def get_handler_url(request, usage_key_str, handler_name):
 # and https://github.com/openedx/XBlock/pull/383 for context.
 @csrf_exempt
 @xframe_options_exempt
-def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, suffix=None, version=None):
+def xblock_handler(request, user_id, secure_token, usage_key, handler_name, suffix=None, version=None):
     """
     Run an XBlock's handler and return the result
 
@@ -192,16 +181,11 @@ def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name,
     auth token included in the URL (see below). As a result it can be exempt
     from CSRF, session auth, and JWT/OAuth.
     """
-    try:
-        usage_key = UsageKey.from_string(usage_key_str)
-    except InvalidKeyError as e:
-        raise Http404 from e
-
     # To support sandboxed XBlocks, custom frontends, and other use cases, we
     # authenticate requests using a secure token in the URL. see
     # openedx.core.djangoapps.xblock.utils.get_secure_hash_for_xblock_handler
     # for details and rationale.
-    if not validate_secure_token_for_xblock_handler(user_id, usage_key_str, secure_token):
+    if not validate_secure_token_for_xblock_handler(user_id, str(usage_key), secure_token):
         raise PermissionDenied("Invalid/expired auth token.")
     if request.user.is_authenticated:
         # The user authenticated twice, e.g. with session auth and the token.
@@ -265,14 +249,10 @@ class BlockFieldsView(APIView):
     """
 
     @atomic
-    def get(self, request, usage_key_str):
+    def get(self, request, usage_key):
         """
         retrieves the xblock, returning display_name, data, and metadata
         """
-        try:
-            usage_key = UsageKey.from_string(usage_key_str)
-        except InvalidKeyError as e:
-            raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
 
         # The "fields" view requires "read as author" permissions because the fields can contain answers, etc.
         block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_READ_AS_AUTHOR)
@@ -288,15 +268,10 @@ def get(self, request, usage_key_str):
         return Response(block_dict)
 
     @atomic
-    def post(self, request, usage_key_str):
+    def post(self, request, usage_key):
         """
         edits the xblock, saving changes to data and metadata only (display_name included in metadata)
         """
-        try:
-            usage_key = UsageKey.from_string(usage_key_str)
-        except InvalidKeyError as e:
-            raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e
-
         user = request.user
         block = load_block(usage_key, user, check_permission=CheckPerm.CAN_EDIT)
         data = request.data.get("data")