add mypy option to automatically infer no "Optional" for nullable=False

[MaicoTimmerman]

Is your feature request related to a problem? Please describe.
While trying to switch from dropbox/sqlalchemy mypy plugin to the official sqlalchemy plugin, I found that in these stubs nullable and primary_key have no effect on the Colum types.

Describe the solution you'd like
Currently the inferred type is always a Union[..., None], however based on nullable and primary_key properties of the Column we could infer that columns should never contain None.

Describe alternatives you've considered
N/A

Additional context

from sqlalchemy.schema import Column
from sqlalchemy.types import Integer
from sqlalchemy.orm.decl_api import declarative_base

Base = declarative_base()

class A(Base):
    __tablename__ = "a"
    id = Column(Integer, primary_key=True, nullable=False)
    a = Column(Integer, nullable=True)

a = A()
reveal_type(a.id)
reveal_type(a.a)

Results from plugins = sqlalchemy.ext.mypy.plugin:

test3.py:14: note: Revealed type is 'Union[builtins.int, None]'
test3.py:15: note: Revealed type is 'Union[builtins.int, None]'
test3.py:16: note: Revealed type is 'Union[builtins.int, None]'

Results from plugins = sqlmypy:

test3.py:14: note: Revealed type is 'builtins.int*'
test3.py:15: note: Revealed type is 'Union[builtins.int*, None]'
test3.py:16: note: Revealed type is 'builtins.int*'

Have a nice day!

[zzzeek]

hiya -

May I ask that you read https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#introspection-of-columns-based-on-typeengine and come back to me with your comments? There is a documented rationale for the current behavior.

[MaicoTimmerman]

Hi, Thanks for the link to the documentation, I missed that rationale.

That said, I'd like to advocate for adapting this to respect the nullability of the column. To my knowledge, nullable columns are only None when they transient. This leaves small window for the user to be able to use an attribute, while it is still None. The current rationale makes the user responsible for always verifying that an attribute is not None, even when it is attached to a session (most of the time).

This covers the usage of the column as expression, but opens for errors when used as a left-hand side of an assignment statement. The column will now accept None for assignment, without mypy detecting it:

from sqlalchemy import create_engine, select
from sqlalchemy.orm import Session, registry
from sqlalchemy.schema import Column
from sqlalchemy.types import Integer

mapper_registry: registry = registry()
engine = create_engine('sqlite:///foo.db')

@mapper_registry.mapped
class A:
    __tablename__ = "a"
    id = Column(Integer, primary_key=True, nullable=False)
    a = Column(Integer, nullable=False)

with Session(engine) as session:
    a: A = session.execute(select(A)).scalar_one()
    a.a = None
    session.commit()

Resulting in sqlalchemy.exc.IntegrityError: (sqlite3.IntegrityError) NOT NULL constraint failed: a.a.

This default behaviour can be overwritten by manually marking the column with type. To be safe from these assignments, all declared classes would need manual type annotations.

I have no data to support it, however I feel that accidentally assigning None to NOT NULL columns is far more common then using a transient attribute that has not been set. Given that there is no way to correctly type both situations, I'd argue types respecting column nullability is the safer default.

Looking forward to your opinion!

[zzzeek]

Hi, Thanks for the link to the documentation, I missed that rationale.

That said, I'd like to advocate for adapting this to respect the nullability of the column. To my knowledge, nullable columns are only None when they transient. This leaves small window for the user to be able to use an attribute, while it is still None. The current rationale makes the user responsible for always verifying that an attribute is not None, even when it is attached to a session (most of the time).

it depends on what people are doing. It would be wrong for the plugin to infer that the attribute is never None, when it certainly can be. This is why it's also very easy to override, if you don't want to worry about the None-ness by using an explicit "Mapped[type]" annotation. I'd rather have users opt-in to that so they know what they are doing.

This covers the usage of the column as expression, but opens for errors when used as a left-hand side of an assignment statement. The column will now accept None for assignment, without mypy detecting it:

from sqlalchemy import create_engine, select
from sqlalchemy.orm import Session, registry
from sqlalchemy.schema import Column
from sqlalchemy.types import Integer

mapper_registry: registry = registry()
engine = create_engine('sqlite:///foo.db')

@mapper_registry.mapped
class A:
    __tablename__ = "a"
    id = Column(Integer, primary_key=True, nullable=False)
    a = Column(Integer, nullable=False)

with Session(engine) as session:
    a: A = session.execute(select(A)).scalar_one()
    a.a = None
    session.commit()

Keeping in mind that I understand people are going to want this behavior and it is planned to add an option to setup.cfg that will allow the old plugin's behavior in this area (e.g., your problem will be solved in the way you want it to), I still think this is wrong. This is asking the mypy plugin, which is tasked with checking for Python type safety, to serve as a database-level validation tool, which is not its purpose. From a Python pov the code is valid. The database then does its job of telling you that on that end, it's not valid. A key philosophy of SQLAlchemy is to not duplicate the work that the database already does. Code that explicitly sets a non nullable col to None will fail immediately and assuming the developer is running rudimentary unit tests against their sofware will find this problem immediately.

once again, when I made this decision, I fully expected people to want the "old" behavior and am totally fine adding a flag in setup.cfg to do so.

This default behaviour can be overwritten by manually marking the column with type. To be safe from these assignments, all declared classes would need manual type annotations.

I have no data to support it, however I feel that accidentally assigning None to NOT NULL columns is far more common then using a transient attribute that has not been set. Given that there is no way to correctly type both situations, I'd argue types respecting column nullability is the safer default.

Looking forward to your opinion!

SQLAlchemy doesn't enforce non-nullability at the object level, so mypy plugins default behavior is to match this. if SQLAlchemy did in fact create a declarative constructor that checked for "None" and also automatically attached validators to Python attributes that detected nullability, then this would be more appropriate. No problem adding an option to do this but from my pov it's not a complete solution unless SQLAlchemy moved to fully interpret "nullable=False" within the ORM attribute and constructor system.

[zzzeek]

note my idea at sqlalchemy/sqlalchemy2-stubs#170 (comment) for a new overlay on declarative that would allow declarative to introspect from annotations the way dataclasses does, that would be the more "shorthand" way of doing mappings and IIRC it also skips most of the mypy plugin.

[zzzeek]

This is now implemented for SQLAlchemy 2.0 using mapped_column, which provides a new approach to mapping columns that is typing-fluent. the mypy plugin is now legacy.