Server_default including quote not detected properly

[GeorgeSchneelochVA]

Describe the bug

A column with the server_default (substring(user_name(),charindex('\',user_name())+(1),len(user_name()))) does not compare properly, causing the migration autogenerate command to think that the server default in the model does not match what is on the database.

Expected behavior
A flask db migrate command should not create any new migration if the server_default for a column in the model is the string described above, and that column on the database has the same string for its default constraint.

To Reproduce
Add a table to SQL Server:

CREATE TABLE test1 (id integer primary key, usernamecol varchar(50))

ALTER TABLE test1 ADD  CONSTRAINT [DF_constraint]  DEFAULT (substring(user_name(),charindex('\',user_name())+(1),len(user_name()))) FOR usernamecol

Define the model in SQLAlchemy:

class test1(db.Model):
    __tablename__ = 'test1'
    id = db.Column(db.Integer, primary_key=True)
    usernamecol = db.Column(
        db.VARCHAR(50),
        server_default=sa.text(
            "(substring(user_name(),charindex('\\',user_name())+(1),len(user_name())))"
        )
    )

Run flask db migrate with compare_server_default=True set in your Migrate(...). There will be a migration generated updating the server_default for that column with exactly the same text as was defined in the server_default in the model. The migration should not have been generated since the server default already matches.

Error

# Copy error here. Please include the full stack trace.

Versions.

• OS: Windows 10
• Python: 3.8.8
• Alembic: 1.9.3
• SQLAlchemy: 2.0.3
• Database: SQL Server 2019
• DBAPI:

Additional context
I did some investigation and it looks like the value is handled correctly until this line, where single quotes are added to both sides of the string. I'm not sure the intent of the code there, but this server default should be treated as a SQL expression rather than a string.

Have a nice day!

[zzzeek]

this happens all in the SQL server dialect at https://github.com/sqlalchemy/alembic/blob/main/alembic/ddl/mssql.py#L227 . the quoting added in the main compare.py is not the issue.

for this default, incoming is:

"(substring(user_name(),charindex('',user_name())+(1),len(user_name())))"

which we trim down to

"substring(user_name(),charindex('',user_name())+(1),len(user_name()))"

metadata default is this:

"substring(user_name(), charindex('',user_name())+(1),len(user_name()))"

it's the space character, that's about it

but if you had a default like this:

"substring('name',1,3)"

SQL Server turns it into

"substring('name', (1), (3))"

which is nuts. so im going to simplify this and just remove all spaces, parenthesis, double and single quotes from the whole expression, and compare them both on what's left.

[zzzeek]

that "substring" example brings out issues with other backends, so i will remove the "add quotes" thing from compare also since that is not helping anyone, the dialects can do this part

[zzzeek]

let's not do that since that string is passed to the user defined function

[sqla-tester]

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

collapse all chars for mssql defaults, move quoting https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4448

[CaselIT]

let's not do that since that string is passed to the user defined function

how about we fix this for 1.10 so we can avoid breaking other things in between? (like the _compare_server_default that 3rd party implementation may have added)?

I don't think it's a critical bug that cannot wait for 1.10

[zzzeek]

@GeorgeSchneelochVA are you blocked by this?

[GeorgeSchneelochVA]

@zzzeek Yes, there are several columns with that server default expression in our system so we will need to exclude those tables from migrations for the time being

[zzzeek]

I will alter the approach im taking to not have any public behavioral changes