fix: introduce NoUpstream exception; more logging; more consistent HTTP API errors

Author: kdmccormick

cms/djangoapps/contentstore/helpers.py

@@ -382,16 +382,16 @@ def _import_xml_node_to_parent(
         raise NotImplementedError("We don't yet support pasting XBlocks with children")
     temp_xblock.parent = parent_key
     if copied_from_block:
-        # Store a reference to where this block was copied from, in the 'copied_from_block' field (AuthoringMixin)
-        temp_xblock.copied_from_block = copied_from_block
         # Try to link the pasted block (downstream) to the copied block (upstream).
         temp_xblock.upstream = copied_from_block
         try:
             UpstreamLink.get_for_block(temp_xblock)
         except (BadDownstream, BadUpstream):
             # Usually this will fail. For example, if the copied block is a modulestore course block, it can't be an
-            # upstream. That's fine; just don't link.
+            # upstream. That's fine! Instead, we store a reference to where this block was copied from, in the
+            # 'copied_from_block' field (from AuthoringMixin).
             temp_xblock.upstream = None
+            temp_xblock.copied_from_block = copied_from_block
         else:
             # But if it doesn't fail, then populate the `upstream_version` field based on what was copied. Note that
             # this could be the latest published version, or it could be an an even newer draft version.

cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py

@@ -294,7 +294,12 @@ def get(self, request: Request, usage_key_string: str):
                         "block_type": child_info.location.block_type,
                         "user_partition_info": user_partition_info,
                         "user_partitions": user_partitions,
-                        "upstream_link": upstream_link.to_json() if upstream_link else None,
+                        "upstream_link": (
+                            # If the block isn't linked to an upstream (which is by far the most common case) then just
+                            # make this field null, which communicates the same info, but with less noise.
+                            upstream_link.to_json() if upstream_link.upstream_ref
+                            else None
+                        ),
                         "validation_messages": validation_messages,
                         "render_error": render_error,
                     })

cms/djangoapps/contentstore/rest_api/v2/views/downstreams.py

@@ -1,66 +1,63 @@
 """
 API Views for managing & syncing links between upstream & downstream content
 
-Only [BETA] endpoints are implemented currently.
-The [FULL] endpoints should be implemented for the Full Libraries Relaunch, or removed from this doc.
-
-[FULL] List downstream blocks that can be synced, filterable by course or sync-readiness.
-    GET /api/v2/contentstore/downstreams
-    GET /api/v2/contentstore/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true
-      200: A paginated list of applicable & accessible downstream blocks.
-
-[BETA] Inspect a single downstream block's link to upstream content.
-    GET /api/v2/contentstore/downstreams/{usage_key_string}
-      200: Upstream link details successfully fetched.
-      404: Block not found, OR user lacks permission to read block.
-      400: Blocks is not linked to upstream content.
-
-[FULL] Sever a single downstream block's link to upstream content.
-    DELETE /api/v2/contentstore/downstreams/{usage_key_string}
-      204: Block successfully unlinked. No response body.
-      404: Block not found, OR user lacks permission to edit block
-      400: Blocks is not linked to upstream content.
-
-[BETA] Establish or modify a single downstream block's link to upstream content.
-    PUT /api/v2/contentstore/downstreams/{usage_key_string}
+API paths (We will move these into proper api_doc_tools annotations soon
+https://github.com/openedx/edx-platform/issues/35653):
+
+  /api/v2/contentstore/downstreams/{usage_key_string}
+
+    GET: Inspect a single downstream block's link to upstream content.
+      200: Upstream link details successfully fetched. Returns UpstreamLink (may contain an error_message).
+      404: Downstream block not found or user lacks permission to edit it.
+
+    DELETE: Sever a single downstream block's link to upstream content.
+      204: Block successfully unlinked (or it wasn't linked in the first place). No response body.
+      404: Downstream block not found or user lacks permission to edit it.
+
+    PUT: Establish or modify a single downstream block's link to upstream content. An authoring client could use this
+         endpoint to add library content in a two-step process, specifically: (1) add a blank block to a course, then
+         (2) link it to a content library with ?sync=True.
       REQUEST BODY: {
         "upstream_ref": str,  // reference to upstream block (eg, library block usage key)
         "sync": bool,  // whether to sync in upstream content (False by default)
       }
-      200: Block's upstream link successfully edited (and synced, if requested).
-      404: Block not found, OR user lacks permission to edit block
+      200: Downstream block's upstream link successfully edited (and synced, if requested). Returns UpstreamLink.
       400: upstream_ref is malformed, missing, or inaccessible.
-      400: Upstream block does not support syncing.
+      400: Content at upstream_ref does not support syncing.
+      404: Downstream block not found or user lacks permission to edit it.
+
+  /api/v2/contentstore/downstreams/{usage_key_string}/sync
 
-[BETA] Sync a downstream block with upstream content.
-    POST /api/v2/contentstore/downstreams/{usage_key_string}/sync
-      200: Block successfully synced with upstream content.
-      404: Block is not linked to upstream, OR block not found, OR user lacks permission to edit block.
-      400: Blocks is not linked to upstream content.
+    POST: Sync a downstream block with upstream content.
+      200: Downstream block successfully synced with upstream content.
+      400: Downstream block is not linked to upstream content.
       400: Upstream is malformed, missing, or inaccessible.
       400: Upstream block does not support syncing.
+      404: Downstream block not found or user lacks permission to edit it.
 
-[BETA] Decline an available sync for a downstream block.
-    DELETE /api/v2/contentstore/downstreams/{usage_key_string}/sync
+    DELETE: Decline an available sync for a downstream block.
       204: Sync successfuly dismissed. No response body.
-      404: Block not found, OR user lacks permission to edit block.
-      400: Blocks is not linked to upstream content.
+      400: Downstream block is not linked to upstream content.
+      404: Downstream block not found or user lacks permission to edit it.
 
-Schema for 200 responses, except where noted:
-  {
-      "upstream_ref": string?
-      "version_synced": string?,
-      "version_available": string?,
-      "version_declined": string?,
-      "error_message": string?,
-      "ready_to_sync": Boolean
-  }
+  # NOT YET IMPLEMENTED -- Will be needed for full Libraries Relaunch in ~Teak.
+  /api/v2/contentstore/downstreams
+  /api/v2/contentstore/downstreams?course_id=course-v1:A+B+C&ready_to_sync=true
+      GET: List downstream blocks that can be synced, filterable by course or sync-readiness.
+        200: A paginated list of applicable & accessible downstream blocks. Entries are UpstreamLinks.
 
-Schema for 4XX responses:
+UpstreamLink response schema:
   {
-      "developer_message": string?
+    "upstream_ref": string?
+    "version_synced": string?,
+    "version_available": string?,
+    "version_declined": string?,
+    "error_message": string?,
+    "ready_to_sync": Boolean
   }
 """
+import logging
+
 from django.contrib.auth.models import User  # pylint: disable=imported-auth-user
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import UsageKey
@@ -71,7 +68,8 @@
 from xblock.core import XBlock
 
 from cms.lib.xblock.upstream_sync import (
-    UpstreamLink, sync_from_upstream, decline_sync, BadUpstream, BadDownstream, fetch_customizable_fields
+    UpstreamLink, UpstreamLinkException, NoUpstream, BadUpstream, BadDownstream,
+    fetch_customizable_fields, sync_from_upstream, decline_sync, sever_upstream_link
 )
 from common.djangoapps.student.auth import has_studio_write_access, has_studio_read_access
 from openedx.core.lib.api.view_utils import (
@@ -82,6 +80,9 @@
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
 
+logger = logging.getLogger(__name__)
+
+
 class _AuthenticatedRequest(Request):
     """
     Alias for the `Request` class which tells mypy to assume that `.user` is not an AnonymousUser.
@@ -117,19 +118,14 @@ def get(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response
         Inspect an XBlock's link to upstream content.
         """
         downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=False)
-        _ensure_upstream_ref(downstream)
-        if link := UpstreamLink.try_get_for_block(downstream):
-            return Response(link.to_json())
-        raise ValidationError(detail=f"Block '{usage_key_string}' is not linked to an upstream")
+        return Response(UpstreamLink.try_get_for_block(downstream).to_json())
 
     def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
         """
         Edit an XBlock's link to upstream content.
         """
         downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
         new_upstream_ref = request.data.get("upstream_ref")
-        if not isinstance(new_upstream_ref, str):
-            raise ValidationError({"upstream_ref": "value missing"})
 
         # Set `downstream.upstream` so that we can try to sync and/or fetch.
         # Note that, if this fails and we raise a 4XX, then we will not call modulstore().update_item,
@@ -142,22 +138,47 @@ def put(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response
             if sync_param == "true":
                 sync_from_upstream(downstream=downstream, user=request.user)
             else:
+                # Even if we're not syncing (i.e., updating the downstream's values with the upstream's), we still need
+                # to fetch the upstream's customizable values and store them as hidden fields on the downstream. This
+                # ensures that downstream authors can restore defaults based on the upstream.
                 fetch_customizable_fields(downstream=downstream, user=request.user)
         except BadDownstream as exc:
+            logger.exception(
+                "'%s' is an invalid downstream; refusing to set its upstream to '%s'",
+                usage_key_string,
+                new_upstream_ref,
+            )
             raise ValidationError(str(exc)) from exc
         except BadUpstream as exc:
+            logger.exception(
+                "'%s' is an invalid upstream reference; refusing to set it as upstream of '%s'",
+                new_upstream_ref,
+                usage_key_string,
+            )
             raise ValidationError({"upstream_ref": str(exc)}) from exc
+        except NoUpstream as exc:
+            raise ValidationError({"upstream_ref": "value missing"}) from exc
         modulestore().update_item(downstream, request.user.id)
-        link: UpstreamLink = UpstreamLink.get_for_block(downstream)  # type: ignore[assignment]
-        return Response(link.to_json())
+        # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the
+        #       upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen.
+        return Response(UpstreamLink.get_for_block(downstream).to_json())
 
-    # def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
-    #     """
-    #     Sever an XBlock's link to upstream content.
-    #     """
-    #     downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
-    #     _ensure_upstream_ref(downstream)
-    #     ....
+    def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
+        """
+        Sever an XBlock's link to upstream content.
+        """
+        downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
+        try:
+            sever_upstream_link(downstream)
+        except NoUpstream as exc:
+            logger.exception(
+                "Tried to DELETE upstream link of '%s', but it wasn't linked to anything in the first place. "
+                "Will do nothing. ",
+                usage_key_string,
+            )
+        else:
+            modulestore().update_item(downstream, request.user.id)
+        return Response(status=204)
 
 
 @view_auth_classes(is_authenticated=True)
@@ -171,27 +192,37 @@ def post(self, request: _AuthenticatedRequest, usage_key_string: str) -> Respons
         Pull latest updates to the block at {usage_key_string} from its linked upstream content.
         """
         downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
-        _ensure_upstream_ref(downstream)
-        if not downstream.upstream:
-            raise NotFound(detail=f"Block '{usage_key_string}' is not linked to a library block")
-        old_version = downstream.upstream_version
         try:
             sync_from_upstream(downstream, request.user)
-        except (BadUpstream, BadDownstream) as exc:
+        except UpstreamLinkException as exc:
+            logger.exception(
+                "Could not sync from upstream '%s' to downstream '%s'",
+                downstream.upstream,
+                usage_key_string,
+            )
             raise ValidationError(detail=str(exc)) from exc
         modulestore().update_item(downstream, request.user.id)
-        link: UpstreamLink = UpstreamLink.get_for_block(downstream)  # type: ignore[assignment]
-        return Response(link.to_json(), status=200)
+        # Note: We call `get_for_block` (rather than `try_get_for_block`) because if anything is wrong with the
+        #       upstream at this point, then that is completely unexpected, so it's appropriate to let the 500 happen.
+        return Response(UpstreamLink.get_for_block(downstream).to_json())
 
     def delete(self, request: _AuthenticatedRequest, usage_key_string: str) -> Response:
         """
         Decline the latest updates to the block at {usage_key_string}.
         """
         downstream = _load_accessible_block(request.user, usage_key_string, require_write_access=True)
-        _ensure_upstream_ref(downstream)
         try:
             decline_sync(downstream)
-        except (BadUpstream, BadDownstream) as exc:
+        except (NoUpstream, BadUpstream, BadDownstream) as exc:
+            # This is somewhat unexpected. If the upstream link is missing or invalid, then the downstream author
+            # shouldn't have been prompted to accept/decline a sync in the first place. Of course, they could have just
+            # hit the HTTP API anyway, or they could be viewing a Studio page which hasn't been refreshed in a while.
+            # So, it's a 400, not a 500.
+            logger.exception(
+                "Tried to decline a sync to downstream '%s', but the upstream link '%s' is invalid.",
+                usage_key_string,
+                downstream.upstream,
+            )
             raise ValidationError(str(exc)) from exc
         modulestore().update_item(downstream, request.user.id)
         return Response(status=204)
@@ -202,9 +233,7 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a
     Given a logged in-user and a serialized usage key of an upstream-linked XBlock, load it from the ModuleStore,
     raising a DRF-friendly exception if anything goes wrong.
 
-    Raises NotFound if usage key is malformed.
-    Raises NotFound if user lacks access.
-    Raises NotFound if block does not exist.
+    Raises NotFound if usage key is malformed, if the user lacks access, or if the block doesn't exist.
     """
     not_found = NotFound(detail=f"Block not found or not accessible: {usage_key_string}")
     try:
@@ -220,11 +249,3 @@ def _load_accessible_block(user: User, usage_key_string: str, *, require_write_a
     except ItemNotFoundError as exc:
         raise not_found from exc
     return block
-
-
-def _ensure_upstream_ref(block: XBlock) -> None:
-    """
-    Raises ValidationError if block is not a downstream block.
-    """
-    if not block.upstream:
-        raise ValidationError(detail=f"Block '{block.usage_key}' is not linked to a library block")