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

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

Closed
8 tasks done
chriswhite199 opened this issue Nov 23, 2021 · 11 comments
Closed
8 tasks done
Labels

Comments

@chriswhite199
Copy link
Contributor

First Check

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

Commit to Help

  • 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 chriswhite199 added the question Further information is requested label Nov 23, 2021
@chriswhite199
Copy link
Contributor Author

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 added a commit to chriswhite199/sqlmodel that referenced this issue Nov 24, 2021
@chriswhite199
Copy link
Contributor Author

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?

@olafdeleeuw
Copy link

@tiangolo
Copy link
Owner

Thanks everyone for the discussion. ☕

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 #165. The fix will be available in the next release, SQLModel 0.0.7, in the next hours. 🎉

Could you check and confirm?

@tiangolo tiangolo added answered and removed question Further information is requested labels Aug 27, 2022
@github-actions
Copy link

github-actions bot commented Sep 7, 2022

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 github-actions bot closed this as completed Sep 7, 2022
@chriswhite199
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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_)

    ...

@tepelbaum
Copy link

tepelbaum commented Nov 29, 2022

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 #442 is merged? :)

@tiangolo
Copy link
Owner

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

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

@tepelbaum
Copy link

tepelbaum commented Oct 23, 2023

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants