Skip to content

Commit 89697c3

Browse files
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: - python/mypy#17402 (worked around) - typeddjango/django-stubs#2228 (the workaround) with that fixed, it exposed a handful issues in django-stubs itself: - typeddjango/django-stubs#2240 - typeddjango/django-stubs#2241 - typeddjango/django-stubs#2244 - typeddjango/django-stubs#2248 - typeddjango/django-stubs#2276 - typeddjango/django-stubs#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 <!-- Describe your PR here. -->
1 parent 1d9d37b commit 89697c3

File tree

9 files changed

+23
-19
lines changed

9 files changed

+23
-19
lines changed

requirements-dev-frozen.txt

+1-1
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ selenium==4.16.0
180180
sentry-arroyo==2.16.5
181181
sentry-cli==2.16.0
182182
sentry-devenv==1.7.0
183-
sentry-forked-django-stubs==5.0.2.post7
183+
sentry-forked-django-stubs==5.0.2.post8
184184
sentry-forked-djangorestframework-stubs==3.15.0.post1
185185
sentry-kafka-schemas==0.1.101
186186
sentry-ophio==0.2.7

requirements-dev.txt

+1-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ pip-tools>=7.1.0
3535
packaging>=21.3
3636

3737
# for type checking
38-
sentry-forked-django-stubs>=5.0.2.post7
38+
sentry-forked-django-stubs>=5.0.2.post8
3939
sentry-forked-djangorestframework-stubs>=3.15.0.post1
4040
lxml-stubs
4141
msgpack-types>=0.2.0

src/sentry/backup/imports.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ def _clear_model_tables_before_import():
7272
for model in reversed:
7373
using = router.db_for_write(model)
7474
manager = model.with_deleted if issubclass(model, ParanoidModel) else model.objects
75-
manager.all().delete() # type: ignore[attr-defined]
75+
manager.all().delete()
7676

7777
# TODO(getsentry/team-ospo#190): Remove the "Node" kludge below in favor of a more permanent
7878
# solution.

src/sentry/db/models/manager/base.py

+6-3
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,13 @@
66
from collections.abc import Callable, Collection, Generator, Mapping, MutableMapping, Sequence
77
from contextlib import contextmanager
88
from enum import IntEnum, auto
9-
from typing import Any, Generic
9+
from typing import Any
1010

1111
from django.conf import settings
1212
from django.db import models, router
1313
from django.db.models import Model
1414
from django.db.models.fields import Field
15-
from django.db.models.manager import BaseManager as DjangoBaseManager
15+
from django.db.models.manager import Manager as DjangoBaseManager
1616
from django.db.models.signals import class_prepared, post_delete, post_init, post_save
1717
from django.utils.encoding import smart_str
1818

@@ -69,7 +69,10 @@ def make_key(model: Any, prefix: str, kwargs: Mapping[str, Model | int | str]) -
6969
return f"{prefix}:{model.__name__}:{md5_text(kwargs_bits_str).hexdigest()}"
7070

7171

72-
class BaseManager(DjangoBaseManager.from_queryset(BaseQuerySet), Generic[M]): # type: ignore[misc]
72+
_base_manager_base = DjangoBaseManager.from_queryset(BaseQuerySet, "_base_manager_base")
73+
74+
75+
class BaseManager(_base_manager_base[M]):
7376
lookup_handlers = {"iexact": lambda x: x.upper()}
7477
use_for_related_fields = True
7578

src/sentry/models/files/abstractfile.py

+6-3
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,8 @@ class Meta:
219219

220220
def _get_chunked_blob(self, mode=None, prefetch=False, prefetch_to=None, delete=True):
221221
return ChunkedFileBlobIndexWrapper(
222-
self.FILE_BLOB_INDEX_MODEL.objects.filter(file=self)
222+
# TODO: file blob inheritance hierarchy is unsound
223+
self.FILE_BLOB_INDEX_MODEL.objects.filter(file=self) # type: ignore[misc]
223224
.select_related("blob")
224225
.order_by("offset"),
225226
mode=mode,
@@ -295,7 +296,8 @@ def putfile(self, fileobj, blob_size=DEFAULT_BLOB_SIZE, commit=True, logger=noop
295296
blob_fileobj = ContentFile(contents)
296297
blob = self.FILE_BLOB_MODEL.from_file(blob_fileobj, logger=logger)
297298
results.append(
298-
self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset)
299+
# TODO: file blob inheritance hierarchy is unsound
300+
self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset) # type: ignore[misc]
299301
)
300302
offset += blob.size
301303
self.size = offset
@@ -334,7 +336,8 @@ def assemble_from_file_blob_ids(self, file_blob_ids, checksum):
334336
offset = 0
335337
for blob in file_blobs:
336338
try:
337-
self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset)
339+
# TODO: file blob inheritance hierarchy is unsound
340+
self.FILE_BLOB_INDEX_MODEL.objects.create(file=self, blob=blob, offset=offset) # type: ignore[misc]
338341
except IntegrityError:
339342
# Most likely a `ForeignKeyViolation` like `SENTRY-11P5`, because
340343
# the blob we want to link does not exist anymore

src/sentry/models/files/abstractfileblob.py

+2-1
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,8 @@ def _ensure_blob_owned(blob):
9292
return
9393
try:
9494
with atomic_transaction(using=router.db_for_write(cls.FILE_BLOB_OWNER_MODEL)):
95-
cls.FILE_BLOB_OWNER_MODEL.objects.create(
95+
# TODO: file blob inheritance hierarchy is unsound
96+
cls.FILE_BLOB_OWNER_MODEL.objects.create( # type: ignore[misc]
9697
organization_id=organization.id, blob=blob
9798
)
9899
except IntegrityError:

src/sentry/organizations/services/organization/impl.py

+2-2
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,7 @@ def get_or_create_team_member(
475475

476476
def update_membership_flags(self, *, organization_member: RpcOrganizationMember) -> None:
477477
model = OrganizationMember.objects.get(id=organization_member.id)
478-
model.flags = self._deserialize_member_flags(organization_member.flags)
478+
model.flags = self._deserialize_member_flags(organization_member.flags) # type: ignore[assignment] # TODO: make BitField a mypy plugin
479479
model.save()
480480

481481
@classmethod
@@ -518,7 +518,7 @@ def merge_users(self, *, organization_id: int, from_user_id: int, to_user_id: in
518518
return
519519

520520
if to_member is None:
521-
to_member = OrganizationMember.objects.create(
521+
to_member = OrganizationMember.objects.create( # type: ignore[misc] # TODO: make BitField a mypy plugin
522522
organization_id=organization_id,
523523
user_id=to_user_id,
524524
role=from_member.role,

src/sentry/reprocessing2.py

+1-1
Original file line numberDiff line numberDiff line change
@@ -547,7 +547,7 @@ def start_group_reprocessing(
547547

548548
# Create a duplicate row that has the same attributes by nulling out
549549
# the primary key and saving
550-
group.pk = group.id = None
550+
group.pk = group.id = None # type: ignore[assignment] # XXX: intentional resetting pk
551551
new_group = group # rename variable just to avoid confusion
552552
del group
553553
new_group.status = original_status

tests/sentry/db/test_transactions.py

+3-6
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
from unittest.mock import patch
33

44
import pytest
5-
from django.db import IntegrityError, router, transaction
5+
from django.db import router, transaction
66
from django.test import override_settings
77

88
from sentry.db.postgres.transactions import (
@@ -30,11 +30,8 @@ def test_collect_transaction_queries(self):
3030
User.objects.filter(username="user1").first()
3131

3232
with transaction.atomic(using=router.db_for_write(Organization)):
33-
try:
34-
with transaction.atomic(using=router.db_for_write(Organization)):
35-
Organization.objects.create(name=None)
36-
except (IntegrityError, MaxSnowflakeRetryError):
37-
pass
33+
with pytest.raises(MaxSnowflakeRetryError):
34+
Organization.objects.create(name=None) # type: ignore[misc] # intentional to trigger error
3835

3936
with transaction.atomic(using=router.db_for_write(Organization)):
4037
Organization.objects.create(name="org3")

0 commit comments

Comments
 (0)