Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting __get__ and __set__ on Column object #170

Open
rslinckx opened this issue Sep 1, 2021 · 108 comments · May be fixed by #174
Open

Supporting __get__ and __set__ on Column object #170

rslinckx opened this issue Sep 1, 2021 · 108 comments · May be fixed by #174
Labels
enhancement New feature or request

Comments

@rslinckx
Copy link

rslinckx commented Sep 1, 2021

The old sqlalchemy-stubs from dropbox included an approximation for the behavior of mapped classes using declarative setup, that is

class Foo(Model):
  a = Column(DateTime)

Foo.a
# type is Column[DateTime]
Foo().a
# type is datetime

They did so by defining the get method on Column here: https://github.com/dropbox/sqlalchemy-stubs/blob/master/sqlalchemy-stubs/sql/schema.pyi#L131

Maybe the same can be done for set, so that

Foo().a = 3
# wrong type, expected datetime object

Of course the whole thing is supported already when using the mypy plugin, but for example when using pylance (vscode type engine) which does not support plugins, it gets confused when using and assigning object values as mapped columns and complains about mismatching types.

I think the same is done on relationship() property

I'm unsure of the deeper implications of adding such type hints, though

@rslinckx rslinckx added the requires triage New issue that requires categorization label Sep 1, 2021
@CaselIT CaselIT added enhancement New feature or request and removed requires triage New issue that requires categorization labels Sep 1, 2021
@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2021

id have to look more but at the moment I'm +0 leaning towards -1 because it seems like it wont work consistently for all kinds of mapper scenarios, also our "_T" is not "int" it's Integer, so it may not be possible in any case.

@CaselIT
Copy link
Member

CaselIT commented Sep 1, 2021

Hi,

As mike mentioned, I think the best we can do is to return Any in every column, since we don't have a python type in the column. That's assuming it does not pose a problem for the plugin

@rslinckx
Copy link
Author

rslinckx commented Sep 1, 2021

I think what they've done in the dropbox stub is reassign the various sqlalchemy types to their 'equivalent' python types, for better or worse (aka Integer = int). I have to admit it's not particularly pretty and probably wrong in many cases. I'm just looking for a way to be able to use the pylance type checker without having every other line underlined because "Column[Integer] can't be assigned int" which makes it really hard to get proper type checking

@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2021

yeah i specifically did not want to do that, and I had assumed the plugin would be relied upon. im hoping tools like pylance add logic that's specific to SQLAlchemy for this since other Python libs including dataclasses etc. need special rules too.

@rslinckx
Copy link
Author

rslinckx commented Sep 1, 2021

Would there be a way to define some kind of manual type where Class.col would return the regular Column[Integer] from sqlalchemy, and Class().col would return a manually defined python type, something like:

class Foo:
   a: ManuallySpecifiedColumn[int, Integer] = Column(Integer)

?

@rslinckx
Copy link
Author

rslinckx commented Sep 1, 2021

yeah i specifically did not want to do that, and I had assumed the plugin would be relied upon. im hoping tools like pylance add logic that's specific to SQLAlchemy for this since other Python libs including dataclasses etc. need special rules too.

I think they've made it pretty clear they would never add anything specific which is not in the stdlib :(

@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2021

its a bug in pylance. extensions are needed for Python typing, they need to support them.

@rslinckx
Copy link
Author

rslinckx commented Sep 1, 2021

See for example: microsoft/pylance-release#845 (comment)

@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2021

hybrid properties should work completely, they are plain descriptors, @CaselIT do we not have hybrids working yet?

@zzzeek
Copy link
Member

zzzeek commented Sep 1, 2021

wow apparntly not. i had a complete working vresion of hybrids i have to find it now

@CaselIT
Copy link
Member

CaselIT commented Sep 1, 2021

hybrid properties should work completely, they are plain descriptors, @CaselIT do we not have hybrids working yet?

I was not aware we had a problem there :)

I'll try looking if supporting returning Any form the __get__ works or if it causes issues to mypy.

One thing to note is that the columns are not descriptors in code

@CaselIT
Copy link
Member

CaselIT commented Sep 1, 2021

@rslinckx can you try #172 ?

@CaselIT
Copy link
Member

CaselIT commented Sep 1, 2021

I've made class level column return mapped, since that's what is actually there at runtime, instead of column as in the dropbox stubs

@rslinckx
Copy link
Author

rslinckx commented Sep 3, 2021

The pull request does suppress the red wiggles, in that it now considers an object-level as Any, and so is compatible with anything. it does not suppress errors on the set side, though so Obj().a = 4 will trigger an error because int is not compatible with Column[str]

I was wondering if it would be acceptable to add a Generic parameter to the Column class representing the python type of the data coming in and out of the column so that get can return either -> Mapped[python type] at class level or -> python type at object level?

@CaselIT
Copy link
Member

CaselIT commented Sep 3, 2021

though so Obj().a = 4 will trigger an error because int is not compatible with Column[str]

is this when using pyright?

I was wondering if it would be acceptable to add a Generic parameter to the Column class representing the python type of the data coming in and out of the column

this is not feasible at the moment, without changes at how the plugin and the stubs work,
It was an explicit choice to make Column generic on the type engine object and not on the python type, since this is more similar to how the code actually works.

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

I want to look into moving to a new style of declarative that eliminates these issues. we already have declarative forms that work fully without a plugin, such as https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#mapping-columns-with-imperative-table ; the second example at https://docs.sqlalchemy.org/en/14/orm/extensions/mypy.html#what-the-plugin-does shows how Mapped is used by the plugin.

I think for this issue, instead of creating more types that "lie", we should introduce new declarative forms that produce correct typing up front. I propose using Mapped and/or some new kind of declarative form that simply does all the things that we want.

Here's a very dramatically different form, which is not really correct but something like this, which could easily ride on top of the existing declarative mechanics:

class A(Base):
    __tablename__ = 'a'

   id = m.int(primary_key=True)
   name = m.string(datatype=String)
   some_expr = m.int(expr=<some sql expression>)
   bs = m.related("B")

there's no reason we have to keep having people use Column etc in their mappings if all the tools are working against it. let's work with the tools

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

I was wondering if it would be acceptable to add a Generic parameter to the Column class representing the python type of the data coming in and out of the column so that get can return either -> Mapped[python type] at class level or -> python type at object level?

IMO it's a failure of the typing system that a type that is essentially Column[Integer[int]] can't be represented without losing features. Instead of Column[int] we have Mapping[int] and we should just try to make the latter do what we need. Column should really not have anything to do with this.

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

are you using pylance in "basic" type checking mode?

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

I think I just need to know what we want here. Do we want to be typing out [int] [str] etc in our mappings? Here's a quick POC, I get no squigglys in pylance basic mode, and mypy passes too, we wouldnt even need the plugin here except for the __init__ method:

from sqlalchemy import Column
from sqlalchemy import create_engine
from sqlalchemy import ForeignKey
from sqlalchemy import Integer
from sqlalchemy import String
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import registry
from sqlalchemy.orm import relationship
from sqlalchemy.orm import Session
from typing import Any


class M:
    def __or__(self, other: Any) -> Any:
        return other


m = M()

r: registry = registry()


@r.mapped
class A:
    __tablename__ = "a"

    id: Mapped[int] = m | Column(Integer, primary_key=True)
    data: Mapped[str] = m | Column(String)
    bs: "Mapped[B]" = m | relationship("B")


@r.mapped
class B:
    __tablename__ = "b"
    id: Mapped[int] = m | Column(Integer, primary_key=True)
    a_id: Mapped[int] = m | Column(ForeignKey("a.id"))
    data: Mapped[str] = m | Column(String)


e = create_engine("sqlite://", echo=True)
r.metadata.create_all(e)

s = Session(e)

a1 = A()
x: str = a1.data

a1.data = "some str"

expr = A.data == 5

The original sqlalchemy stubs only has Any for the types coming from Column. Without a plugin, they can't determine Column[int] either unless it is declared in the code.

I propose looking into helpers like the above for this use case.

@CaselIT
Copy link
Member

CaselIT commented Sep 3, 2021

class A(Base):
tablename = 'a'

id = m.int(primary_key=True)
name = m.string(datatype=String)
some_expr = m.int(expr=)
bs = m.related("B")

I don't personally like that example. It makes it very hard to understand what's being defined as a table.

I don't think we need to actually to change the declaration of something different, we can just change how the init works. (I'm not sure if it's possible also when not using a base class).

Like how pydantic works https://github.com/samuelcolvin/pydantic/blob/5ccbdcb5904f35834300b01432a665c75dc02296/pydantic/main.py#L118-L267
or even sqlmodel: https://github.com/tiangolo/sqlmodel/blob/02da85c9ec39b0ebb7ff91de3045e0aea28dc998/sqlmodel/main.py#L232-L369

If I'm not mistaken they remove the FieldInfo type from the type checking and set the correct type

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

the __init__ of what? Column? I dont want to change Column at all. I think it's time we change that declarative points to things that aren't declarative attributes. this decision wouldn't have been made in a pep 484 world.

@CaselIT
Copy link
Member

CaselIT commented Sep 3, 2021

no of the class

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

how does that help the case where we access A().data ?

the little trick I have above works even if you don't give it a type, this seems too easy so i must be missing something

class M:
    def __or__(self, other: Any) -> Any:
        return other


m = M()

r: registry = registry()


@r.mapped
class A:
    __tablename__ = "a"

    id = m | Column(Integer, primary_key=True)
    data = m | Column(String)
    bs: "Mapped[B]" = m | relationship("B")

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

it's essentially just putting cast(Any) around it, with less (keyboard) typing

@zzzeek
Copy link
Member

zzzeek commented Sep 3, 2021

this would require code changes and plugin changes but what about a syntax like this:

@r.mapped
class A:
    __tablename__ = "a"

    # will figure out Mapped[int]
    id = m(int) | Column(Integer, primary_key=True)

    # or we can be explicit
    data  : Mapped[str] = m | Column(String)

   # or just omit it, and it works out as Any, we'd get the plugin to detect this for real typing

   x = m | Column(Integer)

@CaselIT
Copy link
Member

CaselIT commented Sep 7, 2021

doubled parenthesis are hard to navigate and read, just to type the above example I had several mis-counted parenthesis.

Indeed! This setting they just added to vscode is something I did not know I was missing https://code.visualstudio.com/updates/v1_60#_high-performance-bracket-pair-colorization

@layday
Copy link
Contributor

layday commented Sep 7, 2021

If the only meaning | is meant to convey is that of saving an extra set of brackets, that's just bound to confuse people. > as a replacement for gt is obvious; it's a mathematical symbol used in a mathematical setting.

@layday
Copy link
Contributor

layday commented Sep 7, 2021

Sorry, I missed this line:

and it also makes clear that the "M" thing is not part of our code

It doesn't make that clear at all, at least not to me, as someone who's tried to keep up with typing in Python. My initial impression is that it might be combining column properties, but I might be misinterpreting "M"?

@zzzeek
Copy link
Member

zzzeek commented Sep 7, 2021

There's currently no way to solve this problem without giving something up:. plugin required, change sqlalchemy objects to have the incorrect return type (like a Column function that types as Any, people would have to change their imports and all their code, have confusing errors when they use the wrong Column symbol), require cumbersome parentheses , don't use full declarative, use a special operator convention, or just use entirely new functions for declarative like m_column() or something like that

@CaselIT
Copy link
Member

CaselIT commented Sep 7, 2021

There's currently no way to solve this problem without giving something up:. plugin required, change sqlalchemy objects to have the incorrect return type (like a Column function that types as Any, people would have to change their imports and all their code, have confusing errors when they use the wrong Column symbol), require cumbersome parentheses , don't use full declarative, use a special operator convention, or just use entirely new functions for declarative like m_column() or something like that

I agree. Just to add that all the above solution not necessary need to be in "or". I think an OrmColumn function may be something we may want to consider regardless of other solutions, since the maintenance burden is basically 0 for it.

@zzzeek
Copy link
Member

zzzeek commented Sep 7, 2021

Doesn't work for relationship(), column_property(), etc. Basically we just have to copy pydantic, and have all of these constructs typed as Any. Then for the singular Column we just have to replace it entirely throughout all docs with orm_column(), same naming convention as everything else in the ORM, also typed as Any.

This is really ugly because all of these functions are used in other contexts, such as the dictionary you can send to map_imperatively(), which probably breaks with a change such as this. So I don't see how this is not instead a whole new suite of functions that copy all the ones we have but add this specific Any hack (eg cast_any_relatuonshop(), cast_any_column_property(), etc). Or, we add some kind of cast method like Column(...).cast_as_any(). If the concern is over clarity then I guess it has to have a full name but at least that fixes the nested parenthesis problem.

There is a point here which is anything we do that isn't extremely verbose will not be meaningful to someone not familiar with the convention. M(Column()) is no more meaningful to me than M | Column(), just one is easier to read and type and also suggests that this M doesn't actually do anything code wise.

@CaselIT
Copy link
Member

CaselIT commented Sep 7, 2021

Doesn't work for relationship(), column_property(), etc

I don't think this would be a problem, since we can just type them to return Any since they are pretty much functions.

The issue is would be the Column. I think we could cast it as Any somehow but we would then loose al typing on it for every usage, so that's probably not the best solution.

Personally I don't know what I would choose as solution, since they all seem less than optimal. It may be best to wait and see if something changes in the typing in stuff

In the meantime maybe adding the description protocol to the columns could be a temporary solution?

@zzzeek
Copy link
Member

zzzeek commented Sep 9, 2021

im wondering why we cant just have all the ORM things implement Mapped[] since it is just an interface . then we just need orm_column() and we dont necessarily have to do Any.

@zzzeek
Copy link
Member

zzzeek commented Sep 9, 2021

mapped is not an interface, it subclasses queryable attribute...facepalm, why is that

@zzzeek
Copy link
Member

zzzeek commented Sep 9, 2021

here's one way we can "lie" right now, but we can't do this in the real code. i will try to reorganize Mapped to be at the base:

diff --git a/sqlalchemy-stubs/orm/__init__.pyi b/sqlalchemy-stubs/orm/__init__.pyi
index bc5a75e..d221d2b 100644
--- a/sqlalchemy-stubs/orm/__init__.pyi
+++ b/sqlalchemy-stubs/orm/__init__.pyi
@@ -87,6 +88,8 @@ composite = CompositeProperty
 
 _BackrefResult = Tuple[str, Mapping[str, Any]]
 
+def mapped_column(*arg: Any, **kw: Any) -> Mapped[Any]: ...
+
 def backref(name: str, **kwargs: Any) -> _BackrefResult: ...
 def deferred(*columns: Any, **kw: Any) -> ColumnProperty: ...
 def query_expression(default_expr: Any = ...) -> ColumnProperty: ...
diff --git a/sqlalchemy-stubs/orm/interfaces.pyi b/sqlalchemy-stubs/orm/interfaces.pyi
index 6fabe33..bd19461 100644
--- a/sqlalchemy-stubs/orm/interfaces.pyi
+++ b/sqlalchemy-stubs/orm/interfaces.pyi
@@ -15,6 +15,7 @@ from .base import NOT_EXTENSION as NOT_EXTENSION
 from .base import ONETOMANY as ONETOMANY
 from .mapper import Mapper
 from .util import AliasedInsp
+from . import attributes
 from .. import util
 from ..sql import operators
 from ..sql import roles
@@ -29,11 +30,11 @@ class ORMFromClauseRole(roles.StrictFromClauseRole): ...
 _T = TypeVar("_T")
 
 class MapperProperty(
+    attributes.Mapped[_T],
     HasCacheKey,
     _MappedAttribute,
     InspectionAttr,
     util.MemoizedSlots,
-    Generic[_T],
 ):
     cascade: Any = ...
     is_property: bool = ...

so code looks like :

@reg.mapped
class A:
    __tablename__ = 'a'

    id: Mapped[int] = mapped_column(Integer, primary_key=True)
    data: Mapped[str] = mapped_column(String)
    bs: Mapped["B"] = relationship("B")


@reg.mapped
class B:
    __tablename__ = 'b'
    id = mapped_column(Integer, primary_key=True)
    a_id = mapped_column(ForeignKey("a.id"))
    data = mapped_column(String)


a1 = A()

x: int = a1.id

a1.id = 5

this works. tests even pass. so we will try to make Mapped by itself have all the comparison operations etc. of QueryableAttribute, move it into orm/interfaces or similar, then make it also the base of MappedProperty. columns will use mapped_column(). this is the only way for this to work w/o plugins or akward syntaxes

@zzzeek
Copy link
Member

zzzeek commented Sep 9, 2021

there's not an easy way to have MapperProperty extend from Mapped where Mapped is then providing methods like __eq__() etc. it might be possible but all the internals that do things like "some_prop in set()" have to be changed. this is a huge change to basically work around a "lie" in the typing system.

it's this issue of "lying", which pydantic can get away with because the scope of "field()" is so much more narrow, that I don't think is a good idea for us to be deeply embedded. I wanted to keep the "lie" in a very narrow and explicit scope, that is, in some obvious directive in a mapping.

@zzzeek
Copy link
Member

zzzeek commented Sep 9, 2021

there's also this way, which is to do more or less just what field() does, this is a heavy burden

diff --git a/sqlalchemy-stubs/orm/__init__.pyi b/sqlalchemy-stubs/orm/__init__.pyi
index bc5a75e..c95cc71 100644
--- a/sqlalchemy-stubs/orm/__init__.pyi
+++ b/sqlalchemy-stubs/orm/__init__.pyi
@@ -46,6 +46,7 @@ from .properties import ColumnProperty as ColumnProperty
 from .query import AliasOption as AliasOption
 from .query import FromStatement as FromStatement
 from .query import Query as Query
+from .relationships import _OrderByArgument
 from .relationships import foreign as foreign
 from .relationships import RelationshipProperty as RelationshipProperty
 from .relationships import remote as remote
@@ -74,18 +75,60 @@ from .util import was_deleted as was_deleted
 from .util import with_parent as with_parent
 from .util import with_polymorphic as with_polymorphic
 
+from typing import Optional, Union, AbstractSet, Callable, Literal, Sequence, MutableMapping, Type
+
 def create_session(bind: Optional[Any] = ..., **kwargs: Any) -> Session: ...
 
 with_loader_criteria = LoaderCriteriaOption
-relationship = RelationshipProperty
 
-def relation(*arg: Any, **kw: Any) -> RelationshipProperty[Any]: ...
+
+_BackrefResult = Tuple[str, Mapping[str, Any]]
+
+def relationship(
+        argument: Any,
+        secondary: Optional[Any] = ...,
+        primaryjoin: Optional[Any] = ...,
+        secondaryjoin: Optional[Any] = ...,
+        foreign_keys: Optional[Any] = ...,
+        uselist: Optional[bool] = ...,
+        order_by: _OrderByArgument = ...,
+        backref: Union[str, _BackrefResult] = ...,
+        back_populates: str = ...,
+        overlaps: Union[AbstractSet[str], str] = ...,
+        post_update: bool = ...,
+        cascade: Union[Literal[False], Sequence[str]] = ...,
+        viewonly: bool = ...,
+        lazy: str = ...,
+        collection_class: Optional[Union[Type[Any], Callable[[], Any]]] = ...,
+        passive_deletes: Union[bool, Literal["all"]] = ...,
+        passive_updates: bool = ...,
+        remote_side: Optional[Any] = ...,
+        enable_typechecks: bool = ...,  # NOTE: not documented
+        join_depth: Optional[int] = ...,
+        comparator_factory: Optional[Any] = ...,
+        single_parent: bool = ...,
+        innerjoin: Union[bool, str] = ...,
+        distinct_target_key: Optional[bool] = ...,
+        doc: Optional[str] = ...,
+        active_history: bool = ...,
+        cascade_backrefs: bool = ...,
+        load_on_pending: bool = ...,
+        bake_queries: bool = ...,
+        _local_remote_pairs: Optional[Any] = ...,
+        query_class: Optional[Any] = ...,
+        info: Optional[MutableMapping[Any, Any]] = ...,
+        omit_join: Optional[Literal[False]] = ...,
+        sync_backref: Optional[Any] = ...,
+
+) -> Mapped[Any]: ...
+
+def relation(*arg: Any, **kw: Any) -> Mapped[Any]: ...
 def dynamic_loader(argument: Any, **kw: Any) -> RelationshipProperty[Any]: ...
 
-column_property = ColumnProperty
-composite = CompositeProperty
+def column_property(*arg: Any, **kw: Any) -> ColumnProperty: ...
+
+def composite(*arg: Any, **kw: Any) -> CompositeProperty[Any]: ...
 
-_BackrefResult = Tuple[str, Mapping[str, Any]]
 
 def backref(name: str, **kwargs: Any) -> _BackrefResult: ...
 def deferred(*columns: Any, **kw: Any) -> ColumnProperty: ...

@zzzeek
Copy link
Member

zzzeek commented Sep 9, 2021

which is because relationship() doesn't return Mapped[Any], I guess we can make it return Any, dunno.

zzzeek added a commit that referenced this issue Sep 9, 2021
all ORM functions that are used in declarative are now
made separate from the classes they refer towards in all
cases and indicate the return type of Any so they can be
used inline with Mapped[_T] in declarative mappings
such that IDEs can deliver Mapped[] behavior without any
linter errors.

this will require changes in the mypy plugin as we
can no longer rely on simple things like "composite()" returning
CompositeProperty.

References: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3074

Fixes: #170
@zzzeek zzzeek linked a pull request Sep 9, 2021 that will close this issue
3 tasks
@zzzeek
Copy link
Member

zzzeek commented Sep 10, 2021

hi can folks here review this proposed new API / syntax? This adds a new import space "m" that has new symbols that handle typing correctly, without any lying and without the need for plugins (plugin still needed for Base class and __init__ method). this proposal would be a new system that would be soft roll out in 1.4 and then in 2.0 would be much more prominent. Where possible, it determines the python type from the SQL type (yes, that is doable). For relationships, it works the other way around by default since we usually need an annotation like "List[X]" or similar and that can't be passed as an argument. The gerrit and PR will be updated momentarily with the initial POC for this. it's an all new thing and would need new tests, new docs , etc, the whole deal. but it doesn't impact anything currently present whatsoever so has no backwards incompatibility of any kind.

"""this program types in pylance / mypy etc with no plugins"""

from typing import List

from sqlalchemy import ForeignKey
from sqlalchemy import Integer
from sqlalchemy import String
from sqlalchemy.orm import m
from sqlalchemy.orm import registry

reg = registry()

@reg.mapped
class A:
    __tablename__ = "a"

    # Mapped[int] is inferred
    id = m.column(Integer, primary_key=True)

    # Mapped[str] is inferred
    data = m.column(String(50))
    
    # relationship("B", uselist=True, collection_class=list) 
    # is inferred from annotation
    bs: m.Mapped[List["B"]] = m.related(back_populates="a")

@reg.mapped
class B:
    __tablename__ = "b"

    # Mapped[int] is inferred
    id = m.column(Integer, primary_key=True)

    # no type in column(), so send explict Python type
    # (database type is inferred from ForeignKey when 
    # Table objects are created, this is not new)
    a_id: m.Mapped[int] = m.column(ForeignKey("a.id"))

    # Mapped[str] is inferred
    data = m.column(String)

    # relationship("A", uselist=False) is inferred from annotation
    a: m.Mapped["A"] = m.related(back_populates="bs")


a1 = A()

# pylance etc. knows a1.bs is List[B]
a1.bs.append(B())

b1: B = a1.bs[0]

# pylance knows a1.id is an int
a1.id = 5

# squiggly line w pylance for this type mismatch
# not sure why mypy doesn't complain
a1.id = "Asdf"

@CaselIT
Copy link
Member

CaselIT commented Sep 17, 2021

I've left a review on gerrit

@vamshiaruru-virgodesigns

Hi all, I got here looking for pyright/pylance support for sqlalchemy but I don't really understand what's the status now with regards to pylance support. I understand that in 2.0 release the support will be there directly, but what is the current status? Is there anything I can do to get it working? Thanks :)

@zzzeek
Copy link
Member

zzzeek commented Jul 15, 2022

Hi all, I got here looking for pyright/pylance support for sqlalchemy but I don't really understand what's the status now with regards to pylance support. I understand that in 2.0 release the support will be there directly, but what is the current status? Is there anything I can do to get it working? Thanks :)

the status is unchanged that this will work terrifically in 2.0 when users migrate to new APIs and I am trying to finish documentation now.

for 1.4, there's a lot of recipes above that will get it for you. here's one that should also work

from typing import Any
from typing import Optional
from typing import Type
from typing import TYPE_CHECKING
from typing import TypeVar
from typing import Union

from sqlalchemy import Column
from sqlalchemy import ForeignKey
from sqlalchemy import Integer
from sqlalchemy import String
from sqlalchemy.orm import declarative_base
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import relationship
from sqlalchemy.types import TypeEngine

_T = TypeVar("_T")

if TYPE_CHECKING:

    class mapped_column(Mapped[_T]):
        def __init__(
            self,
            __typ: Optional[Union[TypeEngine[_T], Type[TypeEngine[_T]], Any]],
            *arg,
            **kw,
        ):
            ...


else:
    mapped_column = Column


Base = declarative_base()


class A(Base):
    __tablename__ = "a"

    id = mapped_column(Integer, primary_key=True)
    data = mapped_column(String)
    bs = relationship("B")


class B(Base):
    __tablename__ = "b"
    id = mapped_column(Integer, primary_key=True)
    a_id = mapped_column(ForeignKey("a.id"))
    data = mapped_column(String)

a1 = A()

# (variable) data: str
a1.data

# (variable) data: InstrumentedAttribute[str]
A.data

@vamshiaruru-virgodesigns
Copy link

vamshiaruru-virgodesigns commented Jul 19, 2022

Thank you very much for your help @zzzeek ! I was able to try mapped_column out and it almost works! I have previous installed sqlalchemy2-stubs and that was already helping with typing queries and mutations, but your strategy helps typing columns as well.
However, for relationships with many to many fields, I am still getting a few errors. For example, this error:

Cannot assign member "occasions" for type "Product"
  Expression of type "List[Occasion]" cannot be assigned to member "occasions" of class "Product"
    "List[Occasion]" is incompatible with "relationship"

Occurs when I have a many to many relationship between product and occasions through a secondary table, and I do product.occasions = [...] .

Another issue I face is this.

Expected class type but received "type"

This happens when I do

Base = declarative_meta()
class Product(Base):
    ....

Any help is appreciated, thanks!

@zzzeek
Copy link
Member

zzzeek commented Jul 19, 2022

sure just apply the same trick to relationship, like this works

from typing import Any
from typing import Optional
from typing import Type
from typing import TYPE_CHECKING
from typing import TypeVar
from typing import Union

from sqlalchemy import Column
from sqlalchemy import ForeignKey
from sqlalchemy import Integer
from sqlalchemy import String
from sqlalchemy.orm import declarative_base
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import relationship
from sqlalchemy.types import TypeEngine

_T = TypeVar("_T")

if TYPE_CHECKING:

    class mapped_column(Mapped[_T]):
        def __init__(
            self,
            __typ: Optional[Union[TypeEngine[_T], Type[TypeEngine[_T]], Any]],
            *arg,
            **kw,
        ):
            ...

    def orm_relationship(
        arg: Any, **kw: Any
    ) -> Mapped[Any]:
        ...


else:
    mapped_column = Column
    orm_relationship = relationship

Base = declarative_base()


class A(Base):
    __tablename__ = "a"

    id = mapped_column(Integer, primary_key=True)
    data = mapped_column(String)
    bs: Mapped[list["B"]] = orm_relationship("B")


class B(Base):
    __tablename__ = "b"
    id = mapped_column(Integer, primary_key=True)
    a_id = mapped_column(ForeignKey("a.id"))
    data = mapped_column(String)


a1 = A()

# (variable) data: str
a1.data

# (variable) data: InstrumentedAttribute[str]
A.data

# (variable) bs: InstrumentedAttribute[list[B]]
A.bs

a1 = A()

# (variable) bs: list[B]
a1.bs

@vamshiaruru-virgodesigns
Copy link

vamshiaruru-virgodesigns commented Jul 19, 2022

Thanks again, that works! I tried overloading the relationship method and that was going nowhere, but your method works. Any pointers for Expected class type but received "type"?

@zzzeek
Copy link
Member

zzzeek commented Jul 20, 2022

what is that regarding? the declarative base? I would definitely use the mypy workaround for that part of it

@vamshiaruru-virgodesigns

Oh I missed that, thanks that works!

In case it might help someone, I use __table__ attribute in my relationship() secondary arguments, and that was raising an issue. So I simply added a new attribute called __sqla_table__ to that class which just returns __table__ attribute and that took care of that issue as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants