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

compare_server_default doesn't work with a string column that contains a backslash in its default value #1145

Closed
marcinbarczynski opened this issue Dec 22, 2022 · 3 comments

Comments

@marcinbarczynski
Copy link

marcinbarczynski commented Dec 22, 2022

Describe the bug
If server_default of a string column contains a backslash, alembic generates op.alter_column migration despite the fact that the database and model are consistent.

Expected behavior
If the database and model are consistent, alembic revision --autogenerate should create an empty migration.

To Reproduce

  1. Create mymodel.py:
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import String, Column, Integer

Base = declarative_base()

class Demo(Base):
    __tablename__ = 'demo'
    key = Column(Integer, primary_key=True)
    regex = Column(String, nullable=False, default='abc\\..*', server_default='abc\\..*')
  1. Run alembic init alembic
  2. Set compare_server_default to True in env.py
  3. Set up mymodel in env.py:
from mymodel import Base
target_metadata = Base.metadata
  1. Run alembic revision --autogenerate. It created the following migration:
"""empty message

Revision ID: f5adea3b05e6
Revises: 
Create Date: 2022-12-22 16:15:14.111949

"""
from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision = 'f5adea3b05e6'
down_revision = None
branch_labels = None
depends_on = None


def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('demo',
    sa.Column('key', sa.Integer(), nullable=False),
    sa.Column('regex', sa.String(), server_default='abc\\..*', nullable=False),
    sa.PrimaryKeyConstraint('key')
    )
    # ### end Alembic commands ###


def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.drop_table('demo')
    # ### end Alembic commands ###
  1. Upgrade database with alembic upgrade head
  2. Run alembic revision --autogenerate again.

Error

The last alembic revision --autogenerate creates a non-empty migration file:
Due to a bug it generates a non-empty migration:

"""empty message

Revision ID: 198e4eb04d5c
Revises: f5adea3b05e6
Create Date: 2022-12-22 16:15:26.936522

"""
from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision = '198e4eb04d5c'
down_revision = 'f5adea3b05e6'
branch_labels = None
depends_on = None


def upgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.alter_column('demo', 'regex',
               existing_type=sa.VARCHAR(),
               server_default='abc\\..*',
               existing_nullable=False)
    # ### end Alembic commands ###


def downgrade() -> None:
    # ### commands auto generated by Alembic - please adjust! ###
    op.alter_column('demo', 'regex',
               existing_type=sa.VARCHAR(),
               server_default=sa.text("'abc\\..*'::character varying"),
               existing_nullable=False)
    # ### end Alembic commands ###

The problem happens only if there is \ in the default string.

Versions.

  • OS: Ubuntu 22.04
  • Python: 3.10
  • Alembic: 1.8.1
  • SQLAlchemy: 1.4.42
  • Database: PostgreSQL 13.9
  • DBAPI: psycopg2==2.9.3

Additional context

@marcinbarczynski marcinbarczynski added the requires triage New issue that requires categorization label Dec 22, 2022
@zzzeek
Copy link
Member

zzzeek commented Dec 22, 2022

the patch is approximately:

diff --git a/alembic/autogenerate/compare.py b/alembic/autogenerate/compare.py
index c9971ea..8301e34 100644
--- a/alembic/autogenerate/compare.py
+++ b/alembic/autogenerate/compare.py
@@ -1022,7 +1022,7 @@ def _render_server_default_for_compare(
     if isinstance(metadata_default, str):
         if metadata_col.type._type_affinity is sqltypes.String:
             metadata_default = re.sub(r"^'|'$", "", metadata_default)
-            return repr(metadata_default)
+            return f"'{metadata_default}'"
         else:
             return metadata_default
     else:

however there seem to be no case-oriented tests for many server default cases in test_autogen_diffs, sort of surprising as there was a lot of work done on this at some point , it looks like tests were added to dialect-specific like test_postgresql for Identity, things like that.

string tests should include different patterns of backslashes as well as single quotes that require SQL-escaping, with a round trip to create the DDL in the DB, get the value back, compare it to rendered from the metadata.

@zzzeek zzzeek added bug Something isn't working autogenerate - defaults autogenerate - detection and removed requires triage New issue that requires categorization labels Dec 22, 2022
@sqla-tester
Copy link
Collaborator

Mike Bayer has proposed a fix for this issue in the main branch:

dont use repr to quote string in compare_server_default https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4324

@marcinbarczynski
Copy link
Author

Thanks! That was quick!

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

No branches or pull requests

3 participants