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 error when calling db.Model subclass constructor with parameters #1312

Open
tkieft opened this issue Feb 9, 2024 · 7 comments
Open

Comments

@tkieft
Copy link

tkieft commented Feb 9, 2024

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:

  • Python version: 3.10.13
  • Flask-SQLAlchemy version: 3.1.1
  • SQLAlchemy version: 2.0.18
@thecodingwizard
Copy link

thecodingwizard commented Mar 3, 2024

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

This comment was marked as outdated.

@cainmagi
Copy link

cainmagi commented Mar 26, 2024

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.

def __init__(
self,
app: Flask | None = None,
*,
metadata: sa.MetaData | None = None,
session_options: dict[str, t.Any] | None = None,
query_class: type[Query] = Query,
model_class: _FSA_MCT = Model, # type: ignore[assignment]
engine_options: dict[str, t.Any] | None = None,
add_models_to_shell: bool = True,
disable_autonaming: bool = False,
):

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

@davidism
Copy link
Member

Happy to review a PR

@cainmagi
Copy link

@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
Copy link

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 added a commit to cainmagi/flask-sqlalchemy that referenced this issue Mar 27, 2024
1. Provide a descriptor `ModelGetter` for solving the `db.Model` type from the `db` type dynamically.
2. Add `t.Type[sa_orm.MappedAsDataclass]` to `_FSA_MCT`.
3. Let `SQLAlchemy(...)` annotated by the provided `model_class` type.
@cainmagi
Copy link

@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.

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

No branches or pull requests

5 participants