From 2553119b3afb23b60d5d8d0e39711eb5af45f28f Mon Sep 17 00:00:00 2001 From: anthony sottile <103459774+asottile-sentry@users.noreply.github.com> Date: Mon, 29 Jul 2024 16:58:22 -0400 Subject: [PATCH] ref: get proper base manager typing (#72221) an absolute ton of work went into enabling this -- but after this PR model types / create / update / etc. should mostly be typechecked whereas they were entirely unchecked (everything `Any`) prior to this. most of the core problem was that mypy and django-stubs did not understand our custom `BaseManager` and `BaseQuerySet` since there's a significant amount of django stuff that goes into making those classes "real" fix that involved these issues: - https://github.com/python/mypy/issues/17402 (worked around) - https://github.com/typeddjango/django-stubs/pull/2228 (the workaround) with that fixed, it exposed a handful issues in django-stubs itself: - https://github.com/typeddjango/django-stubs/pull/2240 - https://github.com/typeddjango/django-stubs/pull/2241 - https://github.com/typeddjango/django-stubs/pull/2244 - https://github.com/typeddjango/django-stubs/pull/2248 - https://github.com/typeddjango/django-stubs/pull/2276 - https://github.com/typeddjango/django-stubs/pull/2281 fixing all of those together exposed around 1k issues in sentry code itself which was fixed over a number of PRs: - #75186 - #75108 - #75149 - #75162 - #75150 - #75154 - #75158 - #75148 - #75157 - #75156 - #75152 - #75153 - #75151 - #75113 - #75112 - #75111 - #75110 - #75109 - #75013 - #74641 - #74640 - #73783 - #73787 - #73788 - #73793 - #73791 - #73786 - #73761 - #73742 - #73744 - #73602 - #73596 - #73595 - #72892 - #73589 - #73587 - #73581 - #73580 - #73213 - #73207 - #73206 - #73205 - #73202 - #73198 - #73121 - #73109 - #73073 - #73061 - getsentry/getsentry#14370 - #72965 - #72963 - #72967 - #72877 (eventually was able to remove this when upgrading to mypy 1.11) - #72948 - #72799 - #72898 - #72899 - #72896 - #72895 - #72903 - #72894 - #72897 - #72893 - #72889 - #72887 - #72888 - #72811 - #72872 - #72813 - #72815 - #72812 - #72808 - #72802 - #72807 - #72809 - #72810 - #72814 - #72816 - #72818 - #72806 - #72801 - #72797 - #72800 - #72798 - #72771 - #72718 - #72719 - #72710 - #72709 - #72706 - #72693 - #72641 - #72591 - #72635 mypy 1.11 also included some important improvements with typechecking `for model_cls in (M1, M2, M2): ...` which also exposed some problems in our code. unfortunately upgrading mypy involved fixing a lot of things as well: - #75020 - #74982 - #74976 - #74974 - #74972 - #74967 - #74954 - getsentry/getsentry#14739 - getsentry/getsentry#14734 - #74959 - #74958 - #74956 - #74953 - #74955 - #74952 - #74895 - #74892 - #74897 - #74896 - #74893 - #74880 - #74900 - #74902 - #74903 - #74904 - #74899 - #74894 - #74882 - #74881 - #74871 - #74870 - #74855 - #74856 - #74853 - #74857 - #74858 - #74807 - #74805 - #74803 - #74806 - #74798 - #74809 - #74801 - #74804 - #74800 - #74799 - #74725 - #74682 - #74677 - #74680 - #74683 - #74681 needless to say this is probably the largest stacked PR I've ever done -- and I'm pretty happy with how I was able to split this up into digestable chunks --- requirements-dev-frozen.txt | 2 +- requirements-dev.txt | 2 +- src/sentry/backup/imports.py | 2 +- src/sentry/db/models/manager/base.py | 9 ++++++--- src/sentry/models/files/abstractfile.py | 9 ++++++--- src/sentry/models/files/abstractfileblob.py | 3 ++- src/sentry/organizations/services/organization/impl.py | 4 ++-- src/sentry/reprocessing2.py | 2 +- tests/sentry/db/test_transactions.py | 9 +++------ 9 files changed, 23 insertions(+), 19 deletions(-) diff --git a/requirements-dev-frozen.txt b/requirements-dev-frozen.txt index 8d52ffa52e150b..f55cd5a4194a3a 100644 --- a/requirements-dev-frozen.txt +++ b/requirements-dev-frozen.txt @@ -180,7 +180,7 @@ selenium==4.16.0 sentry-arroyo==2.16.5 sentry-cli==2.16.0 sentry-devenv==1.7.0 -sentry-forked-django-stubs==5.0.2.post7 +sentry-forked-django-stubs==5.0.2.post8 sentry-forked-djangorestframework-stubs==3.15.0.post1 sentry-kafka-schemas==0.1.101 sentry-ophio==0.2.7 diff --git a/requirements-dev.txt b/requirements-dev.txt index 2ab4f9606f3d27..ae15e0f26195f7 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -35,7 +35,7 @@ pip-tools>=7.1.0 packaging>=21.3 # for type checking -sentry-forked-django-stubs>=5.0.2.post7 +sentry-forked-django-stubs>=5.0.2.post8 sentry-forked-djangorestframework-stubs>=3.15.0.post1 lxml-stubs msgpack-types>=0.2.0 diff --git a/src/sentry/backup/imports.py b/src/sentry/backup/imports.py index 62189d100438bd..5d0bdb3acfbb90 100644 --- a/src/sentry/backup/imports.py +++ b/src/sentry/backup/imports.py @@ -72,7 +72,7 @@ def _clear_model_tables_before_import(): for model in reversed: using = router.db_for_write(model) manager = model.with_deleted if issubclass(model, ParanoidModel) else model.objects - manager.all().delete() # type: ignore[attr-defined] + manager.all().delete() # TODO(getsentry/team-ospo#190): Remove the "Node" kludge below in favor of a more permanent # solution. diff --git a/src/sentry/db/models/manager/base.py b/src/sentry/db/models/manager/base.py index 3161635d7a2f9d..873ff080069ac8 100644 --- a/src/sentry/db/models/manager/base.py +++ b/src/sentry/db/models/manager/base.py @@ -6,13 +6,13 @@ from collections.abc import Callable, Collection, Generator, Mapping, MutableMapping, Sequence from contextlib import contextmanager from enum import IntEnum, auto -from typing import Any, Generic +from typing import Any from django.conf import settings from django.db import models, router from django.db.models import Model from django.db.models.fields import Field -from django.db.models.manager import BaseManager as DjangoBaseManager +from django.db.models.manager import Manager as DjangoBaseManager from django.db.models.signals import class_prepared, post_delete, post_init, post_save from django.utils.encoding import smart_str @@ -69,7 +69,10 @@ def make_key(model: Any, prefix: str, kwargs: Mapping[str, Model | int | str]) - return f"{prefix}:{model.__name__}:{md5_text(kwargs_bits_str).hexdigest()}" -class BaseManager(DjangoBaseManager.from_queryset(BaseQuerySet), Generic[M]): # type: ignore[misc] +_base_manager_base = DjangoBaseManager.from_queryset(BaseQuerySet, "_base_manager_base") + + +class BaseManager(_base_manager_base[M]): lookup_handlers = {"iexact": lambda x: x.upper()} use_for_related_fields = True diff --git a/src/sentry/models/files/abstractfile.py b/src/sentry/models/files/abstractfile.py index 946e659214413e..eba3e68cadf17b 100644 --- a/src/sentry/models/files/abstractfile.py +++ b/src/sentry/models/files/abstractfile.py @@ -219,7 +219,8 @@ class Meta: def _get_chunked_blob(self, mode=None, prefetch=False, prefetch_to=None, delete=True): return ChunkedFileBlobIndexWrapper( - self.FILE_BLOB_INDEX_MODEL.objects.filter(file=self) + # TODO: file blob inheritance hierarchy is unsound + self.FILE_BLOB_INDEX_MODEL.objects.filter(file=self) # type: ignore[misc] .select_related("blob") .order_by("offset"), mode=mode, @@ -295,7 +296,8 @@ def putfile(self, fileobj, blob_size=DEFAULT_BLOB_SIZE, commit=True, logger=noop blob_fileobj = ContentFile(contents) blob = self.FILE_BLOB_MODEL.from_file(blob_fileobj, logger=logger) results.append( - self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset) + # TODO: file blob inheritance hierarchy is unsound + self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset) # type: ignore[misc] ) offset += blob.size self.size = offset @@ -334,7 +336,8 @@ def assemble_from_file_blob_ids(self, file_blob_ids, checksum): offset = 0 for blob in file_blobs: try: - self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset) + # TODO: file blob inheritance hierarchy is unsound + self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset) # type: ignore[misc] except IntegrityError: # Most likely a `ForeignKeyViolation` like `SENTRY-11P5`, because # the blob we want to link does not exist anymore diff --git a/src/sentry/models/files/abstractfileblob.py b/src/sentry/models/files/abstractfileblob.py index 6598e1da17a4c5..eeac09357f539f 100644 --- a/src/sentry/models/files/abstractfileblob.py +++ b/src/sentry/models/files/abstractfileblob.py @@ -92,7 +92,8 @@ def _ensure_blob_owned(blob): return try: with atomic_transaction(using=router.db_for_write(cls.FILE_BLOB_OWNER_MODEL)): - cls.FILE_BLOB_OWNER_MODEL.objects.create( + # TODO: file blob inheritance hierarchy is unsound + cls.FILE_BLOB_OWNER_MODEL.objects.create( # type: ignore[misc] organization_id=organization.id, blob=blob ) except IntegrityError: diff --git a/src/sentry/organizations/services/organization/impl.py b/src/sentry/organizations/services/organization/impl.py index 53761e90a80f31..947ad568e0866b 100644 --- a/src/sentry/organizations/services/organization/impl.py +++ b/src/sentry/organizations/services/organization/impl.py @@ -475,7 +475,7 @@ def get_or_create_team_member( def update_membership_flags(self, *, organization_member: RpcOrganizationMember) -> None: model = OrganizationMember.objects.get(id=organization_member.id) - model.flags = self._deserialize_member_flags(organization_member.flags) + model.flags = self._deserialize_member_flags(organization_member.flags) # type: ignore[assignment] # TODO: make BitField a mypy plugin model.save() @classmethod @@ -518,7 +518,7 @@ def merge_users(self, *, organization_id: int, from_user_id: int, to_user_id: in return if to_member is None: - to_member = OrganizationMember.objects.create( + to_member = OrganizationMember.objects.create( # type: ignore[misc] # TODO: make BitField a mypy plugin organization_id=organization_id, user_id=to_user_id, role=from_member.role, diff --git a/src/sentry/reprocessing2.py b/src/sentry/reprocessing2.py index 00b778944384b6..b103ebb3eb3aad 100644 --- a/src/sentry/reprocessing2.py +++ b/src/sentry/reprocessing2.py @@ -547,7 +547,7 @@ def start_group_reprocessing( # Create a duplicate row that has the same attributes by nulling out # the primary key and saving - group.pk = group.id = None + group.pk = group.id = None # type: ignore[assignment] # XXX: intentional resetting pk new_group = group # rename variable just to avoid confusion del group new_group.status = original_status diff --git a/tests/sentry/db/test_transactions.py b/tests/sentry/db/test_transactions.py index 0d4d9167b6d92d..e9b836a8c8cf77 100644 --- a/tests/sentry/db/test_transactions.py +++ b/tests/sentry/db/test_transactions.py @@ -2,7 +2,7 @@ from unittest.mock import patch import pytest -from django.db import IntegrityError, router, transaction +from django.db import router, transaction from django.test import override_settings from sentry.db.postgres.transactions import ( @@ -30,11 +30,8 @@ def test_collect_transaction_queries(self): User.objects.filter(username="user1").first() with transaction.atomic(using=router.db_for_write(Organization)): - try: - with transaction.atomic(using=router.db_for_write(Organization)): - Organization.objects.create(name=None) - except (IntegrityError, MaxSnowflakeRetryError): - pass + with pytest.raises(MaxSnowflakeRetryError): + Organization.objects.create(name=None) # type: ignore[misc] # intentional to trigger error with transaction.atomic(using=router.db_for_write(Organization)): Organization.objects.create(name="org3")