Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type (typehint) error for db.relationship #1318

Open
cainmagi opened this issue Mar 26, 2024 · 5 comments
Open

Type (typehint) error for db.relationship #1318

cainmagi opened this issue Mar 26, 2024 · 5 comments

Comments

@cainmagi
Copy link

cainmagi commented Mar 26, 2024

Problem Description

The typehint of

db.relationship("...", secondary=..., back_populates="...")

should be sq_orm.Relationship[...], not sq_orm.RelationshipProperty[...].

The mismatch of the typehint causes the manual annotation supported by sqlalchemy fails:

image

How to fix it

Go here:

def relationship(
self, *args: t.Any, **kwargs: t.Any
) -> sa_orm.RelationshipProperty[t.Any]:
"""A :func:`sqlalchemy.orm.relationship` that applies this extension's
:attr:`Query` class for dynamic relationships and backrefs.
.. versionchanged:: 3.0
The :attr:`Query` class is set on ``backref``.
"""
self._set_rel_query(kwargs)
return sa_orm.relationship(*args, **kwargs)

Make this modification:

    def relationship(
        self, *args: t.Any, **kwargs: t.Any
-   ) -> sa_orm.RelationshipProperty[t.Any]:
+   ) -> sa_orm.Relationship[t.Any]:
        """A :func:`sqlalchemy.orm.relationship` that applies this extension's

Things will get corrected.

It is also recommended to modify this place:

def _relation(
self, *args: t.Any, **kwargs: t.Any
) -> sa_orm.RelationshipProperty[t.Any]:

But the following place should NOT be changed, because it is consistent with sq_orm:

def dynamic_loader(
self, argument: t.Any, **kwargs: t.Any
) -> sa_orm.RelationshipProperty[t.Any]:

Codes with typehint errors when using flask-sqlalchemy

# -*- coding: UTF-8 -*-

try:
    from typing import List
except ImportError:
    from builtins import list as List

from flask_sqlalchemy import SQLAlchemy
import sqlalchemy as sa
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import DeclarativeBase, MappedAsDataclass


class Base(DeclarativeBase, MappedAsDataclass):
    """The base class for creating SQLAlchemy models.

    All mixins are defined in the mro list.

    All metadata of are defined as attributes.
    """


db = SQLAlchemy(model_class=Base)


roles = db.Table(
    "role_users",
    sa.Column("user_id", sa.ForeignKey("user.id"), primary_key=True),
    sa.Column("role_id", sa.ForeignKey("role.id"), primary_key=True),
)


class User(db.Model):
    id: Mapped[int] = mapped_column(primary_key=True, init=False)
    # Expression of type "RelationshipProperty[Any]" cannot be assigned to declared type "Mapped[List[Role]]"
    # "RelationshipProperty[Any]" is incompatible with "Mapped[List[Role]]"Pylance[reportAssignmentType] 
    # (https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportAssignmentType)
    roles: Mapped[List["Role"]] = db.relationship(
        "Role", secondary=roles, back_populates="users", default_factory=list
    )


class Role(db.Model):
    id: Mapped[int] = mapped_column(primary_key=True, init=False)
    # Expression of type "RelationshipProperty[Any]" cannot be assigned to declared type "Mapped[List[User]]"
    #  "RelationshipProperty[Any]" is incompatible with "Mapped[List[User]]"Pylance[reportAssignmentType] 
    # (https://github.com/microsoft/pyright/blob/main/docs/configuration.md#reportAssignmentType)
    users: Mapped[List["User"]] = db.relationship(
        "User", secondary=roles, back_populates="roles", default_factory=list
    )

Codes working perfectly if only using sqlalchemy

# -*- coding: UTF-8 -*-

try:
    from typing import List
except ImportError:
    from builtins import list as List

import sqlalchemy as sa
from sqlalchemy.orm import Mapped, mapped_column, relationship
from sqlalchemy.orm import DeclarativeBase, MappedAsDataclass


class Base(DeclarativeBase, MappedAsDataclass):
    """The base class for creating SQLAlchemy models.

    All mixins are defined in the mro list.

    All metadata of are defined as attributes.
    """


roles = sa.Table(
    "role_users",
    Base.metadata,
    sa.Column("user_id", sa.ForeignKey("user.id"), primary_key=True),
    sa.Column("role_id", sa.ForeignKey("role.id"), primary_key=True),
)


class User(Base):
    __tablename__ = "users"
    id: Mapped[int] = mapped_column(primary_key=True, init=False)
    roles: Mapped[List["Role"]] = relationship(
        "Role", secondary=roles, back_populates="users", default_factory=list
    )


class Role(Base):
    __tablename__ = "roles"
    id: Mapped[int] = mapped_column(primary_key=True, init=False)
    users: Mapped[List["User"]] = relationship(
        "User", secondary=roles, back_populates="roles", default_factory=list
    )

Environment:

  • Python version: 3.10.13
  • Flask-SQLAlchemy version: 3.1.1
  • SQLAlchemy version: 2.0.28
@davidism
Copy link
Member

Happy to review a PR.

cainmagi added a commit to cainmagi/flask-sqlalchemy that referenced this issue Mar 26, 2024
Fix the typehint inconsistence of `db.relationship(...)`.
cainmagi added a commit to cainmagi/flask-sqlalchemy that referenced this issue Mar 26, 2024
Fix the typehint inconsistence of `db.relationship(...)`.
cainmagi added a commit to cainmagi/flask-sqlalchemy that referenced this issue Mar 26, 2024
Make the typehint of `SQLAlchemy.relationship` consistent with `sqlalchemy.orm.relationship`.
@Abdullah00110
Copy link

can i do ?

@cainmagi
Copy link
Author

cainmagi commented Nov 1, 2024

can i do ?

Well, I have already submitted the PR several months ago. That PR is still opened now. I do not think it is necessary to submit another one unless my PR is rejected for some reasons.

@Abdullah00110
Copy link

ok i will try another

@alexthomsonnz
Copy link

PR looks good, can it have a review and get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants