sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.61k stars 234 forks source link

Upgrading to 1.12 changes type of `Mapped[str]` in new migrations #1471

Closed knkski closed 2 months ago

knkski commented 2 months ago

Describe the bug

I upgraded Alembic from 1.11.x to 1.13.x. Everything mostly works, except now I'm getting unnecessary conversions of my Mapped[str] columns (in PostgreSQL). Alembic wants to convert them from sa.TEXT to sa.String. This is likely due to the defaulting of compare_type to True with 1.12.0.

Expected behavior

I expected Alembic to see that the columns are text, and are defined in code as Mapped[str], and realize that everything's good. I believe that the Mapped[str] columns got created as text because they were originally defined as Mapped[str] = mapped_column(TEXT), then the = mapped_column(TEXT) bit got removed as unnecessary, which turns out to be only true when Alembic isn't comparing types.

To Reproduce

# Example SQLAlchemy table definition
class Example(Base):
    __tablename__ = "example"
    text: Mapped[str]

Error

# Generated code in the new migration post 1.12.0
    op.alter_column('example', 'text',
               existing_type=sa.TEXT(),
               type_=sa.String(),
               existing_nullable=False)

Versions.

Additional context

This might not be a bug, and just working as intended. Looking at PostgreSQL's documentation, it says:

In addition, PostgreSQL provides the text type, which stores strings of any length. Although the text type is not in the SQL standard, several other SQL database management systems have it as well. text is PostgreSQL's native string data type, in that most built-in functions operating on strings are declared to take or return text not character varying. For many purposes, character varying acts as though it were a domain over text.

It makes sense to me for Alembic to default to text for Mapped[str] when using PostgreSQL, or at least not complain when seeing a text column that corresponds to Mapped[str] in code. However, since text isn't actually in the SQL standard, that might be too specialized and not something that Alembic wants to support. It would just be nice if I didn't have to add = mapped_column(TEXT) everywhere.

Have a nice day!

zzzeek commented 2 months ago

hi -

this doesnt look like a bug. If you have:

# Example SQLAlchemy table definition
class Example(Base):
    __tablename__ = "example"
    text: Mapped[str]

that's the same as:

# Example SQLAlchemy table definition
class Example(Base):
    __tablename__ = "example"
    text = Column(String())

String is VARCHAR. If your PG database has TEXT in it, that suggests that this model was not used to generate the model.

What I would recommend here is, well it depends what you want:

  1. set up Mapped[str] as TEXT at least for PostgreSQL:

class Base(DeclarativeBase):
    type_annotation_map = {
        str: String().with_variant(TEXT, "postgresql"),
    }
  1. turn off the compare_type boolean
  2. use a custom compare type function to correct as needed: https://alembic.sqlalchemy.org/en/latest/autogenerate.html#compare-types