refactor: make drop_constraint() constraint_name Optional

[JudeDavis1]

Alembic generates drop_constraint in migration files, sometimes with None as the constraint_name. The current type is str, but alembic still puts None. To stop Mypy from complaining, this parameter needs to be an optional.

[CaselIT]

Hi,

I'm not sure how that drop constraint would succeed when run in the db with specifying a name? Like what would op.drop_constraint(None, 'some_table') do?

@zzzeek Is the above supposed to succeed on some db?

[zzzeek]

nope it's not. if None is generated there from alembic, the migration needs to be corrected. typing doing its job

[CaselIT]

@JudeDavis1 autogenerate is setting none since when creating a constraint a name is not generally needed, so it's optional in the sqlalchemy model.

To properly remove that you will need to find what's the name that was given to it looking in the db with a tool like dbeaver or using the sqlalchemy inspector were constraint names are returned for most databases.

Thanks in any case!

[JudeDavis1]

nope it's not. if None is generated there from alembic, the migration needs to be corrected. typing doing its job

@zzzeek @CaselIT If this was the case, I would expect there to at least be an error/warning saying something.

[CaselIT]

Well the documentation of autogenerate specify that autogenerate produces "candidate" migrations to review and modify manually, so I think this is covered by that.

I don't think a warning should be raised for this, but guess a log indicating that one or more constraint / indexes do not have a name and the drop command should be updated could be added (assuming one is not already issued)

@zzzeek do you think it would be useful or cause too much noise?

[zzzeek]

@zzzeek @CaselIT If this was the case, I would expect there to at least be an error/warning saying something.

there is in fact an error raised, which is when you try to run the migration, it will raise at that point. alembic's autogenerate feature only generates so-called "candidate" Python code and it does not have a layer of error checking within the rendering phase.

Overall, here are all the places that the None condition could be caught:

• When drop_constraint is rendered into the .py file- no, -1 on adding here
• when drop_constraint is checked by mypy yes
• When the drop_constraint function receives the input - no, -1 on adding here
• When the drop_constraint function creates DropConstraintOp - no, this may be a place it can raise ahead of time and for some reason it sees the name as "optional" - if that can be changed we probably should
• when internal constructs are created for the constraint - no
• When SQLAlchemy compiles the "DROP CONSTRAINT" operation - yes, right here
• When the command is run in the database - yes, but it wont get that far due to compile failing

While users frequently want this feature, "autogenerate" is not intended to be an invisible step in an otherwise fully automatic process to generate diffs and upgrade a database. Manually reviewing the code is required, we dont promise transparently correct migrations and we dont raise within the render phase, as it's not necessary.