🏷️ Adjust type annotation for Field.sa_type to support instantiated SQLAlchemy types

[diachkow]

Note

When I was writing description for this PR, I found another discussion started for just the same issue I was experiencing with mypy, so this changes are basically fixing the issue described here

Using sa_type and sa_column_kwargs instead of just sa_column can benefit when using inheritance for classes derived from SQLModel as it was suggested here.

If sa_column is not specified and sa_type is provided, it will be passed as a second, type argumnet to sqlalchemy.Column instance. If you would check sqlalchemy.Column construction definition, it looks as following:

    def __init__(
        self,
        __name_pos: Optional[
            Union[str, _TypeEngineArgument[_T], SchemaEventTarget]
        ] = None,
        __type_pos: Optional[
            Union[_TypeEngineArgument[_T], SchemaEventTarget]
        ] = None,
        *args: SchemaEventTarget,
        name: Optional[str] = None,
        type_: Optional[_TypeEngineArgument[_T]] = None,
        autoincrement: _AutoIncrementType = "auto",
        default: Optional[Any] = _NoArg.NO_ARG,
        insert_default: Optional[Any] = _NoArg.NO_ARG,
        doc: Optional[str] = None,
        key: Optional[str] = None,
        index: Optional[bool] = None,
        unique: Optional[bool] = None,
        info: Optional[_InfoType] = None,
        nullable: Optional[
            Union[bool, Literal[SchemaConst.NULL_UNSPECIFIED]]
        ] = SchemaConst.NULL_UNSPECIFIED,
        onupdate: Optional[Any] = None,
        primary_key: bool = False,
        server_default: Optional[_ServerDefaultArgument] = None,
        server_onupdate: Optional[_ServerOnUpdateArgument] = None,
        quote: Optional[bool] = None,
        system: bool = False,
        comment: Optional[str] = None,
        insert_sentinel: bool = False,
        _omit_from_statements: bool = False,
        _proxies: Optional[Any] = None,
        **dialect_kwargs: Any,
    ):

Note the __type_pos argument with Union[_TypeEngineArgument[_T], SchemaEventTarget] where

_TypeEngineArgument = Union[Type["TypeEngine[_T]"], "TypeEngine[_T]"]

So, from technical perspective you can pass not only the subclass of TypeEngine, e.g. SQLAlchemy's sqltype such as String, Integer, DateTime, JSON etc, but also an instance of this type.

I was trying for JSONB(none_as_null=True) and String(50) and it worked just fine, alembic migrations were generated correctly, only mypy was arguing for type mismatch with call-overload issue.

To fix mypy error, we can update type annotation for sqlmodel.main.Field.sa_type to support also an instantiated SQLAlchemy's sqltype to match those of sqlalchemy.Column

Related discussions:

• Typing errors when using `sa_type` and "specific" SQLAlchemy types #1228
• Bad type annotation for sa_type #1669
• Using sa_type causes Mypy type error #955

[YuriiMotov]

LGTM

Simple code example to check (in the details)

Details

from datetime import datetime

from sqlmodel import DateTime, Field, SQLModel, create_engine

class A(SQLModel):
    created_at: datetime = Field(sa_type=DateTime(timezone=False))

engine = create_engine("sqlite:///")
SQLModel.metadata.create_all(engine)

Running mypy gives

error: No overload variant of "Field" matches argument type "DateTime"  [call-overload]

on master and

Success: no issues found in 1 source file

after applying this fix

[YuriiMotov]

LGTM
See #1345 (review)

[diachkow]

Thanks for approval! Who is responsible for merging this, what are the rules in this repo?

[YuriiMotov]

Thanks for approval! Who is responsible for merging this, what are the rules in this repo?

Only Sebastian can merge it. I already forwarded it to him. We should just wait

[diachkow]

I have resolved conflicts here. I see you guys made some patch releases recently, hope this change can make it into one of them 🙃