refactor: replace backref with back_populates (pypi#14788)

* feat(dev): add check for relationship(backref=) Signed-off-by: Mike Fiedler <[email protected]> * refactor: replace backref with back_populates Add both sides of the relationship equation where needed. Signed-off-by: Mike Fiedler <[email protected]> * chore: remove unused backref If we don't use it, remove it. Signed-off-by: Mike Fiedler <[email protected]> * fix: ensure descriptions are deleted when release is Signed-off-by: Mike Fiedler <[email protected]> --------- Signed-off-by: Mike Fiedler <[email protected]>
miketheman · Oct 27, 2023 · f74c002 · f74c002
1 parent fd6088d
commit f74c002
Show file tree

Hide file tree

Showing 8 changed files with 147 additions and 41 deletions.
diff --git a/dev/flake8/checkers.py b/dev/flake8/checkers.py
@@ -23,13 +23,40 @@
 from typing import Any
 
 WH001_msg = "WH001 Prefer `urllib3.util.parse_url` over `urllib.parse.urlparse`"
+WH002_msg = (
+    "WH002 Prefer `sqlalchemy.orm.relationship(back_populates=...)` "
+    "over `sqlalchemy.orm.relationship(backref=...)`"
+)
 
 
 class WarehouseVisitor(ast.NodeVisitor):
     def __init__(self, filename: str) -> None:
         self.errors: list[tuple[int, int, str]] = []
         self.filename = filename
 
+    def check_for_backref(self, node) -> None:
+        def _check_keywords(keywords: list[ast.keyword]) -> None:
+            for kw in keywords:
+                if kw.arg == "backref":
+                    self.errors.append((kw.lineno, kw.col_offset, WH002_msg))
+
+        # Nodes can be either Attribute or Name, and depending on the type
+        # of node, the value.func can be either an attr or an id.
+        # TODO: This is aching for a better way to do this.
+        if isinstance(node.value, ast.Call):
+            if (
+                isinstance(node.value.func, ast.Attribute)
+                and node.value.func.attr == "relationship"
+                and isinstance(node.value.keywords, list)
+            ):
+                _check_keywords(node.value.keywords)
+            elif (
+                isinstance(node.value.func, ast.Name)
+                and node.value.func.id == "relationship"
+                and isinstance(node.value.keywords, list)
+            ):
+                _check_keywords(node.value.keywords)
+
     def visit_Name(self, node: ast.Name) -> None:  # noqa: N802
         if node.id == "urlparse":
             self.errors.append((node.lineno, node.col_offset, WH001_msg))
@@ -46,6 +73,14 @@ def visit_Attribute(self, node: ast.Attribute) -> None:  # noqa: N802
 
         self.generic_visit(node)
 
+    def visit_Assign(self, node: ast.Assign) -> None:  # noqa: N802
+        self.check_for_backref(node)
+        self.generic_visit(node)
+
+    def visit_AnnAssign(self, node: ast.AnnAssign) -> None:  # noqa: N802
+        self.check_for_backref(node)
+        self.generic_visit(node)
+
 
 class WarehouseCheck:
     def __init__(self, tree: ast.AST, filename: str) -> None:

diff --git a/tests/unit/packaging/test_models.py b/tests/unit/packaging/test_models.py
@@ -570,6 +570,16 @@ def test_trusted_published_mixed(self, db_session):
 
         assert not release.trusted_published
 
+    def test_description_relationship(self, db_request):
+        """When a Release is deleted, the Description is also deleted."""
+        release = DBReleaseFactory.create()  # also creates a Description
+        description = release.description
+
+        db_request.db.delete(release)
+
+        assert release in db_request.db.deleted
+        assert description in db_request.db.deleted
+
 
 class TestFile:
     def test_requires_python(self, db_session):

diff --git a/tests/unit/packaging/test_tasks.py b/tests/unit/packaging/test_tasks.py
@@ -389,13 +389,12 @@ def test_update_description_html(monkeypatch, db_request):
 
 
 def test_update_release_description(db_request):
-    release = ReleaseFactory.create()
     description = DescriptionFactory.create(
-        release=release,
         raw="rst\n===\n\nbody text",
         html="",
         rendered_by="0.0",
     )
+    release = ReleaseFactory.create(description=description)
 
     task = pretend.stub()
     update_release_description(task, db_request, release.id)

diff --git a/warehouse/accounts/models.py b/warehouse/accounts/models.py
@@ -43,6 +43,12 @@
 if TYPE_CHECKING:
     from warehouse.macaroons.models import Macaroon
     from warehouse.oidc.models import PendingOIDCPublisher
+    from warehouse.organizations.models import (
+        Organization,
+        OrganizationApplication,
+        OrganizationRole,
+        Team,
+    )
     from warehouse.packaging.models import Project
 
 
@@ -97,40 +103,64 @@ class User(SitemapMixin, HasEvents, db.Model):
     last_totp_value: Mapped[str | None]
 
     webauthn: Mapped[list[WebAuthn]] = orm.relationship(
-        "WebAuthn", backref="user", cascade="all, delete-orphan", lazy=True
+        back_populates="user", cascade="all, delete-orphan", lazy=True
     )
 
     recovery_codes: Mapped[list[RecoveryCode]] = orm.relationship(
-        "RecoveryCode", backref="user", cascade="all, delete-orphan", lazy="dynamic"
+        back_populates="user", cascade="all, delete-orphan", lazy="dynamic"
     )
 
     emails: Mapped[list[Email]] = orm.relationship(
-        "Email", backref="user", cascade="all, delete-orphan", lazy=False
+        back_populates="user", cascade="all, delete-orphan", lazy=False
     )
 
     macaroons: Mapped[list[Macaroon]] = orm.relationship(
-        "Macaroon",
         cascade="all, delete-orphan",
         lazy=True,
         order_by="Macaroon.created.desc()",
     )
 
+    organization_applications: Mapped[list[OrganizationApplication]] = orm.relationship(
+        back_populates="submitted_by",
+    )
+
+    organizations: Mapped[list[Organization]] = orm.relationship(
+        secondary="organization_roles",
+        back_populates="users",
+        lazy=True,
+        order_by="Organization.name",
+        viewonly=True,
+    )
+
     pending_oidc_publishers: Mapped[list[PendingOIDCPublisher]] = orm.relationship(
-        "PendingOIDCPublisher",
-        backref="added_by",
+        back_populates="added_by",
         cascade="all, delete-orphan",
         lazy=True,
     )
 
     projects: Mapped[list[Project]] = orm.relationship(
-        "Project",
         secondary="roles",
         back_populates="users",
         lazy=True,
         viewonly=True,
         order_by="Project.normalized_name",
     )
 
+    organization_roles: Mapped[list[OrganizationRole]] = orm.relationship(
+        back_populates="user",
+        cascade="all, delete-orphan",
+        lazy=True,
+        viewonly=True,
+    )
+
+    teams: Mapped[list[Team]] = orm.relationship(
+        secondary="team_roles",
+        back_populates="members",
+        lazy=True,
+        viewonly=True,
+        order_by="Team.name",
+    )
+
     @property
     def primary_email(self):
         primaries = [x for x in self.emails if x.primary]
@@ -243,6 +273,7 @@ class WebAuthn(db.Model):
         nullable=False,
         index=True,
     )
+    user: Mapped[User] = orm.relationship(back_populates="webauthn")
     label: Mapped[str]
     credential_id: Mapped[str] = mapped_column(unique=True)
     public_key: Mapped[str | None] = mapped_column(unique=True)
@@ -258,6 +289,7 @@ class RecoveryCode(db.Model):
         nullable=False,
         index=True,
     )
+    user: Mapped[User] = orm.relationship(back_populates="recovery_codes")
     code: Mapped[str] = mapped_column(String(length=128))
     generated: Mapped[datetime_now]
     burned: Mapped[datetime.datetime | None]
@@ -281,6 +313,7 @@ class Email(db.ModelBase):
         PG_UUID(as_uuid=True),
         ForeignKey("users.id", deferrable=True, initially="DEFERRED"),
     )
+    user: Mapped[User] = orm.relationship(back_populates="emails")
     email: Mapped[str] = mapped_column(String(length=254))
     primary: Mapped[bool]
     verified: Mapped[bool]

diff --git a/warehouse/email/ses/models.py b/warehouse/email/ses/models.py
@@ -248,9 +248,8 @@ class EmailMessage(db.Model):
     missing: Mapped[bool_false]
 
     # Relationships!
-    events = orm.relationship(
-        "Event",
-        backref="email",
+    events: Mapped[list["Event"]] = orm.relationship(
+        back_populates="email",
         cascade="all, delete-orphan",
         lazy=False,
         order_by=lambda: Event.created,
@@ -275,6 +274,10 @@ class Event(db.Model):
         ),
         index=True,
     )
+    email: Mapped[EmailMessage] = orm.relationship(
+        back_populates="events",
+        lazy=False,
+    )
 
     event_id: Mapped[str] = mapped_column(unique=True, index=True)
     event_type: Mapped[Enum] = mapped_column(

diff --git a/warehouse/oidc/models/_core.py b/warehouse/oidc/models/_core.py
@@ -13,19 +13,22 @@
 from __future__ import annotations
 
 from collections.abc import Callable
-from typing import Any, TypeVar
+from typing import TYPE_CHECKING, Any, TypeVar
 
 import sentry_sdk
 
 from sqlalchemy import ForeignKey, String, orm
 from sqlalchemy.dialects.postgresql import UUID
-from sqlalchemy.orm import mapped_column
+from sqlalchemy.orm import Mapped, mapped_column
 
 from warehouse import db
-from warehouse.macaroons.models import Macaroon
 from warehouse.oidc.errors import InvalidPublisherError
 from warehouse.oidc.interfaces import SignedClaims
-from warehouse.packaging.models import Project
+
+if TYPE_CHECKING:
+    from warehouse.accounts.models import User
+    from warehouse.macaroons.models import Macaroon
+    from warehouse.packaging.models import Project
 
 C = TypeVar("C")
 
@@ -241,12 +244,13 @@ def publisher_url(self, claims=None) -> str | None:  # pragma: no cover
 class OIDCPublisher(OIDCPublisherMixin, db.Model):
     __tablename__ = "oidc_publishers"
 
-    projects = orm.relationship(
-        Project,
+    projects: Mapped[list[Project]] = orm.relationship(
         secondary=OIDCPublisherProjectAssociation.__table__,
-        backref="oidc_publishers",
+        back_populates="oidc_publishers",
+    )
+    macaroons: Mapped[list[Macaroon]] = orm.relationship(
+        cascade="all, delete-orphan", lazy=True
     )
-    macaroons = orm.relationship(Macaroon, cascade="all, delete-orphan", lazy=True)
 
     __mapper_args__ = {
         "polymorphic_identity": "oidc_publishers",
@@ -266,6 +270,7 @@ class PendingOIDCPublisher(OIDCPublisherMixin, db.Model):
     added_by_id = mapped_column(
         UUID(as_uuid=True), ForeignKey("users.id"), nullable=False, index=True
     )
+    added_by: Mapped[User] = orm.relationship(back_populates="pending_oidc_publishers")
 
     __mapper_args__ = {
         "polymorphic_identity": "pending_oidc_publishers",

diff --git a/warehouse/organizations/models.py b/warehouse/organizations/models.py
@@ -82,8 +82,10 @@ class OrganizationRole(db.Model):
         ForeignKey("organizations.id", onupdate="CASCADE", ondelete="CASCADE"),
     )
 
-    user: Mapped[User] = relationship(lazy=False)
-    organization: Mapped[Organization] = relationship(lazy=False)
+    user: Mapped[User] = relationship(back_populates="organization_roles", lazy=False)
+    organization: Mapped[Organization] = relationship(
+        back_populates="roles", lazy=False
+    )
 
 
 class OrganizationProject(db.Model):
@@ -270,10 +272,16 @@ class Organization(OrganizationMixin, HasEvents, db.Model):
         onupdate=func.now(),
         comment="Datetime the organization was approved by administrators.",
     )
+    application: Mapped[OrganizationApplication] = relationship(
+        back_populates="organization"
+    )
 
     users: Mapped[list[User]] = relationship(
-        secondary=OrganizationRole.__table__, backref="organizations", viewonly=True
+        secondary=OrganizationRole.__table__,
+        back_populates="organizations",
+        viewonly=True,
     )
+    roles: Mapped[list[OrganizationRole]] = relationship(back_populates="organization")
     teams: Mapped[list[Team]] = relationship(
         back_populates="organization",
         order_by=lambda: Team.name.asc(),
@@ -471,9 +479,11 @@ class OrganizationApplication(OrganizationMixin, db.Model):
         comment="If the request was approved, ID of resulting Organization",
     )
 
-    submitted_by: Mapped[User] = relationship(backref="organization_applications")
+    submitted_by: Mapped[User] = relationship(
+        back_populates="organization_applications"
+    )
     organization: Mapped[Organization] = relationship(
-        backref="application", viewonly=True
+        back_populates="application", viewonly=True
     )
 
 
@@ -644,10 +654,10 @@ class Team(HasEvents, db.Model):
         lazy=False, back_populates="teams"
     )
     members: Mapped[list[User]] = relationship(
-        secondary=TeamRole.__table__, backref="teams", viewonly=True
+        secondary=TeamRole.__table__, back_populates="teams", viewonly=True
     )
     projects: Mapped[list[Project]] = relationship(
-        secondary=TeamProjectRole.__table__, backref="teams", viewonly=True
+        secondary=TeamProjectRole.__table__, back_populates="team", viewonly=True
     )
 
     def record_event(self, *, tag, request: Request = None, additional=None):