sqlalchemy / sqlalchemy

The Database Toolkit for Python
https://www.sqlalchemy.org
MIT License
9.05k stars 1.36k forks source link

Enums are not created in new type alias syntax #11305

Closed Alc-Alc closed 1 month ago

Alc-Alc commented 1 month ago

Describe the bug

Using the new type alias syntax with a literal causes errors during the model creation. I would expect both the syntaxes to work OOTB as both are type aliases (check Additional Info for screenshot)

Tried adding type_annotation_map (code as seen in Additional Info) same error

Optional link from https://docs.sqlalchemy.org which documents the behavior that is expected

https://docs.sqlalchemy.org/en/20/orm/declarative_tables.html#using-python-enum-or-pep-586-literal-types-in-the-type-map

SQLAlchemy Version in Use

2.0.29

DBAPI (i.e. the database driver)

NA - Happens at model creation

Database Vendor and Major Version

NA - Happens at model creation

Python Version

3.12

Operating system

OSX

To Reproduce

from typing import Literal, reveal_type
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column

type Options484 = Literal["one", "two"]
Options = Literal["one", "two"]

reveal_type(Options)  # Runtime type is '_LiteralGenericAlias'
reveal_type(Options484)  # Runtime type is 'TypeAliasType'

class Base(DeclarativeBase): ...

class MyTable(Base):
    __tablename__ = "my_table"
    id: Mapped[int] = mapped_column(primary_key=True)
    option_1: Mapped[Options]  # works
    option_2: Mapped[Options484]  # does not work

Error

sqlalchemy.exc.ArgumentError: Could not locate SQLAlchemy Core type for Python type typing.Literal['one', 'two'] inside the 'option_2' attribute Mapped annotation

Additional context

Hovering over the aliases in reveal_type shows they are both picked up by Pylance (Pyright) as type aliases.

For Options

image

For Options484

image

Tried adding type_annotation_map for TypeAliasType, same error as shown in "Error".

from typing import Literal, TypeAliasType
from enum import Enum
from sqlalchemy import Enum as SAEnum
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column

type Options484 = Literal["one", "two"]
Options = Literal["one", "two"]

class Base(DeclarativeBase): ...

class MyTable(Base):
    __tablename__ = "my_table"
    id: Mapped[int] = mapped_column(primary_key=True)
    option_1: Mapped[Options]  # works
    option_2: Mapped[Options484]  # does not work
    type_annotation_map = {
        TypeAliasType: SAEnum(Enum),
    }

PS: 484 should actually be 695, a reference to https://peps.python.org/pep-0695/ which describes the syntax. I got the PEPs confused. 🙃

CaselIT commented 1 month ago

Hi,

I fairly sure this has already been reported but I cannot find it. Sadly python decided that for the typing feature it was the best thing to have a thousand different ways of doing anything, so it's quite difficult to support all the various incantations.

thanks for reporting

CaselIT commented 1 month ago

Also TypeAliasType is not a valid type here, since it's likely that everything defined with type ... becomes one. Indeed:

>>> type x = int
>>> type(x)
<class 'typing.TypeAliasType'>
>>> type y = int | str | float
>>> type(y)
<class 'typing.TypeAliasType'>
CaselIT commented 1 month ago

It seems that one could use __value__ to access what the type alias points to

Alc-Alc commented 1 month ago

I fairly sure this has already been reported but I cannot find it.

I came across https://github.com/sqlalchemy/sqlalchemy/issues/10807 which felt similar but the issue was closed and it seems it was released in 2.0.25

Added preliminary support for Python 3.12 pep-695 type alias structures, when resolving custom type maps for ORM Annotated Declarative mappings.

but I was still running into this so I opened a new one.

CaselIT commented 1 month ago

Ok, Thanks for finding that. Then I guess this is an unsupported case

Alc-Alc commented 1 month ago

@CaselIT I have made an attempt here at fixing this, if you feel this is a good starting point I am happy to raise a PR and apply any relevant suggestions. I added one new test (modified based on an existing test) and it passes.

https://github.com/sqlalchemy/sqlalchemy/commit/8fd36a4815db947489652487bbfb5c030973cfe2 shows my attempted changes

CaselIT commented 1 month ago

That seems to work but I wonder if it could be easier to just do at the start of _resolve_type

if is_pep695(python_type):
  return _resolve_type(python_type.__value__)

This would handle anything is declared with the new type syntax. It would also allow for alias of alias (not sure if value in this case is also a typealiastype or if it's resolved)

Feel free to open a PR. It's a good start (if you do can you also add a change log?)

sqla-tester commented 1 month ago

Alc-Alc has proposed a fix for this issue in the main branch:

Enums are not created with pep695 literal https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5260

zzzeek commented 1 month ago

was there another issue or discussion where we went over this previously?

sqla-tester commented 1 month ago

Alc-Alc has proposed a fix for this issue in the rel_2_0 branch:

improve pep-695 inference including Enum support https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/5278

zzzeek commented 1 month ago

OK mostly this just builds on 8772041cc6 / #10807 which is very recent . great thanks for the PR

Alc-Alc commented 1 month ago

was there another issue or discussion where we went over this previously?

Yes, I noticed you figured it in the next comment. Just for completion sake, https://github.com/sqlalchemy/sqlalchemy/issues/11305#issuecomment-2074638338 states my reasoning behind opening this issue.