MSSQL needs "NOT NULL" sent explicitly for a change in type to maintain not null

[AbdealiLoKo]

Describe the bug
Looks like in alembic when we do an alter_column() to change the type of a column type_= it is ignoring the existing_nullable=True and removing the nullable.

Expected behavior
I expected the type of my column to change the type - without any other change to the column

To Reproduce
I have a migration like the following in my alembic versions:

"""user_notification: Fix type of title

Revision ID: eb57aaff3a51
Revises: 5a41b8b16e55
Create Date: 2020-03-25 21:00:57.615363

"""
import sqlalchemy as sa
from alembic import op

# revision identifiers, used by Alembic.
revision = 'eb57aaff3a51'
down_revision = '5a41b8b16e55'
branch_labels = None
depends_on = None

def upgrade():
    with op.batch_alter_table('user_notification') as batch_op:
        batch_op.alter_column(
            'title',
            existing_type=sa.String(length=100),
            type_=sa.String(length=511),
            existing_nullable=False,
        )

def downgrade():
    with op.batch_alter_table('user_notification') as batch_op:
        batch_op.alter_column(
            'title',
            existing_type=sa.String(length=511),
            type_=sa.String(length=100),
            existing_nullable=False,
        )

• When I do db upgrade 5a41b8b16e55 - The DDL shows: title varchar(100) COLLATE SQL_Latin1_General_CP1_CI_AS NOT NULL,
• When I do db upgrade eb57aaff3a51 - The DDL shows: title varchar(511) COLLATE SQL_Latin1_General_CP1_CI_AS NULL,
  The "NOT NULL" has changed to "NULL" - even though I provided existing_nullable=False

NOTE: I have other cases in my migrations where I only change the name of the column and not the type - I don't notice this behavior at that time. It seems to be only when I make a change to the type.

Versions.
​- OS: CentOS 7
​- Python: 3.6.5
​- Alembic: 1.7.5
​- SQLAlchemy: 1.4.29
​- Database: MSSQL 2019
​- DBAPI: pyodbc

Additional context
Feels like it may be related to this: #812
But I am using the latest version of alembic as of now

[zzzeek]

is it running a batch "CREATE TABLE" statement ? or you have batch mode left as "auto" so these are ALTER TABLE statements?

[zzzeek]

so far this looks like the identical thing that #812 has fixed. We dont render the word "NULL" in the DDL. I'm testing regular op.alter_column(). Not clear if you are forcing batch CREATE TABLE or not, if so I'd advise not using that it isn't well tested outside of SQLite.

can't reproduce given behavior at the moment

    @combinations((True,), (False,), argnames="change_nullability")
    def test_alter_column_type_and_nullability_two(self, change_nullability):
        context = op_fixture("mssql")

        args = dict(
            existing_type=String(100),
            type_=String(511),
            existing_nullable=False,
        )
        if change_nullability:
            args["nullable"] = False

        op.alter_column("t", "c", **args)

        if change_nullability:
            context.assert_(
                "ALTER TABLE t ALTER COLUMN c VARCHAR(511) NOT NULL"
            )
        else:
            context.assert_("ALTER TABLE t ALTER COLUMN c VARCHAR(511)")

no "NULL" keyword is generated. I also see collation things in your output so please share complete details, thanks.

[AbdealiLoKo]

I do not set recreate= in batch_alter_table('user_notification') in my migrations - is there a global config I should be checking - I haven't purposefully changed this value anywhere per se.
The above migration file is the exact migration where I can reproduce this problem.

Also, a quick note that the DDL I shows on top was the DDL that dbeaver was showing me when I connect to my mssql and check the DB (not the DDL generated by sqlalchemy)

NOTE: I need to support multiple databases: sqlite, mysql, mssql, oracle, postgres
So, I have render_as_batch=True in my env.py - https://alembic.sqlalchemy.org/en/latest/batch.html#batch-mode-with-autogenerate. But I guess that only affects the auto generation.

[AbdealiLoKo]

I've created a more reproducible example.
alembic-test.zip

Steps followed:

Get a basic alembic setup

$ mkdir alembic-test
$ cd alembic-test

$ python -m venv venv

$ venv/bin/pip -V
pip 19.0.3 from /home/abdealijk/alembic-test/venv/lib/python3.7/site-packages/pip (python 3.7)

$ venv/bin/pip install alembic
...
Successfully installed Mako-1.1.6 MarkupSafe-2.0.1 SQLAlchemy-1.4.29 alembic-1.7.5 greenlet-1.1.2 importlib-metadata-4.10.0 importlib-resources-5.4.0 typing-extensions-4.0.1 zipp-3.7.0

$ venv/bin/pip install sqlalchemy[mssql]
...
Successfully installed pyodbc-4.0.32

$ venv/bin/alembic init alembic

# In alembic.ini - Change sqlalchemy.url
$ grep "sqlalchemy.url" alembic.ini
sqlalchemy.url = mssql+pyodbc://root:password@172.29.0.1:1433/db_empty?driver=ODBC+Driver+17+for+SQL+Server

Create first migration - add user_notification table

$ venv/bin/alembic revision -m "initial"
  Generating /home/abdealijk/alembic-test/alembic/versions/3905e21d3e98_initial.py ...  done

# Add upgrade/downgrade for the first migration manually
$ cat alembic/versions/3905e21d3e98_initial.py
"""initial
Revision ID: 3905e21d3e98
Revises:
Create Date: 2022-01-12 21:21:44.975323
"""
from alembic import op
import sqlalchemy as sa
# revision identifiers, used by Alembic.
revision = '3905e21d3e98'
down_revision = None
branch_labels = None
depends_on = None
def upgrade():
    op.create_table(
        'user_notification',
        sa.Column('id', sa.Integer(), nullable=False),
        sa.Column('title', sa.String(length=100), nullable=False),
        sa.PrimaryKeyConstraint('id', name=op.f('pk_user_notification')),
    )
def downgrade():
    op.drop_table('user_notification')

$ venv/bin/alembic upgrade head
INFO  [alembic.runtime.migration] Context impl MSSQLImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade  -> 3905e21d3e98, initial

-- DDL from DBeaver
CREATE TABLE db_empty.dbo.user_notification (
	id int IDENTITY(1,1) NOT NULL,
	title varchar(100) COLLATE SQL_Latin1_General_CP1_CI_AS NOT NULL,
	CONSTRAINT pk_user_notification PRIMARY KEY (id)
);

# From pyodbc
$ isql -v -k "DRIVER={ODBC Driver 17 for SQL Server};SERVER=172.29.0.1,1433;DATABASE=db_empty;UID=root;PWD=password"
SQL> sp_help user_notification
...
[ISQL]INFO: SQLMoreResults returned SQL_SUCCESS_WITH_INFO
+---------------------------------------------------------------------------------------------------------------------------------+---------------+
| Column_name  | Type    | Computed | Length | Prec | Scale| Nullable | TrimTrailingBlanks | FixedLenNullInSource | Collation                     |
+---------------------------------------------------------------------------------------------------------------------------------+---------------+
| id           | int     | no       | 4      | 10   | 0    | no       | (n/a)              | (n/a)                 |                              |
| title        | varchar | no       | 511    |      |      | no       | no                 | no                    | SQL_Latin1_General_CP1_CI_AS |
+---------------------------------------------------------------------------------------------------------------------------------+---------------+
SQLRowCount returns 2

Create second migration - change type for user_notification.title

$ venv/bin/alembic revision -m "change_type"
  Generating /home/abdealijk/alembic-test/alembic/versions/0e6416b813f6_change_type.py ...  done

# Add upgrade/downgrade for the second migration manually
$ cat alembic/versions/0e6416b813f6_change_type.py
"""change_type
Revision ID: 0e6416b813f6
Revises: 3905e21d3e98
Create Date: 2022-01-12 21:26:41.093681
"""
from alembic import op
import sqlalchemy as sa
# revision identifiers, used by Alembic.
revision = "0e6416b813f6"
down_revision = "3905e21d3e98"
branch_labels = None
depends_on = None
def upgrade():
    with op.batch_alter_table("user_notification") as batch_op:
        batch_op.alter_column(
            "title",
            existing_type=sa.String(length=100),
            type_=sa.String(length=511),
            existing_nullable=False,
        )
def downgrade():
    with op.batch_alter_table("user_notification") as batch_op:
        batch_op.alter_column(
            "title",
            existing_type=sa.String(length=511),
            type_=sa.String(length=100),
            existing_nullable=False,
        )

$ venv/bin/alembic upgrade head
INFO  [alembic.runtime.migration] Context impl MSSQLImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 3905e21d3e98 -> 0e6416b813f6, change_type

-- DDL from Dbeaver:
CREATE TABLE db_empty.dbo.user_notification (
	id int IDENTITY(1,1) NOT NULL,
	title varchar(511) COLLATE SQL_Latin1_General_CP1_CI_AS NULL,
	CONSTRAINT pk_user_notification PRIMARY KEY (id)
);

# From pyodbc
$ isql -v -k "DRIVER={ODBC Driver 17 for SQL Server};SERVER=172.29.0.1,1433;DATABASE=db_empty;UID=root;PWD=password"
SQL> sp_help user_notification
...
[ISQL]INFO: SQLMoreResults returned SQL_SUCCESS_WITH_INFO
+---------------------------------------------------------------------------------------------------------------------------------+---------------+
| Column_name  | Type    | Computed | Length | Prec | Scale| Nullable | TrimTrailingBlanks | FixedLenNullInSource | Collation                     |
+---------------------------------------------------------------------------------------------------------------------------------+---------------+
| id           | int     | no       | 4      | 10   | 0    | no       | (n/a)              | (n/a)                 |                              |
| title        | varchar | no       | 511    |      |      | yes      | no                 | yes                   | SQL_Latin1_General_CP1_CI_AS |
+---------------------------------------------------------------------------------------------------------------------------------+---------------+
SQLRowCount returns 2

SQL run by alembic

-- When I try to generate the SQL from alembic
$ venv/bin/alembic upgrade head --sql
INFO  [alembic.runtime.migration] Context impl MSSQLImpl.
INFO  [alembic.runtime.migration] Generating static SQL
INFO  [alembic.runtime.migration] Will assume transactional DDL.
BEGIN TRANSACTION;
CREATE TABLE alembic_version (
    version_num VARCHAR(32) NOT NULL,
    CONSTRAINT alembic_version_pkc PRIMARY KEY (version_num)
);
GO

INFO  [alembic.runtime.migration] Running upgrade  -> 3905e21d3e98, initial
-- Running upgrade  -> 3905e21d3e98
CREATE TABLE user_notification (
    id INTEGER NOT NULL IDENTITY,
    title VARCHAR(100) NOT NULL,
    CONSTRAINT pk_user_notification PRIMARY KEY (id)
);
GO
INSERT INTO alembic_version (version_num) OUTPUT inserted.version_num VALUES ('3905e21d3e98');
GO

INFO  [alembic.runtime.migration] Running upgrade 3905e21d3e98 -> 0e6416b813f6, change_type
-- Running upgrade 3905e21d3e98 -> 0e6416b813f6
ALTER TABLE user_notification ALTER COLUMN title VARCHAR(511);
GO
UPDATE alembic_version SET version_num='0e6416b813f6' WHERE alembic_version.version_num = '3905e21d3e98';
GO
COMMIT;
GO

The differences I see are:

• In DBeaver DDL: NOT NULL -> NULL
• In isql's command: Nullable changed from no -> yes and FixedLenNullInSource changed from no -> yes

[AbdealiLoKo]

As a workaround - I did:

    with op.batch_alter_table("user_notification") as batch_op:
        batch_op.alter_column(
            "title",
            existing_type=sa.String(length=100),
            type_=sa.String(length=511),
            nullable=False,  # <---------------- Added this line
            existing_nullable=False,
        )

And now the SQL being generated is:

ALTER TABLE user_notification ALTER COLUMN title VARCHAR(511) NOT NULL;

And both isql and DBeaver DDL are showing me that only the type changed - nothing else changed.

But this workaround does not work with oracle which throws the error:

ORA-01442: column to be modified to NOT NULL is already NOT NULL

Looks like MSSQL needs the existing_nullable to be provided even when nullable=None when generating the SQL

[zzzeek]

OK so this was the other thing I suspected, MSSQL's ALTER needs us to maintain the nullability with this particular statement, similar to MySQL, so we'd (alembic internals) need to include the directive explicitly for this case. we dont want you to have to set nullable=False since as you can see other DBs will complain.