From 7cd490ce383e41f712138042cce16638ab6747bd Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 2 Jan 2025 15:35:36 -0500 Subject: [PATCH 1/3] build: Unpin mypy and upgrade it to latest This is possible now that django-stubs and djangorestframework-stubs are unpinned: https://github.com/openedx/edx-platform/commit/a5b773ce7b9d5b1c7fd5e72eadef895a4072917b which became possible once we upgraded to django 4.2. Closes: https://github.com/openedx/edx-platform/issues/35667 --- requirements/constraints.txt | 6 ------ requirements/edx/development.txt | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 4258dd8f15f1..a985952c8a3b 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -112,12 +112,6 @@ markdown<3.4.0 # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35270 moto<5.0 -# Date: 2024-10-16 -# MyPY 1.12.0 fails on all PRs with the following error: -# openedx/core/djangoapps/content_libraries/api.py:732: error: INTERNAL ERROR -# Issue for unpinning: https://github.com/openedx/edx-platform/issues/35667 -mypy<1.12.0 - # Date: 2024-07-16 # We need to upgrade the version of elasticsearch to atleast 7.15 before we can upgrade to Numpy 2.0.0 # Otherwise we see a failure while running the following command: diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 72522de5db01..7778ca937b26 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1292,7 +1292,7 @@ multidict==6.1.0 # -r requirements/edx/testing.txt # aiohttp # yarl -mypy==1.11.2 +mypy==1.14.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/development.in From 424616c3d228c1a7e3688e37bbc174bc869cacda Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Tue, 7 Jan 2025 10:18:46 -0500 Subject: [PATCH 2/3] build: Upgrade django-stubs even further, to fix mypy Rather than constraining django-stubs' major version to our django major version (4.x.x), we are going to go one ahead (5.x.x), as recommended by https://github.com/python/mypy/issues/17958 Also includes an unrelated common_constraints update. --- requirements/constraints.txt | 8 +++++--- requirements/edx/development.txt | 11 ++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index a985952c8a3b..fcea298aedae 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -61,11 +61,13 @@ django-webpack-loader==0.7.0 djangorestframework<3.15.0 # Date: 2024-07-19 -# Generally speaking, the major version of django-stubs should match the major version of django. -# Specifically, we need to perpetually constrain django-stubs to a compatible version based on: +# Generally speaking, the major version of django-stubs must either match the major version +# of django, or exceed it by 1. So, we will need to perpetually constrain django-stubs and +# update it as we perform django upgrades. For more details, see: # https://github.com/typeddjango/django-stubs?tab=readme-ov-file#version-compatibility +# including the note on "Partial Support". # Issue: https://github.com/openedx/edx-platform/issues/35275 -django-stubs<5 +django-stubs<6 # Date: 2024-07-23 # django-storages==1.14.4 breaks course imports diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 7778ca937b26..1e81b10f47be 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -76,6 +76,7 @@ asgiref==3.8.1 # django # django-cors-headers # django-countries + # django-stubs asn1crypto==1.5.1 # via # -r requirements/edx/doc.txt @@ -577,7 +578,7 @@ django-storages==1.14.3 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edxval -django-stubs==4.2.7 +django-stubs==5.1.1 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/development.in @@ -625,7 +626,7 @@ djangorestframework==3.14.0 # openedx-learning # ora2 # super-csv -djangorestframework-stubs==3.14.5 +djangorestframework-stubs==3.15.2 # via -r requirements/edx/development.in djangorestframework-xml==2.0.0 # via @@ -1293,9 +1294,7 @@ multidict==6.1.0 # aiohttp # yarl mypy==1.14.1 - # via - # -c requirements/edx/../constraints.txt - # -r requirements/edx/development.in + # via -r requirements/edx/development.in mypy-extensions==1.0.0 # via mypy mysqlclient==2.2.6 @@ -2119,8 +2118,6 @@ tqdm==4.67.1 # -r requirements/edx/testing.txt # nltk # openai -types-pytz==2024.2.0.20241221 - # via django-stubs types-pyyaml==6.0.12.20241230 # via # django-stubs From 69bcbacd757ad6f3cb15dc77acbcdce227b4724a Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Tue, 7 Jan 2025 11:32:24 -0500 Subject: [PATCH 3/3] build: Fix type annotations for new mypy version Includes some new Request type annotations in openedx.core.types.http, plus a new meta-utility @type_annotation_only to ensure that we don't accidentally start instantiating those new classes. --- .../djangoapps/content_libraries/views.py | 5 ++- .../content_libraries/views_collections.py | 17 +++---- .../core/djangoapps/content_staging/api.py | 4 +- .../core/djangoapps/content_staging/models.py | 4 +- .../content_tagging/rest_api/v1/views.py | 7 +-- openedx/core/types/http.py | 45 +++++++++++++++++++ openedx/core/types/meta.py | 37 +++++++++++++++ openedx/core/types/user.py | 6 ++- 8 files changed, 107 insertions(+), 18 deletions(-) create mode 100644 openedx/core/types/http.py create mode 100644 openedx/core/types/meta.py diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index c8c91d50331f..4c14651c7961 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -122,6 +122,7 @@ from openedx.core.lib.api.view_utils import view_auth_classes from openedx.core.djangoapps.safe_sessions.middleware import mark_user_change_as_expected from openedx.core.djangoapps.xblock import api as xblock_api +from openedx.core.types.http import RestRequest from .models import ContentLibrary, LtiGradedResource, LtiProfile @@ -667,7 +668,7 @@ class LibraryBlockCollectionsView(APIView): View to set collections for a component. """ @convert_exceptions - def patch(self, request, usage_key_str) -> Response: + def patch(self, request: RestRequest, usage_key_str) -> Response: """ Sets Collections for a Component. @@ -688,7 +689,7 @@ def patch(self, request, usage_key_str) -> Response: library_key=key.lib_key, component=component, collection_keys=collection_keys, - created_by=self.request.user.id, + created_by=request.user.id, content_library=content_library, ) diff --git a/openedx/core/djangoapps/content_libraries/views_collections.py b/openedx/core/djangoapps/content_libraries/views_collections.py index b6c1c999ba94..21c4b12dd3da 100644 --- a/openedx/core/djangoapps/content_libraries/views_collections.py +++ b/openedx/core/djangoapps/content_libraries/views_collections.py @@ -25,6 +25,7 @@ ContentLibraryCollectionComponentsUpdateSerializer, ContentLibraryCollectionUpdateSerializer, ) +from openedx.core.types.http import RestRequest class LibraryCollectionsView(ModelViewSet): @@ -89,7 +90,7 @@ def get_object(self) -> Collection: return collection @convert_exceptions - def retrieve(self, request, *args, **kwargs) -> Response: + def retrieve(self, request: RestRequest, *args, **kwargs) -> Response: """ Retrieve the Content Library Collection """ @@ -97,7 +98,7 @@ def retrieve(self, request, *args, **kwargs) -> Response: return super().retrieve(request, *args, **kwargs) @convert_exceptions - def list(self, request, *args, **kwargs) -> Response: + def list(self, request: RestRequest, *args, **kwargs) -> Response: """ List Collections that belong to Content Library """ @@ -105,7 +106,7 @@ def list(self, request, *args, **kwargs) -> Response: return super().list(request, *args, **kwargs) @convert_exceptions - def create(self, request, *args, **kwargs) -> Response: + def create(self, request: RestRequest, *args, **kwargs) -> Response: """ Create a Collection that belongs to a Content Library """ @@ -139,7 +140,7 @@ def create(self, request, *args, **kwargs) -> Response: return Response(serializer.data) @convert_exceptions - def partial_update(self, request, *args, **kwargs) -> Response: + def partial_update(self, request: RestRequest, *args, **kwargs) -> Response: """ Update a Collection that belongs to a Content Library """ @@ -161,7 +162,7 @@ def partial_update(self, request, *args, **kwargs) -> Response: return Response(serializer.data) @convert_exceptions - def destroy(self, request, *args, **kwargs) -> Response: + def destroy(self, request: RestRequest, *args, **kwargs) -> Response: """ Soft-deletes a Collection that belongs to a Content Library """ @@ -176,7 +177,7 @@ def destroy(self, request, *args, **kwargs) -> Response: @convert_exceptions @action(detail=True, methods=['post'], url_path='restore', url_name='collection-restore') - def restore(self, request, *args, **kwargs) -> Response: + def restore(self, request: RestRequest, *args, **kwargs) -> Response: """ Restores a soft-deleted Collection that belongs to a Content Library """ @@ -191,7 +192,7 @@ def restore(self, request, *args, **kwargs) -> Response: @convert_exceptions @action(detail=True, methods=['delete', 'patch'], url_path='components', url_name='components-update') - def update_components(self, request, *args, **kwargs) -> Response: + def update_components(self, request: RestRequest, *args, **kwargs) -> Response: """ Adds (PATCH) or removes (DELETE) Components to/from a Collection. @@ -209,7 +210,7 @@ def update_components(self, request, *args, **kwargs) -> Response: content_library=content_library, collection_key=collection_key, usage_keys=usage_keys, - created_by=self.request.user.id, + created_by=request.user.id, remove=(request.method == "DELETE"), ) diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index f0432922dcb0..5f85d701faa5 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -66,7 +66,7 @@ def save_xblock_to_user_clipboard(block: XBlock, user_id: int, version_num: int olx=block_data.olx_str, display_name=block_metadata_utils.display_name_with_default(block), suggested_url_name=usage_key.block_id, - tags=block_data.tags, + tags=block_data.tags or {}, version_num=(version_num or 0), ) (clipboard, _created) = _UserClipboard.objects.update_or_create(user_id=user_id, defaults={ @@ -209,7 +209,7 @@ def _user_clipboard_model_to_data(clipboard: _UserClipboard) -> UserClipboardDat status=content.status, block_type=content.block_type, display_name=content.display_name, - tags=content.tags, + tags=content.tags or {}, version_num=content.version_num, ), source_usage_key=clipboard.source_usage_key, diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index 2eab7954e826..5e007bc4485a 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -67,7 +67,9 @@ class Meta: version_num = models.PositiveIntegerField(default=0) # Tags applied to the original source block(s) will be copied to the new block(s) on paste. - tags = models.JSONField(null=True, help_text=_("Content tags applied to these blocks")) + tags: models.JSONField[dict | None, dict | None] = models.JSONField( + null=True, help_text=_("Content tags applied to these blocks") + ) @property def olx_filename(self) -> str: diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index c2f79ef677db..615406ffccc5 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -10,7 +10,6 @@ from rest_framework import status from rest_framework.decorators import action from rest_framework.exceptions import PermissionDenied, ValidationError -from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView from openedx_events.content_authoring.data import ContentObjectData, ContentObjectChangedData @@ -19,6 +18,8 @@ CONTENT_OBJECT_TAGS_CHANGED, ) +from openedx.core.types.http import RestRequest + from ...auth import has_view_object_tags_access from ...api import ( create_taxonomy, @@ -99,7 +100,7 @@ def perform_create(self, serializer): serializer.instance = create_taxonomy(**serializer.validated_data, orgs=user_admin_orgs) @action(detail=False, url_path="import", methods=["post"]) - def create_import(self, request: Request, **kwargs) -> Response: # type: ignore + def create_import(self, request: RestRequest, **kwargs) -> Response: # type: ignore """ Creates a new taxonomy with the given orgs and imports the tags from the uploaded file. """ @@ -183,7 +184,7 @@ class ObjectTagExportView(APIView): """" View to export a CSV with all children and tags for a given course/context. """ - def get(self, request: Request, **kwargs) -> StreamingHttpResponse: + def get(self, request: RestRequest, **kwargs) -> StreamingHttpResponse: """ Export a CSV with all children and tags for a given course/context. """ diff --git a/openedx/core/types/http.py b/openedx/core/types/http.py new file mode 100644 index 000000000000..2896256a107a --- /dev/null +++ b/openedx/core/types/http.py @@ -0,0 +1,45 @@ +""" +Typing utilities for the HTTP requests, responses, etc. + +Includes utilties to work with both vanilla django as well as djangorestframework. +""" +from __future__ import annotations + +import django.contrib.auth.models # pylint: disable=imported-auth-user +import django.http +import rest_framework.request + +import openedx.core.types.user +from openedx.core.types.meta import type_annotation_only + + +@type_annotation_only +class HttpRequest(django.http.HttpRequest): + """ + A request which either has a concrete User (from django.contrib.auth) or is anonymous. + """ + user: openedx.core.types.User + + +@type_annotation_only +class AuthenticatedHttpRequest(HttpRequest): + """ + A request which is guaranteed to have a concrete User (from django.contrib.auth). + """ + user: django.contrib.auth.models.User + + +@type_annotation_only +class RestRequest(rest_framework.request.Request): + """ + Same as HttpRequest, but extended for rest_framework views. + """ + user: openedx.core.types.User + + +@type_annotation_only +class AuthenticatedRestRequest(RestRequest): + """ + Same as AuthenticatedHttpRequest, but extended for rest_framework views. + """ + user: django.contrib.auth.models.User diff --git a/openedx/core/types/meta.py b/openedx/core/types/meta.py new file mode 100644 index 000000000000..39162b05b879 --- /dev/null +++ b/openedx/core/types/meta.py @@ -0,0 +1,37 @@ +""" +Typing utilities for use on other typing utilities. +""" +from __future__ import annotations + +import typing as t + + +def type_annotation_only(cls: type) -> type: + """ + Decorates class which should only be used in type annotations. + + This is useful when you want to enhance an existing 3rd-party concrete class with + type annotations for its members, but don't want the enhanced class to ever actually + be instantiated. For examples, see openedx.core.types.http. + """ + if t.TYPE_CHECKING: + return cls + return _forbid_init(cls) + + +def _forbid_init(forbidden: type) -> type: + """ + Return a class which refuses to be instantiated. + """ + class _ForbidInit: + """ + The resulting class. + """ + def __init__(self, *args, **kwargs): + raise NotImplementedError( + f"Class {forbidden.__module__}:{forbidden.__name__} " + "cannot be instantiated. You may use it as a type annotation, but objects " + "can only be created from its concrete superclasses." + ) + + return _ForbidInit diff --git a/openedx/core/types/user.py b/openedx/core/types/user.py index 95b1fec607fc..9eb63edba358 100644 --- a/openedx/core/types/user.py +++ b/openedx/core/types/user.py @@ -1,8 +1,10 @@ """ Typing utilities for the User models. """ -from typing import Union +from __future__ import annotations + +import typing as t import django.contrib.auth.models -User = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] +User: t.TypeAlias = django.contrib.auth.models.User | django.contrib.auth.models.AnonymousUser