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

Regression in 1.6 onwards: duplicated check constraints in Create Table DDL #1004

Closed
cansjt opened this issue Mar 13, 2022 · 4 comments
Closed
Labels
bug Something isn't working op directives

Comments

@cansjt
Copy link
Contributor

cansjt commented Mar 13, 2022

Describe the bug

Finally being able to upgrade from the latest 1.5.x to 1.7.x, I stumbled on the following problem: when built using SQLAlchemy expressions, check constraints may be duplicated in the DDL statement produced by alembic.

Expected behavior

To Reproduce
Please try to provide a Minimal, Complete, and Verifiable example, with the migration script and/or the SQLAlchemy tables or models involved.
See also Reporting Bugs on the website.

from alembic import op
import sqlalchemy as sa
from sqlalchemy.dialects.postgresql import UUID

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


def upgrade():
    # Columns forward declarations for those used in constraints
    column = sa.Column('column', sa.Integer, nullable=True)

    op.create_table(
        't',
        sa.Column('t_id', UUID, nullable=False),
        column,
        sa.PrimaryKeyConstraint('t_id'),
        sa.CheckConstraint(sa.and_(column > 0, column < 5),
                           name='ck_dim_job_ad_remote_type_range',
                           ),
        schema='issue',
    )


def downgrade():
    op.drop_table('t', schema='issue')

Will produce the following DLL:

CREATE TABLE issue.t (
    t_id UUID NOT NULL, 
    "column" INTEGER, 
    PRIMARY KEY (t_id), 
    CONSTRAINT ck_dim_job_ad_remote_type_range CHECK ("column" > 0 AND "column" < 5), 
    CONSTRAINT ck_dim_job_ad_remote_type_range CHECK ("column" > 0 AND "column" < 5)
);

Error

n/a

Versions.

  • OS: Ubuntu / Debian
  • Python: 3.9.10
  • Alembic: >1.6.0 (tested with 1.6.0, 1.6.5, 1.7.5)
  • SQLAlchemy: 1.4.31 (also tested with coming 1.4.33)
  • Database: postgresql
  • DBAPI: psycopg2

Additional context

Rewritting the constraint with a text() expression (e.g. sa.text('"column" > 0 AND "column" < 5') in the above example) works around the issue.

This does not seem to come from SQLAlchemy (or SQLAlchmey somehow works around this). Indeed the following code:

from sqlalchemy.dialects.postgresql import dialect
from sqlalchemy.sql.ddl import CreateTable
from sqlalchemy import __version__

METADATA = MetaData()
T = Table(
    't',
    METADATA,
    Column('t_id', UUID, nullable=False),
    # Field to have a constraint applied to
    Column('column', Integer, nullable=True),
    PrimaryKeyConstraint('t_id'),
    comment='Bla bla bla',
    schema='issue',
)
"""Dimensions collection table to pull data from datapond (debezium)

"""
T.append_constraint(CheckConstraint(and_(T.c.column < 5, T.c.column > 0)))


print(f'-- SQLAlchemy {__version__}')
stmt = CreateTable(T)
str(dialect.ddl_compiler(dialect(), stmt))

As well as using the exact same construct as in the migration (column variable, used to build the constraint) yields the expected DDL statement:

-- 1.4.33

CREATE TABLE issue.t (
    t_id UUID NOT NULL, 
    "column" INTEGER, 
    PRIMARY KEY (t_id), 
    CHECK ("column" < 5 AND "column" > 0)
)

Have a nice day!
Thanks, have a nice day as well!

@cansjt cansjt added the requires triage New issue that requires categorization label Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
@cansjt
Copy link
Contributor Author

cansjt commented Mar 13, 2022

The issue seems to be on line 213, in alembic/operations/schemaobj.py, adding the column Column does add the constraint to the table.

It is then copied and added again in the list comprehension on l. 215.

The following patch fixes the issue:

diff --git a/alembic/operations/schemaobj.py b/alembic/operations/schemaobj.py
index c8fab93..afba786 100644
--- a/alembic/operations/schemaobj.py
+++ b/alembic/operations/schemaobj.py
@@ -214,7 +214,7 @@ class SchemaObjects:
 
         constraints = [
             sqla_compat._copy(elem, target_table=t)
-            if getattr(elem, "parent", None) is not None
+            if getattr(elem, "parent", None) is not t and getattr(elem, "parent", None) is not None
             else elem
             for elem in columns
             if isinstance(elem, (Constraint, Index))

@zzzeek
Copy link
Member

zzzeek commented Mar 13, 2022

yeah looks like you have a PR coming. can you write a test for it?

@zzzeek
Copy link
Member

zzzeek commented Mar 13, 2022

I think in tests/test_op.py should wok

@zzzeek zzzeek added bug Something isn't working op directives and removed requires triage New issue that requires categorization labels Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
cansjt added a commit to cansjt/alembic that referenced this issue Mar 13, 2022
@sqla-tester
Copy link
Collaborator

Nicolas CANIART has proposed a fix for this issue in the main branch:

Fix duplicated constraints when using expressions https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3702

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working op directives
Projects
None yet
Development

No branches or pull requests

3 participants