Discrepancies in column types for social-auth-core derived database tables

[nsoranzo]

Describe the bug
social-auth-core 4.5.5 has added initial type annotation, which is breaking our test_galaxy_packages tests (since these tests don't pin a version of the library).
The failing mypy tests are all about column types for database table inheriting from social-auth-core *Mixin classes. Some failures seem legitimate errors in our column type definitions, others may be discrepancies we can ignore with a type: ignore, and some may be errors in type upstream type annotation:

galaxy/model/__init__.py:10152: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "AssociationMixin" defined
the type as "str")  [assignment]
        server_url: Mapped[Optional[str]] = mapped_column(VARCHAR(255))
                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10153: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "AssociationMixin" defined
the type as "str")  [assignment]
        handle: Mapped[Optional[str]] = mapped_column(VARCHAR(255))
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10154: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "AssociationMixin" defined
the type as "str")  [assignment]
        secret: Mapped[Optional[str]] = mapped_column(VARCHAR(255))
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10155: error: Incompatible types in assignment
(expression has type "Mapped[int | None]", base class "AssociationMixin" defined
the type as "int")  [assignment]
        issued: Mapped[Optional[int]]
        ^
galaxy/model/__init__.py:10156: error: Incompatible types in assignment
(expression has type "Mapped[int | None]", base class "AssociationMixin" defined
the type as "int")  [assignment]
        lifetime: Mapped[Optional[int]]
        ^
galaxy/model/__init__.py:10157: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "AssociationMixin" defined
the type as "str")  [assignment]
        assoc_type: Mapped[Optional[str]] = mapped_column(VARCHAR(64))
                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10216: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "CodeMixin" defined the
type as "str")  [assignment]
        email: Mapped[Optional[str]] = mapped_column(VARCHAR(200))
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10217: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "CodeMixin" defined the
type as "str")  [assignment]
        code: Mapped[Optional[str]] = mapped_column(VARCHAR(32))
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10244: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "NonceMixin" defined the
type as "str")  [assignment]
        server_url: Mapped[Optional[str]] = mapped_column(VARCHAR(255))
                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10245: error: Incompatible types in assignment
(expression has type "Mapped[int | None]", base class "NonceMixin" defined the
type as "int")  [assignment]
        timestamp: Mapped[Optional[int]]
        ^
galaxy/model/__init__.py:10246: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "NonceMixin" defined the
type as "str")  [assignment]
        salt: Mapped[Optional[str]] = mapped_column(VARCHAR(40))
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10282: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "PartialMixin" defined the
type as "str")  [assignment]
        token: Mapped[Optional[str]] = mapped_column(VARCHAR(32))
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10283: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "PartialMixin" defined the
type as "dict[Any, Any]")  [assignment]
        data: Mapped[Optional[str]] = mapped_column(TEXT)
                                      ^~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10284: error: Incompatible types in assignment
(expression has type "Mapped[int | None]", base class "PartialMixin" defined the
type as "str")  [assignment]
        next_step: Mapped[Optional[int]]
        ^
galaxy/model/__init__.py:10285: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "PartialMixin" defined the
type as "str")  [assignment]
        backend: Mapped[Optional[str]] = mapped_column(VARCHAR(32))
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10327: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "UserMixin" defined the
type as "None")  [assignment]
        uid: Mapped[Optional[str]] = mapped_column(VARCHAR(255))
                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10328: error: Incompatible types in assignment
(expression has type "Mapped[str | None]", base class "UserMixin" defined the
type as "str")  [assignment]
        provider: Mapped[Optional[str]] = mapped_column(VARCHAR(32))
                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
galaxy/model/__init__.py:10332: error: Incompatible types in assignment
(expression has type "Mapped[User | None]", base class "UserMixin" defined the
type as "str")  [assignment]
        user: Mapped[Optional["User"]] = relationship(back_populates="soci...
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~...

For reference, you can look at the reference definition of these tables from the social-auth-storage-sqlalchemy package: https://github.com/python-social-auth/social-storage-sqlalchemy/blob/master/social_sqlalchemy/storage.py

Initial assessment:

• Class PSAAssociation(AssociationMixin):
  • columns server_url, handle, secret, issued, lifetime, assoc_type: we could drop Optional
• Class PSACode(CodeMixin):
  • columns email, code: we could drop Optional
• Class PSANonce(NonceMixin):
  • columns server_url, timestamp, salt: we could drop Optional
• Class PSAPartial(PartialMixin):
  • columns: token, backend: we could drop Optional
  • column data: I think this should be changed from Mapped[Optional[str]] = mapped_column(TEXT) to Mapped[[Dict[str, Any]] = mapped_column(MutableJSONType)
  • column next_step: we could drop Optional. Need to fix upstream type from str to int
• Class UserAuthnzToken(UserMixin):
  • columns uid, provider: we could drop Optional
  • column user: drop the user attribute of the UserMixin class upstream

@jdavcs @dannon Thoughts?

[jdavcs]

social-auth-core 4.5.5 has added initial type annotation, which is breaking our test_galaxy_packages tests (since these tests don't pin a version of the library).

@nsoranzo thank you for pointing out the root cause!

Removing Optional is not so each: that type determines the generated SQL for the column definition: without Optional, you have NOT NULL added to the DDL.

I think the least invasive approach is to simply add type-ignores. Another approach would be to change this:
foo = Mapped[Optional[str]] = mapped_column(...)
to this:
foo = Mapped[str] = mapped_column(..., nullable=True). The nullable argument overrides the type; so in the Python application the type is expected to be str, whereas the database accepts nulls; so the DDL would be unchanged. However, I really dislike this approach because the code becomes very confusing.

Those columns should not be nullable, of course. However, there are numerous cases like those in the galaxy model, and all of them would require migrations. Which is why I suggest using type-ignore.

[mvdbeek]

However, I really dislike this approach because the code becomes very confusing.

The type annotation says how we use things in python, not how the database stores things and that it's nullable isn't of interest. deriving the column configuration from type annotations is certainly neat when it does match, but there's a reason this can be overridden. A prime example of how this makes no sense is

galaxy/lib/galaxy/model/__init__.py

Line 1464 in 2bf7c7e

dependencies: Mapped[Optional[bytes]] = mapped_column(MutableJSONType)

... while that's the database column type it is not at all correct for the python side.
I think foo = Mapped[str] = mapped_column(..., nullable=True) is the right approach, at least for the optionality case.

[jdavcs]

Apparently, it's not solvable with `nullable=True'

galaxy/model/__init__.py:10087: error: Incompatible types in assignment (expression has type "Mapped[str]", base class "AssociationMixin" defined the type as "str")  [assignment]
        server_url: Mapped[str] = mapped_column(VARCHAR(255), nullable=True)

I'll look some more, but it may be an upstream problem. In which case I suggest going with the type-ignore, at least for now.

[jdavcs]

although, apparently type-ignore won't solve that either.

lib/galaxy/model/__init__.py:10088: error: Unused "type: ignore" comment  [unused-ignore]
        handle: Mapped[Optional[str]] = mapped_column(VARCHAR(255))  # type:ignore[assignment]

[nsoranzo]

@jdavcs Thanks for looking at this, the type: ignore are indeed needed any way.
For PSAPartial.data though it seems we have an actual wrong type on our end, is there any value in that column on Main's database we could use to check?

[jdavcs]

...For PSAPartial.data though it seems we have an actual wrong type on our end, is there any value in that column on Main's database we could use to check?

Those columns on both main and eu are empty. So a migration (TEXT to BLOB) would be painless.
data: Mapped[Optional[Dict[str, Any]]] = mapped_column(JSONType, nullable=True)?

[nsoranzo]

...For PSAPartial.data though it seems we have an actual wrong type on our end, is there any value in that column on Main's database we could use to check?

Those columns on both main and eu are empty. So a migration (TEXT to BLOB) would be painless. data: Mapped[Optional[Dict[str, Any]]] = mapped_column(JSONType, nullable=True)?

According to https://github.com/python-social-auth/social-core/blob/806de35d3756153cfadf64efbdd29c1ad6f425a6/social_core/storage.py#L295 , it should default to an empty dict, but this would already be a step in the right direction.