Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

Fix issue #1312: Type (typehint) error when callingdb.Model subclass constructor with parameters#1321

Open
cainmagi wants to merge 10 commits intopallets-eco:stablefrom
cainmagi:fix-issue-1312
Open

Fix issue #1312: Type (typehint) error when callingdb.Model subclass constructor with parameters#1321
cainmagi wants to merge 10 commits intopallets-eco:stablefrom
cainmagi:fix-issue-1312

Conversation

@cainmagi
Copy link

@cainmagicainmagi commentedMar 27, 2024
edited
Loading

Fix the typehint inconsistence of initializing a subclass ofdb.Model.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
    • Only bug fixing. No need to do this.
  • Add or update relevant docs, in the docs folder and in code.
    • Only bug fixing. No need to do this.
  • Add an entry inCHANGES.rst summarizing the change and linking to the issue.
    • Only bug fixing. No need to add any change logs for users.
  • Add.. versionchanged:: entries in any relevant code docs.
    • Only bug fixing. No need to do this.
  • Runpre-commit hooks and fix any issues.
  • Runpytest andtox, no tests failed.

Appendices

Appendix A: The idea of this PR.

This example shows the core idea how this PR works.Test(prop: _Mul) makesprop as a parameter of its type hint. Then the descriptor will solve the type of the propertyTest().prop dynamically according to the parameter_Mul of the parameterized type hintTest[_Mul]. For example, if a new instance is initialized astest: Test[int], thenIntOrStr will solve the type oftest.prop asint.

fromtypingimportUnion,AnyfromtypingimportGeneric,TypeVarfromtyping_extensionsimportoverload,reveal_type_Mul=TypeVar("_Mul",bound=Union[int,str])classIntOrStr:@overloaddef__get__(self,obj:"Test[int]",obj_cls=None)->int: ...@overloaddef__get__(self,obj:"Test[str]",obj_cls=None)->str: ...@overloaddef__get__(self,obj:"Test[Union[int, str]]",obj_cls=None    )->Union[int,str]: ...def__get__(self,obj:"Test[Any]",obj_cls=None)->Any:returngetattr(obj,"_prop")classTest(Generic[_Mul]):prop=IntOrStr()def__init__(self,prop:_Mul)->None:self._prop=propreveal_type(Test(1).prop)# intreveal_type(Test("ss").prop)# strdeftest_union(val:Union[int,str])->None:"""Match the pre-defined 3rd overload."""reveal_type(Test(val).prop)# int | strdeftest_any(val:Any)->None:"""Here, Any can be matched with any overload. So the first overload is    preferred."""reveal_type(Test(val).prop)# int

Appendix B: Validate the performance of this PR

This PR only changes the behavior during the static type checking. In other words, at run time, the codes work totally the same as the original version. I made this customization because I think the original codes only have type checking issues but work perfectly at run time. That's why I do not submit any changelogs here, because actually I did not change any functionalities or behaviors at run time.

The following code can be used for testing the performance of the type hints provided by this PR. It works perfected in most cases. However, there are two known cases that the type check shows false negative results. Seetest_not_as_expectation.

It seems that the static type checking codes cannot be added to pytest. That's why I do not add more tests for this PR.

fromtypingimportType,Any,TYPE_CHECKINGfromtyping_extensionsimportNever,reveal_typeimportdataclassesimportsqlalchemy.ormassa_ormfromflask_sqlalchemyimportSQLAlchemyfromflask_sqlalchemy.modelimportDefaultMeta,Modeldeftest_default()->None:db=SQLAlchemy()reveal_type(db)# SQLAlchemy[type[Model]]reveal_type(db.Model)# type[_FSAModel_KW]deftest_unknown_class(model_class:Any)->None:db=SQLAlchemy(model_class=model_class)reveal_type(db)# SQLAlchemy[Any]reveal_type(db.Model)# type[_FSAModel_KW]deftest_meta_class_v1(meta_class:Type[sa_orm.DeclarativeMeta])->None:ifTYPE_CHECKING:db=SQLAlchemy(model_class=meta_class(type))reveal_type(db)# SQLAlchemy[type[__class_type]]reveal_type(db.Model)# type[_FSAModel_KW]classTestClass(metaclass=meta_class):passdb_v2=SQLAlchemy(model_class=TestClass)reveal_type(db_v2)# SQLAlchemy[type[TestClass]]reveal_type(db_v2.Model)# type[_FSAModel_KW]deftest_meta_class_v2(meta_class:Type[DefaultMeta])->None:ifTYPE_CHECKING:db=SQLAlchemy(model_class=meta_class(type))reveal_type(db)# SQLAlchemy[type[__class_type]]reveal_type(db.Model)# type[_FSAModel_KW]classTestClass(metaclass=meta_class):passdb_v2=SQLAlchemy(model_class=TestClass)reveal_type(db_v2)# SQLAlchemy[type[TestClass]]reveal_type(db_v2.Model)# type[_FSAModel_KW]deftest_sqlalchemy2_base(model_class:Type[sa_orm.DeclarativeBase])->None:db=SQLAlchemy(model_class=model_class)reveal_type(db)# SQLAlchemy[type[DeclarativeBase]]reveal_type(db.Model)# type[_FSAModel_KW]deftest_sqlalchemy2_nometa(model_class:Type[sa_orm.DeclarativeBaseNoMeta])->None:db=SQLAlchemy(model_class=model_class)reveal_type(db)# SQLAlchemy[type[DeclarativeBaseNoMeta]]reveal_type(db.Model)# type[_FSAModel_KW]deftest_sqlalchemy2_dataclass(model_class:Type[sa_orm.MappedAsDataclass])->None:db=SQLAlchemy(model_class=model_class)reveal_type(db)# SQLAlchemy[type[MappedAsDataclass]]reveal_type(db.Model)# type[_FSAModel_DataClass]deftest_sqlalchemy2_hybird()->None:classBase1(sa_orm.MappedAsDataclass,sa_orm.DeclarativeBase):passdb1=SQLAlchemy(model_class=Base1)reveal_type(db1)# SQLAlchemy[type[Base1]]reveal_type(db1.Model)# type[_FSAModel_DataClass]classBase2(sa_orm.DeclarativeBase,sa_orm.MappedAsDataclass):passdb2=SQLAlchemy(model_class=Base2)reveal_type(db2)# SQLAlchemy[type[Base2]]reveal_type(db2.Model)# type[_FSAModel_DataClass]classAnyClass:passclassBase3(sa_orm.DeclarativeBase,AnyClass):passdb3=SQLAlchemy(model_class=Base3)reveal_type(db3)# SQLAlchemy[type[Base3]]reveal_type(db3.Model)# type[_FSAModel_KW]deftest_class_init_kw()->None:classBaseKW(sa_orm.DeclarativeBase):passdb=SQLAlchemy(model_class=BaseKW)reveal_type(db)# SQLAlchemy[type[BaseKW]]reveal_type(db.Model)# type[_FSAModel_KW]classModelSa(BaseKW):__tablename__="modelsas"id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True)name:sa_orm.Mapped[str]classModelDb(db.Model):id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True)name:sa_orm.Mapped[str]# Well done! Now db.Model works in the same way compared to the base class.reveal_type(ModelSa.__init__)# (self: ModelSa, **kw: Any) -> Nonereveal_type(ModelDb.__init__)# (self: ModelDb, **kw: Any) -> Nonedeftest_class_init_kw_v2()->None:BaseKWMeta=sa_orm.declarative_base()reveal_type(BaseKWMeta)# Anyassertisinstance(BaseKWMeta,sa_orm.DeclarativeMeta)assertnotissubclass(BaseKWMeta,sa_orm.DeclarativeBase)assertnotissubclass(BaseKWMeta,sa_orm.DeclarativeBaseNoMeta)assertnotissubclass(BaseKWMeta,sa_orm.MappedAsDataclass)db=SQLAlchemy(model_class=BaseKWMeta)reveal_type(db)# SQLAlchemy[DeclarativeMeta]reveal_type(db.Model)# type[_FSAModel_KW]classModelSa(BaseKWMeta):__tablename__="modelsas"id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True)name:sa_orm.Mapped[str]classModelDb(db.Model):__tablename__="modeldbs"id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True)name:sa_orm.Mapped[str]# Note that the typehint of `ModelSa.__init__` is wrong. It is not consistent with# the run-time usage. However, `ModelDb.__init__` is consistent. In otherwords,# both ModelSa(name="name") and ModelDb(name="name") can work at run time.reveal_type(ModelSa.__init__)# (self: ModelSa) -> Nonereveal_type(ModelDb.__init__)# (self: ModelDb, **kw: Any) -> Nonedeftest_class_init_dataclass()->None:classBaseDataClass(sa_orm.DeclarativeBase,sa_orm.MappedAsDataclass):passdb=SQLAlchemy(model_class=BaseDataClass)reveal_type(db)# SQLAlchemy[type[BaseDataClass]]reveal_type(db.Model)# type[_FSAModel_DataClass]classModelSa(BaseDataClass):__tablename__="modelsas"id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True,init=False)name:sa_orm.Mapped[str]classModelDb(db.Model):id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True,init=False)name:sa_orm.Mapped[str]# Well done! Now db.Model works in the same way compared to the base class.reveal_type(ModelSa.__init__    )# (self: ModelSa, name: SQLCoreOperations[str] | str) -> Nonereveal_type(ModelDb.__init__    )# (self: ModelDb, name: SQLCoreOperations[str] | str) -> Nonedeftest_not_allowed()->None:Pass1=sa_orm.declarative_base()reveal_type(Pass1)# Anyassertisinstance(Pass1,sa_orm.DeclarativeMeta)assertnotissubclass(Pass1,sa_orm.DeclarativeBase)assertnotissubclass(Pass1,sa_orm.DeclarativeBaseNoMeta)assertnotissubclass(Pass1,sa_orm.MappedAsDataclass)reveal_type(SQLAlchemy(model_class=Pass1))# SQLAlchemy[type[DeclarativeBase]]classPass2(metaclass=DefaultMeta):__fsa__=SQLAlchemy()registry=sa_orm.registry()__tablename__="pass2s"id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True)reveal_type(SQLAlchemy(model_class=Pass2))# SQLAlchemy[type[Pass2]]classPass3(Model):passreveal_type(SQLAlchemy(model_class=Pass3))# SQLAlchemy[type[Pass3]]classPass4(sa_orm.DeclarativeBase):passreveal_type(SQLAlchemy(model_class=Pass4))# SQLAlchemy[type[Pass4]]classPass5(sa_orm.DeclarativeBaseNoMeta):passreveal_type(SQLAlchemy(model_class=Pass5))# SQLAlchemy[type[Pass5]]classPass6(sa_orm.DeclarativeBase,sa_orm.MappedAsDataclass):passreveal_type(SQLAlchemy(model_class=Pass6))# SQLAlchemy[type[Pass6]]classNotPass1:pass# As expectation# Argument of type "type[NotPass1]" cannot be assigned to parameter "model_class"# of type "_FSA_MCT_T@SQLAlchemy" in function "__init__"SQLAlchemy(model_class=NotPass1)classNotPass2(sa_orm.DeclarativeMeta):pass# As expectation# Argument of type "type[NotPass2]" cannot be assigned to parameter "model_class"# of type "_FSA_MCT_T@SQLAlchemy" in function "__init__"SQLAlchemy(model_class=NotPass2)@dataclasses.dataclassclassNotPass3:a:int=dataclasses.field(default=1)# As expectation# Argument of type "type[NotPass3]" cannot be assigned to parameter "model_class"# of type "_FSA_MCT_T@SQLAlchemy" in function "__init__"SQLAlchemy(model_class=NotPass3)deftest_not_as_expectation()->Never:"""The following cases show the limitation of the current implementation. In the    following tests, such usages should not be allowed and will raise run time errors.    However, the current implementation cannot reveal the errors during the static    type checks.    I do not have a good idea to solve them. I think maybe these cases can be left as    they are.    """classBase(sa_orm.DeclarativeBase,sa_orm.MappedAsDataclass):passclassUnexpected1(Base):__tablename__="unexpecteds1"id:sa_orm.Mapped[int]=sa_orm.mapped_column(primary_key=True,init=False)a:sa_orm.Mapped[int]# `Unexpected1` should not be used as the model_class. But the type checker allows# this usage.reveal_type(SQLAlchemy(model_class=Unexpected1))# SQLAlchemy[type[Unexpected1]]classUnexpected2(sa_orm.MappedAsDataclass):pass# `Unexpected2` does not inherit `DeclarativeBase`. It should not be used as the# `model_class`. However, currently, we allow it. That's because Python does not# support typing like `Intersection[ParentClass1, ParentClass2]` yet. If we want# `SQLAlchemy` to be aware of `MappedAsDataclass`, this class has to be accepted# as the input candidate of `model_class`.reveal_type(SQLAlchemy(model_class=Unexpected2))# SQLAlchemy[type[Unexpected2]]raiseTypeErrorif__name__=="__main__":test_default()test_unknown_class(type)test_meta_class_v1(DefaultMeta)test_meta_class_v2(DefaultMeta)test_sqlalchemy2_base(type("new", (sa_orm.DeclarativeBase,),dict()))test_sqlalchemy2_nometa(type("new", (sa_orm.DeclarativeBaseNoMeta,),dict()))test_sqlalchemy2_dataclass(type("new", (sa_orm.MappedAsDataclass,sa_orm.DeclarativeBase),dict())    )test_sqlalchemy2_hybird()test_class_init_kw()test_class_init_kw_v2()test_class_init_dataclass()test_not_allowed()try:test_not_as_expectation()print("Not as expectation.")exceptException:print("As expectation.")

7erra, savchenko, edran, and oelhammouchi reacted with thumbs up emoji
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.
@cainmagicainmagi changed the titleFix issue 1312Fix issue #1312: Type (typehint) error when callingdb.Model subclass constructor with parametersMar 27, 2024
@coessiane

This comment was marked as off-topic.

@krbreyn
Copy link

krbreyn commentedNov 6, 2024
edited
Loading

Is there any update on this? I have been searching recently for a fix to avoid having to throw "# type: ignore" all over the code....

@cainmagi
Copy link
Author

Is there any update on this? I have been searching recently for a fix to avoid having to throw "# type: ignore" all over the code....

@lawnchairmeku I think I have already done what I can offer. Hopefully, the dev team will have time to review this. If I find this PR has conflict with the target branch, I will take actions to resolve the conflicts immediately, but currently, its condition is good.

This project is maintained by a community, so it may take very long for reviewing such a big update. If you intend to start a new project, I recommend this to you:
https://flask-sqlalchemy-lite.readthedocs.io/en/latest/

I felt a little bit regret that I did not choose it for my previous project.flask-sqlalchemy redefines the base model to provide extra functionalities, but I found that these functionalities are not necessary to me. If you have the same situation, it will be better to useflask-sqlalchemy-lite because it works better with the modern type hint system.

@cainmagi
Copy link
Author

Is there any update on this? I have been searching recently for a fix to avoid having to throw "# type: ignore" all over the code....

@lawnchairmeku After viewing your reply, I made a decision to migrate toflask-sqlalchemy-lite in my project. The SQLAlchemy definition in that project took around 1600 lines, but it only took me around one hour to make everything done. It is easier than I thought before. I think you can also consider doing that even if you have already usedflask-sqlachemy because it takes too long to see a feedback from this project.

@davidism
Copy link
Member

The maintainers of this project are the maintainers of lite as well. Just haven't has a lot of time to look at this with other responsibilities. We also recommend trying lite, it avoids the issues that this extensions customizations can create.

@cainmagi
Copy link
Author

The maintainers of this project are the maintainers of lite as well. Just haven't has a lot of time to look at this with other responsibilities. We also recommend trying lite, it avoids the issues that this extensions customizations can create.

@davidism Thank you for your suggestions! The lite version works very well to me. Please take your time, I am not intending to expedite your actions. If you find a better solution for this issue, please feel free to reject my PR.

@oelhammouchi
Copy link

oelhammouchi commentedFeb 28, 2025
edited
Loading

Hi, any idea when this could be integrated? It would a great QoL improvement!

EDIT: just found out about the saga with the type checkers, good grief. I see why this is isn't a 'quick fix' situation now, it's not a flask-sqlalchemy problem.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cainmagi@coessiane@krbreyn@davidism@oelhammouchi

[8]ページ先頭

©2009-2026 Movatter.jp