Use BaseMixin[type] for UUID pkeys and update other type annotations (

#1927)

* Use ProtoMixin name suffix for mixin classes that have expectations (ie, are protocols)

* Lock rows for update when reordering

funnel/forms/account.py

@@ -496,7 +496,7 @@ def validate_username(self, field: forms.Field) -> None:
         raise_username_error(reason)
 
 
-class EnableNotificationsDescriptionMixin:
+class EnableNotificationsDescriptionProtoMixin:
     """Mixin to add a link in the description for enabling notifications."""
 
     enable_notifications: forms.Field
@@ -513,7 +513,9 @@ def set_queries(self) -> None:
 
 
 @Account.forms('email_add')
-class NewEmailAddressForm(EnableNotificationsDescriptionMixin, forms.RecaptchaForm):
+class NewEmailAddressForm(
+    EnableNotificationsDescriptionProtoMixin, forms.RecaptchaForm
+):
     """Form to add a new email address to an account."""
 
     __expects__ = ('edit_user',)
@@ -560,7 +562,7 @@ class EmailPrimaryForm(forms.Form):
 
 
 @Account.forms('phone_add')
-class NewPhoneForm(EnableNotificationsDescriptionMixin, forms.RecaptchaForm):
+class NewPhoneForm(EnableNotificationsDescriptionProtoMixin, forms.RecaptchaForm):
     """Form to add a new mobile number (SMS-capable) to an account."""
 
     __expects__ = ('edit_user',)

funnel/models/account.py

@@ -6,7 +6,7 @@
 import itertools
 from collections.abc import Iterable, Iterator
 from datetime import datetime, timedelta
-from typing import ClassVar, Literal, Union, cast, overload
+from typing import ClassVar, Literal, cast, overload
 from uuid import UUID
 
 import phonenumbers
@@ -259,7 +259,7 @@ class Account(UuidMixin, BaseMixin, Model):
         sa.orm.mapped_column(sa.Integer, nullable=False), read={'all'}
     )
 
-    search_vector: Mapped[str] = sa.orm.mapped_column(
+    search_vector: Mapped[TSVectorType] = sa.orm.mapped_column(
         TSVectorType(
             'title',
             'name',
@@ -1227,11 +1227,10 @@ def organization_links(self) -> list:
 add_search_trigger(Account, 'name_vector')
 
 
-class AccountOldId(UuidMixin, BaseMixin, Model):
+class AccountOldId(UuidMixin, BaseMixin[UUID], Model):
     """Record of an older UUID for an account, after account merger."""
 
     __tablename__ = 'account_oldid'
-    __uuid_primary_key__ = True
 
     #: Old account, if still present
     old_account: Mapped[Account] = relationship(
@@ -2178,7 +2177,7 @@ def get(
 )
 
 #: Anchor type
-Anchor = Union[AccountEmail, AccountEmailClaim, AccountPhone, EmailAddress, PhoneNumber]
+Anchor = AccountEmail | AccountEmailClaim | AccountPhone | EmailAddress | PhoneNumber
 
 # Tail imports
 # pylint: disable=wrong-import-position

funnel/models/comment.py

@@ -4,7 +4,7 @@
 
 from collections.abc import Sequence
 from datetime import datetime
-from typing import Any
+from typing import TYPE_CHECKING, Any
 
 from werkzeug.utils import cached_property
 
@@ -134,7 +134,7 @@ def __init__(self, **kwargs) -> None:
         self.count = 0
 
     @cached_property
-    def parent(self) -> BaseMixin:
+    def parent(self) -> Project | Proposal | Update:
         # FIXME: Move this to a CommentMixin that uses a registry, like EmailAddress
         if self.project is not None:
             return self.project
@@ -147,11 +147,8 @@ def parent(self) -> BaseMixin:
     with_roles(parent, read={'all'}, datasets={'primary'})
 
     @cached_property
-    def parent_type(self) -> str | None:
-        parent = self.parent
-        if parent is not None:
-            return parent.__tablename__
-        return None
+    def parent_type(self) -> str:
+        return self.parent.__tablename__
 
     with_roles(parent_type, read={'all'})
 
@@ -363,6 +360,7 @@ def _message_expression(cls):
 
     @property
     def title(self) -> str:
+        """A made-up title referring to the context for the comment."""
         obj = self.commentset.parent
         if obj is not None:
             return _("{user} commented on {obj}").format(
@@ -439,3 +437,10 @@ class __Commentset:
         ),
         viewonly=True,
     )
+
+
+# Tail imports for type checking
+if TYPE_CHECKING:
+    from .project import Project
+    from .proposal import Proposal
+    from .update import Update

funnel/models/membership_mixin.py

@@ -5,6 +5,7 @@
 from collections.abc import Callable, Iterable
 from datetime import datetime as datetime_type
 from typing import TYPE_CHECKING, Any, ClassVar, Generic, TypeVar
+from uuid import UUID
 
 from sqlalchemy import event
 from sqlalchemy.sql.expression import ColumnElement
@@ -27,7 +28,7 @@
     sa,
 )
 from .account import Account
-from .reorder_mixin import ReorderMixin
+from .reorder_mixin import ReorderProtoMixin
 
 # Export only symbols needed in views.
 __all__ = [
@@ -40,7 +41,9 @@
 # --- Typing ---------------------------------------------------------------------------
 
 MembershipType = TypeVar('MembershipType', bound='ImmutableMembershipMixin')
-FrozenAttributionType = TypeVar('FrozenAttributionType', bound='FrozenAttributionMixin')
+FrozenAttributionType = TypeVar(
+    'FrozenAttributionType', bound='FrozenAttributionProtoMixin'
+)
 
 # --- Enum -----------------------------------------------------------------------------
 
@@ -82,10 +85,9 @@ class MembershipRecordTypeError(MembershipError):
 
 
 @declarative_mixin
-class ImmutableMembershipMixin(UuidMixin, BaseMixin):
+class ImmutableMembershipMixin(UuidMixin, BaseMixin[UUID]):
     """Support class for immutable memberships."""
 
-    __uuid_primary_key__ = True
     #: Can granted_by be null? Only in memberships based on legacy data
     __null_granted_by__: ClassVar[bool] = False
     #: List of columns that will be copied into a new row when a membership is amended
@@ -383,7 +385,7 @@ def member_id(cls) -> Mapped[int]:
     @with_roles(read={'member', 'editor'}, grants_via={None: {'admin': 'member'}})
     @declared_attr
     @classmethod
-    def member(cls) -> Mapped[Account]:
+    def member(cls) -> Mapped[Account]:  # type: ignore[override]
         """Member in this membership record."""
         return relationship(Account, foreign_keys=[cls.member_id])
 
@@ -490,7 +492,7 @@ def migrate_account(cls, old_account: Account, new_account: Account) -> None:
 
 
 @declarative_mixin
-class ReorderMembershipMixin(ReorderMixin):
+class ReorderMembershipProtoMixin(ReorderProtoMixin):
     """Customizes ReorderMixin for membership models."""
 
     if TYPE_CHECKING:
@@ -558,7 +560,7 @@ def parent_scoped_reorder_query_filter(self) -> ColumnElement:
 
 
 @declarative_mixin
-class FrozenAttributionMixin:
+class FrozenAttributionProtoMixin:
     """Provides a `title` data column and support method to freeze it."""
 
     if TYPE_CHECKING:
@@ -580,7 +582,7 @@ def _title(cls) -> Mapped[str | None]:
     def title(self) -> str:
         """Attribution title for this record."""
         if self._local_data_only:
-            return self._title  # This may be None
+            return self._title  # This may be None  # type: ignore[return-value]
         return self._title or self.member.title
 
     @title.setter

funnel/models/moderation.py

@@ -2,6 +2,8 @@
 
 from __future__ import annotations
 
+from uuid import UUID
+
 from baseframe import __
 from coaster.sqlalchemy import StateManager, with_roles
 from coaster.utils import LabeledEnum
@@ -20,9 +22,8 @@ class MODERATOR_REPORT_TYPE(LabeledEnum):  # noqa: N801
     SPAM = (2, 'spam', __("Spam"))
 
 
-class CommentModeratorReport(UuidMixin, BaseMixin, Model):
+class CommentModeratorReport(UuidMixin, BaseMixin[UUID], Model):
     __tablename__ = 'comment_moderator_report'
-    __uuid_primary_key__ = True
 
     comment_id = sa.orm.mapped_column(
         sa.Integer, sa.ForeignKey('comment.id'), nullable=False, index=True

funnel/models/notification.py

@@ -636,7 +636,7 @@ def allow_transport(cls, transport: str) -> bool:
     @property
     def role_provider_obj(self) -> _F | _D:
         """Return fragment if exists, document otherwise, indicating role provider."""
-        return cast(Union[_F, _D], self.fragment or self.document)
+        return cast(_F | _D, self.fragment or self.document)
 
     def dispatch(self) -> Generator[NotificationRecipient, None, None]:
         """
@@ -733,7 +733,7 @@ def __getattr__(self, attr: str) -> Any:
         return getattr(self.cls, attr)
 
 
-class NotificationRecipientMixin:
+class NotificationRecipientProtoMixin:
     """Shared mixin for :class:`NotificationRecipient` and :class:`NotificationFor`."""
 
     notification: Mapped[Notification] | Notification | PreviewNotification
@@ -788,7 +788,7 @@ def is_not_deleted(self, revoke: bool = False) -> bool:
         return False
 
 
-class NotificationRecipient(NotificationRecipientMixin, NoIdMixin, Model):
+class NotificationRecipient(NoIdMixin, NotificationRecipientProtoMixin, Model):
     """
     The recipient of a notification.
 
@@ -1201,7 +1201,7 @@ def migrate_account(cls, old_account: Account, new_account: Account) -> None:
         )
 
 
-class NotificationFor(NotificationRecipientMixin):
+class NotificationFor(NotificationRecipientProtoMixin):
     """View-only wrapper to mimic :class:`UserNotification`."""
 
     notification: Notification | PreviewNotification

funnel/models/proposal.py

@@ -33,7 +33,7 @@
 )
 from .project import Project
 from .project_membership import project_child_role_map
-from .reorder_mixin import ReorderMixin
+from .reorder_mixin import ReorderProtoMixin
 from .video_mixin import VideoMixin
 
 __all__ = ['PROPOSAL_STATE', 'Proposal', 'ProposalSuuidRedirect']
@@ -119,7 +119,7 @@ class PROPOSAL_STATE(LabeledEnum):  # noqa: N801
 
 
 class Proposal(  # type: ignore[misc]
-    UuidMixin, BaseScopedIdNameMixin, VideoMixin, ReorderMixin, Model
+    UuidMixin, BaseScopedIdNameMixin, VideoMixin, ReorderProtoMixin, Model
 ):
     __tablename__ = 'proposal'
 

funnel/models/proposal_membership.py

@@ -10,9 +10,9 @@
 from .account import Account
 from .helpers import reopen
 from .membership_mixin import (
-    FrozenAttributionMixin,
+    FrozenAttributionProtoMixin,
     ImmutableUserMembershipMixin,
-    ReorderMembershipMixin,
+    ReorderMembershipProtoMixin,
 )
 from .project import Project
 from .proposal import Proposal
@@ -21,7 +21,10 @@
 
 
 class ProposalMembership(  # type: ignore[misc]
-    FrozenAttributionMixin, ReorderMembershipMixin, ImmutableUserMembershipMixin, Model
+    ImmutableUserMembershipMixin,
+    FrozenAttributionProtoMixin,
+    ReorderMembershipProtoMixin,
+    Model,
 ):
     """Users can be presenters or reviewers on proposals."""
 

funnel/models/reorder_mixin.py

@@ -8,23 +8,24 @@
 
 from . import Mapped, QueryProperty, db, declarative_mixin, sa
 
-__all__ = ['ReorderMixin']
+__all__ = ['ReorderProtoMixin']
 
 
-# Use of TypeVar for subclasses of ReorderMixin as defined in this mypy ticket:
+# Use of TypeVar for subclasses of ReorderMixin as defined in these mypy tickets:
 # https://github.com/python/mypy/issues/1212
-Reorderable = TypeVar('Reorderable', bound='ReorderMixin')
+# https://github.com/python/mypy/issues/7191
+Reorderable = TypeVar('Reorderable', bound='ReorderProtoMixin')
 
 
 @declarative_mixin
-class ReorderMixin:
+class ReorderProtoMixin:
     """Adds support for re-ordering sequences within a parent container."""
 
     if TYPE_CHECKING:
         #: Subclasses must have a created_at column
         created_at: Mapped[datetime]
         #: Subclass must have a primary key that is int or uuid
-        id: Mapped[int]  # noqa: A001
+        id: Mapped[int | UUID]  # noqa: A001
         #: Subclass must declare a parent_id synonym to the parent model fkey column
         parent_id: Mapped[int | UUID]
         #: Subclass must declare a seq column or synonym, holding a sequence id. It
@@ -36,7 +37,7 @@ class ReorderMixin:
         query: ClassVar[QueryProperty]
 
     @property
-    def parent_scoped_reorder_query_filter(self: Reorderable):
+    def parent_scoped_reorder_query_filter(self: Reorderable) -> sa.ColumnElement[bool]:
         """
         Return a query filter that includes a scope limitation to the parent.
 
@@ -80,6 +81,7 @@ def reorder_item(self: Reorderable, other: Reorderable, before: bool) -> None:
                 cls.seq >= min(self.seq, other.seq),
                 cls.seq <= max(self.seq, other.seq),
             )
+            .with_for_update(of=cls)  # Lock these rows to prevent a parallel update
             .options(sa.orm.load_only(cls.id, cls.seq))
             .order_by(*order_columns)
             .all()
@@ -99,7 +101,9 @@ def reorder_item(self: Reorderable, other: Reorderable, before: bool) -> None:
         new_seq_number = self.seq
         # Temporarily give self an out-of-bounds number
         self.seq = (
-            sa.select(sa.func.coalesce(sa.func.max(cls.seq) + 1, 1))
+            sa.select(  # type: ignore[assignment]
+                sa.func.coalesce(sa.func.max(cls.seq) + 1, 1)
+            )
             .where(self.parent_scoped_reorder_query_filter)
             .scalar_subquery()
         )
@@ -109,7 +113,7 @@ def reorder_item(self: Reorderable, other: Reorderable, before: bool) -> None:
         for reorderable_item in items_to_reorder[1:]:  # Skip 0, which is self
             reorderable_item.seq, new_seq_number = new_seq_number, reorderable_item.seq
             # Flush to force execution order. This does not expunge SQLAlchemy cache as
-            # of SQLAlchemy 1.3.x. Should that behaviour change, a switch to
+            # of SQLAlchemy 2.0.x. Should that behaviour change, a switch to
             # bulk_update_mappings will be required
             db.session.flush()
             if reorderable_item.id == other.id:

funnel/models/sponsor_membership.py

@@ -10,9 +10,9 @@
 from .account import Account
 from .helpers import reopen
 from .membership_mixin import (
-    FrozenAttributionMixin,
+    FrozenAttributionProtoMixin,
     ImmutableUserMembershipMixin,
-    ReorderMembershipMixin,
+    ReorderMembershipProtoMixin,
 )
 from .project import Project
 from .proposal import Proposal
@@ -21,9 +21,9 @@
 
 
 class ProjectSponsorMembership(  # type: ignore[misc]
-    FrozenAttributionMixin,
-    ReorderMembershipMixin,
     ImmutableUserMembershipMixin,
+    FrozenAttributionProtoMixin,
+    ReorderMembershipProtoMixin,
     Model,
 ):
     """Sponsor of a project."""
@@ -151,8 +151,8 @@ def has_sponsors(self) -> bool:
 # FIXME: Replace this with existing proposal collaborator as they're now both related
 # to "account"
 class ProposalSponsorMembership(  # type: ignore[misc]
-    FrozenAttributionMixin,
-    ReorderMembershipMixin,
+    FrozenAttributionProtoMixin,
+    ReorderMembershipProtoMixin,
     ImmutableUserMembershipMixin,
     Model,
 ):