sqlmodel icon indicating copy to clipboard operation
sqlmodel copied to clipboard

Enum types on inherited models don't correctly create type in Postgres

Open chriswhite199 opened this issue 4 years ago • 3 comments

First Check

  • [X] I added a very descriptive title to this issue.
  • [X] I used the GitHub search to find a similar issue and didn't find it.
  • [X] I searched the SQLModel documentation, with the integrated search.
  • [X] I already searched in Google "How to X in SQLModel" and didn't find any information.
  • [X] I already read and followed all the tutorial in the docs and didn't find an answer.
  • [X] I already checked if it is not related to SQLModel but to Pydantic.
  • [X] I already checked if it is not related to SQLModel but to SQLAlchemy.

Commit to Help

  • [X] I commit to help with one of those options 👆

Example Code

import enum
import uuid

from sqlalchemy import Enum, Column, create_mock_engine
from sqlalchemy.sql.type_api import TypeEngine
from sqlmodel import SQLModel, Field


class MyEnum(enum.Enum):
    A = 'A'
    B = 'B'


class MyEnum2(enum.Enum):
    C = 'C'
    D = 'D'


class BaseModel(SQLModel):
    id: uuid.UUID = Field(primary_key=True)
    enum_field: MyEnum2 = Field(sa_column=Column(Enum(MyEnum2)))


class FlatModel(SQLModel, table=True):
    id: uuid.UUID = Field(primary_key=True)
    enum_field: MyEnum = Field(sa_column=Column(Enum(MyEnum)))


class InheritModel(BaseModel, table=True):
    pass


def dump(sql: TypeEngine, *args, **kwargs):
    dialect = sql.compile(dialect=engine.dialect)
    sql_str = str(dialect).rstrip()
    if sql_str:
        print(sql_str + ';')


engine = create_mock_engine('postgresql://', dump)

SQLModel.metadata.create_all(bind=engine, checkfirst=False)

Description

When executing the above example code, the output shows that only the enum from FlatModel is correctly created, while the enum from the inherited class is not:

CREATE TYPE myenum AS ENUM ('A', 'B');
# There should be a TYPE def for myenum2, but isn't

CREATE TABLE flatmodel (
	enum_field myenum, 
	id UUID NOT NULL, 
	PRIMARY KEY (id)
);
CREATE INDEX ix_flatmodel_id ON flatmodel (id);

CREATE TABLE inheritmodel (
	enum_field myenum2, 
	id UUID NOT NULL, 
	PRIMARY KEY (id)
);
CREATE INDEX ix_inheritmodel_id ON inheritmodel (id);

Operating System

macOS

Operating System Details

No response

SQLModel Version

0.0.4

Python Version

3.9.7

Additional Context

No response

chriswhite199 avatar Nov 23 '21 21:11 chriswhite199

The equiv straight sqlalchemy example code works without issue:

import enum
import uuid

from sqlalchemy import Enum, Column, create_mock_engine
from sqlalchemy.orm import declarative_base, declarative_mixin
from sqlalchemy.sql.type_api import TypeEngine
from sqlmodel.sql.sqltypes import GUID


class MyEnum(enum.Enum):
    A = 'A'
    B = 'B'


class MyEnum2(enum.Enum):
    C = 'C'
    D = 'D'


Base = declarative_base()


@declarative_mixin
class BaseModel:
    id: uuid.UUID = Column(GUID(), primary_key=True)
    enum_field: MyEnum2 = Column(Enum(MyEnum2))


class FlatModel(Base):
    id: uuid.UUID = Column(GUID(), primary_key=True)
    enum_field: MyEnum = Column(Enum(MyEnum))

    __tablename__ = 'flat'


class InheritModel(BaseModel, Base):
    __tablename__ = 'inherit'


def dump(sql: TypeEngine, *args, **kwargs):
    dialect = sql.compile(dialect=engine.dialect)
    sql_str = str(dialect).rstrip()
    if sql_str:
        print(sql_str + ';')


engine = create_mock_engine('postgresql://', dump)

Base.metadata.create_all(bind=engine, checkfirst=False)

Output

CREATE TYPE myenum AS ENUM ('A', 'B');
CREATE TYPE myenum2 AS ENUM ('C', 'D');

CREATE TABLE flat (
	id UUID NOT NULL, 
	enum_field myenum, 
	PRIMARY KEY (id)
);

CREATE TABLE inherit (
	id UUID NOT NULL, 
	enum_field myenum2, 
	PRIMARY KEY (id)
);

chriswhite199 avatar Nov 24 '21 00:11 chriswhite199

Well the partial-fix seems to be addressed in a number of PRs (#24 , #165 - which i created not knowing #24 was already out there). There is still an issue with inheritance if you manually use sa_column in the non-table parent class but I didn't get anywhere understanding why this was the case.

Are there more maintainers other than @tiangolo who can look over these PRs and get one of them merged?

chriswhite199 avatar Nov 24 '21 17:11 chriswhite199

I'm having the same issue but posted it in the closed issue #164:

  • https://github.com/tiangolo/sqlmodel/issues/131#issuecomment-1162749873
  • https://github.com/tiangolo/sqlmodel/issues/131#issuecomment-1164005895

olafdeleeuw avatar Jun 24 '22 07:06 olafdeleeuw

Thanks everyone for the discussion. :coffee:

If you want to help me, the best help I can get is helping others with their questions in issues. That's what consumes most of the time.

Maybe this was solved in https://github.com/tiangolo/sqlmodel/pull/165. The fix will be available in the next release, SQLModel 0.0.7, in the next hours. :tada:

Could you check and confirm?

tiangolo avatar Aug 27 '22 22:08 tiangolo

Assuming the original need was handled, this will be automatically closed now. But feel free to add more comments or create new issues or PRs.

github-actions[bot] avatar Sep 07 '22 00:09 github-actions[bot]

I can confirm that if you do not use sa_column for the field, this is now working as expected with sqlmodel:0.0.8 and sqlalchemy:1.4.41.

However, a word to other who might end up here - it does NOT work if you manually define the field via sa_column:

class BaseModel(SQLModel):
    id: uuid.UUID = Field(primary_key=True)

    # this will not work
    #enum_field: MyEnum2 = Field(sa_column=Column(Enum(MyEnum2)))

    # this will work
    #enum_field: MyEnum2 = Field()

chriswhite199 avatar Sep 07 '22 21:09 chriswhite199

An addendum - defining the enum with a string mixin (for nicer on-the-wire strings used with fastapi) using the following notation also breaks the type creation DDL (in that It will not create the dedicated type and the field will just be a varchar):

class MyEnum(str, enum.Enum):
    A = 'A'
    B = 'B'

chriswhite199 avatar Sep 07 '22 21:09 chriswhite199

I assume this could easily be fixed by amending the if statement order in main.py:get_sqlalchemy_type function to test the subclass is an Enum before string:

def get_sqlachemy_type(field: ModelField) -> Any:
    if issubclass(field.type_, str):
        if field.field_info.max_length:
            return AutoString(length=field.field_info.max_length)
        return AutoString

    ...

    # move this to the top?
    if issubclass(field.type_, Enum):
        return sa_Enum(field.type_)

    ...

chriswhite199 avatar Sep 07 '22 21:09 chriswhite199

An addendum - defining the enum with a string mixin (for nicer on-the-wire strings used with fastapi) using the following notation also breaks the type creation DDL (in that It will not create the dedicated type and the field will just be a varchar):

class MyEnum(str, enum.Enum):
    A = 'A'
    B = 'B'

Any news on this point? I am having this exact issue with enums defined with string mixin (sqlmodel 0.0.6 and 0.0.8 tested without success).

Will we be good to go when https://github.com/tiangolo/sqlmodel/pull/442 is merged? :)

tepelbaum avatar Nov 29 '22 16:11 tepelbaum

Thanks @chriswhite199 and @tepelbaum! This was solved in https://github.com/tiangolo/sqlmodel/pull/669

It will be available in the next release, 0.0.9. 🎉

tiangolo avatar Oct 23 '23 09:10 tiangolo

Thanks @chriswhite199 and @tepelbaum! This was solved in #669

It will be available in the next release, 0.0.9. 🎉

Thanks @tiangolo , and thanks a lot for this!! Can't wait for the pydantic V2 support :)

tepelbaum avatar Oct 23 '23 12:10 tepelbaum