pallets-eco / flask-sqlalchemy

Adds SQLAlchemy support to Flask
https://flask-sqlalchemy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
4.22k stars 899 forks source link

Type error when calling db.Model subclass constructor with parameters #1312

Open tkieft opened 7 months ago

tkieft commented 7 months ago

Pylance / Pyright reports a type error when instantiating a Model subclass with parameters:

Expected no arguments to "User" constructor

Minimal example:

from flask_sqlalchemy import SQLAlchemy
from sqlalchemy import BigInteger, String
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column

class Base(DeclarativeBase):
    pass

db = SQLAlchemy(model_class=Base)

class User(db.Model):
    __tablename__ = "users"

    id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True)
    username: Mapped[str] = mapped_column(String(255), unique=True, nullable=False)

def test():
    user = User(username="x")
    print(user)

I expected there to be no type errors, since this operation is supported within SQLAlchemy.

Environment:

thecodingwizard commented 6 months ago

I'm facing the same issue. Additionally, MappedAsDataclass to the base class does not work.

As a workaround, adding MappedAsDataclass after db.Model in the User class seems to work.

Alternatively:

class Base(DeclarativeBase, MappedAsDataclass):
    pass

class ProperlyTypedSQLAlchemy(SQLAlchemy):
    """Temporary type hinting workaround for Flask SQLAlchemy.

    This is a temporary workaround for the following issue:
    https://github.com/pallets-eco/flask-sqlalchemy/issues/1312
    This workaround may not be correct.
    """

    Model: Type[Base]

db = SQLAlchemy(model_class=Base)
db = cast(ProperlyTypedSQLAlchemy, db)
john0isaac commented 6 months ago

This issue is related to Python, not Flasksqlalchmey.

Your class definition is missing a constructor to initialize it with arguments the way you are doing.

When you create a new object from a Class in Python you can't initialize it with arguments unless it has a constructor function that runs when a new instance is created from this class and initializes it with the values you are passing.

the correct code will be like this:

from flask_sqlalchemy import SQLAlchemy
from sqlalchemy import BigInteger, String
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column

class Base(DeclarativeBase):
    pass

db = SQLAlchemy(model_class=Base)

class User(db.Model):
    __tablename__ = "users"

    id: Mapped[int] = mapped_column(BigInteger, primary_key=True, autoincrement=True)
    username: Mapped[str] = mapped_column(String(255), unique=True, nullable=False)

    def __init__(self, username):
        self.username = username

def test():
    user = User(username="x")
    print(user)

Also, note that you pass the Base as the model class to the SQLAlchemy instance, not the imported DeclarativeBase so you have the flexibility to customize the Base class and everything will inherit from it.

cainmagi commented 6 months ago

I spot another typehint issue just now. See #1318

Hopefully, these two issues can be solved together. To my understanding. SQLAlchemy should be a generic class like this:

class SQLAlchemy(Generic[_FSA_MCT]):
    def __init__(..., model_class: _FSA_MCT, ...): ...

    @property
    def Model(self) -> _FSA_MCT: ...

In current implementation, the TypeVar has already been provided. However, SQLAlchemy it not generic, that's why the type check fails. https://github.com/pallets-eco/flask-sqlalchemy/blob/42a36a3cb604fd39d81d00b54ab3988bbd0ad184/src/flask_sqlalchemy/extension.py#L164-L175

If the typehints can be corrected, the dirty workaround # type: ignore[assignment] at Line 171 can be removed.

davidism commented 6 months ago

Happy to review a PR

cainmagi commented 6 months ago

@davidism

I am trying to make a PR now. However, I am sorry that this issue is far more complicated than I thought.

self.Model should not be merely the model_class passed in the initialization. Actually, in some cases, it is also a subclass of Model. I will try to give a solution, but it may not be able to totally solve this issue.

cainmagi commented 6 months ago

I think I got an idea to fix it. But I am not sure whether this fixture will cause side effects. I am working on #1318 now. If it has been finalized, I will try to submit another PR for this.

from sqlalchemy.util import typing as compat_typing

@compat_typing.dataclass_transform(
    field_specifiers=(
        sa_orm.MappedColumn,
        sa_orm.RelationshipProperty,
        sa_orm.Composite,
        sa_orm.Synonym,
        sa_orm.mapped_column,
        sa_orm.relationship,
        sa_orm.composite,
        sa_orm.synonym,
        sa_orm.deferred,
    ),
)
class _FSAModel(Model):
    metadata: sa.MetaData

After changing this, I found the db.Model has correct initialization.

image

But this change still needs some improvement. It should take effect only when provided model_class is subtype of MappedAsDataclass. Maybe I can add an overload to implement it.

cainmagi commented 6 months ago

@davidism I am glad to tell you that I finally found a good solution (see #1321). Although my PR still has few remained issues. It has tackled the type errors in most cases. I also attach codes for testing is and attach another part for explaining my idea.

Even if you reject this PR, I still hope that it can suggest a possible direction for solving this issue. At least, this PR works well for me.