🏷️ 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 🙃

[AgamjotSB]

was hitting this error when using sa_type, thanks for keeping it updated

[diachkow]

It would be nice to get it merged though 😅

[AgamjotSB]

Hey @tiangolo, gently bumping this. It's been open about a year now, has an LGTM from @YuriiMotov, and @svlandeg mentioned back in September she'd forwarded it to you for merging.

To recap the issue:

• Runtime is fine: sa_type already works correctly at runtime (SQLAlchemy accepts instantiated types like DateTime(timezone=False) just fine).
• Typing is failing: The type annotation on Field.sa_type doesn't reflect that, so type checkers (basedpyright, zuban, mypy) flag valid code as an error.
• Impact: It doesn't break anything at runtime, but it causes CI pipelines to fail.

Worth noting this is really a bug fix, not a feature. It's currently labeled as feature, but the code already does the right thing, this PR just corrects the type signature to match. Might be worth relabeling!

Totally understand if it's just buried under everything else, I didn't want it to get lost. Thanks for all the incredible work you do on FastAPI and SQLModel (and friends) :)