Flask-SQLAlchemy: Make model query non-generic

[mano7onam]

No description provided.

[AlexWaygood]

Cc. @kkirsche

[kkirsche]

Cc. @kkirsche

Ah, that makes sense as in 99% of cases, Model will be used as a parent class to the specific model where these properties exist. Sorry about that

[AlexWaygood]

This looks plausible to me, but I don't really feel qualified to review this -- I don't know my way around the Flask-SQLAlchemy codebase or the SQLAlchemy codebase very well. I'd appreciate it if another maintainer could also take a look.

[kkirsche]

I think using a property may cause issues for code like this:

        user = User.query.filter(User.username == username.lower()).first()

In which case I'm seeing:

overloaded function has no attribute `filter` 

and Pylance in VSCode is telling me:

Cannot access member `filter` for type `property`

When I've set this up using the following declarative base for my project locally:

from sqlalchemy import MetaData
from sqlalchemy.orm import DeclarativeMeta, registry, Query
from typing import TypeVar

# The following naming conventions are in use for constraints, but additional
# naming conventions may be imposed by the team if multiple sub-applications exist.
convention = {
    "ix": "ix_%(column_0_label)s",
    "uq": "uq_%(table_name)s_%(column_0_name)s",
    "ck": "ck_%(table_name)s_%(constraint_name)s",
    "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
    "pk": "pk_%(table_name)s",
}
metadata = MetaData(naming_convention=convention)
mapper_registry = registry(metadata=metadata)

Self = TypeVar("Self", bound="Base")


class Base(metaclass=DeclarativeMeta):
    """The base class of SQLAlchemy models.

    See the link below for details about this class:
        https://docs.sqlalchemy.org/en/14/orm/declarative_styles.html#creating-an-explicit-base-non-dynamically-for-use-with-mypy-similar     # noqa
    """

    __abstract__ = True

    registry = mapper_registry
    metadata = mapper_registry.metadata

    # These aren't actually properties at runtime, but we treat them as such so we can
    # use `self` type without using typing.Self before Python 3.11
    @property
    def query_class(self: Self) -> type["Query[Self]"] | None:
        ...

    @query_class.setter
    def query_class(self: Self, value: type["Query[Self]"] | None) -> None:
        ...

    @property
    def query(self: Self) -> "Query[Self]" | None:
        ...

    @query.setter
    def query(self: Self, value: "Query[Self]" | None) -> None:
        ...

    __init__ = mapper_registry.constructor

[AlexWaygood]

I think using a property may cause issues for code like this:

        user = User.query.filter(User.username == username.lower()).first()

Is it idiomatic to access query as a class attribute in that way? If it is, then maybe the best course of action would just be to revert the changes made to Model in #8389 -- what do you think?

[kkirsche]

I think using a property may cause issues for code like this:

        user = User.query.filter(User.username == username.lower()).first()

Is it idiomatic to access query as a class attribute in that way? If it is, then maybe the best course of action would just be to revert the changes made to Model in #8389 -- what do you think?

I would agree with that.

It is idiomatic and is demonstrated in their documentation:
https://flask-sqlalchemy.palletsprojects.com/en/2.x/queries/#querying-records

What's happening is normally, SQLAlchemy models do not have the query_property unless you add it:
https://docs.sqlalchemy.org/en/13/orm/contextual.html#sqlalchemy.orm.scoping.scoped_session.query_property

Flask-SQLAlchemy does the work of automating this process for you, so that (historically only when a request is created and dispatched to the WSGI route but now anytime), the query property will be there. This is done here:
https://github.com/pallets-eco/flask-sqlalchemy/blob/main/src/flask_sqlalchemy/__init__.py#L841

So the intent of this was to make the Query[T] attribute that Flask-SQLAlchemy is automating visible via the type hint. The generic would allow it to use itself, while Self seems to be limited currently such that it's not possible to represent this (as I can't seem to make a generic bound to Self, so it changes based on the current Self).

As a result, I would lean towards reverting the changes to model from #8389 until either a more appropriate solution can be determined or the behavior of Self is sufficiently expanded/matured to support this type of use case.

[kkirsche]

The main problem that I was looking to solve, just to close the loop was the need for cast:

cast("Query[User]", User.query).filter(User.username == username.lower()).first()

That often run rampant throughout a codebase when using this style of retrieval without having the generic class attribute

[AlexWaygood]

Thanks @kkirsche, that's really helpful :)

@mano7onam, given this, could you possibly change your PR so that it's a simple revert of the changes made to Model in #8389? I think the sad fact of the matter is that there isn't really a good way of typing this class at present, with the current type system :(

[AlexWaygood]

Thanks for the PR, and sorry for the delay on this! I realised there was a middle-ground where we could make it non-generic, but without throwing out all of the improvements @kkirsche added. I've gone ahead and applied my idea, and I'll merge when the CI is green :)

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[kkirsche]

Thanks for your help with this, Alex. I hope to be around more in the coming weeks.

Thank you @mano7onam for your help with getting this fixed. Sorry for the error in the first place!