fix(postgresql): identity column not added when existing_server_default omitted - #1869
RitiGrover wants to merge 2 commits into
Conversation
…lt omitted Operations.alter_column defaults existing_server_default to False (its 'not specified' sentinel), but the PostgreSQL identity-column compiler only checked for None to decide whether to render 'ADD GENERATED ... AS IDENTITY'. False fell through into the diff-against-existing-identity branch, computed an empty diff, and rendered an invalid, clause-less ALTER TABLE ... ALTER COLUMN statement. Fixes sqlalchemy#1504
|
Hi, and thanks for the pull request! This project accepts pull requests only for issues that a maintainer has already marked with the open for pull requests label. That way we can settle on an approach before anyone spends time writing code. This pull request references issue #1504, which hasn't been marked open for pull requests, so the change isn't authorized yet and I'm closing it automatically. Once a maintainer adds the label to #1504, reopen this pull request and it will stay open. If #1504 needs more detail before that can happen, a complete runnable example is usually the missing piece, and adding one to the issue is the fastest way to get there. This is automatic and procedural. It isn't a judgment on your change, and nothing you've written here is lost. Thanks for your interest in the project! |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision e3d5339 of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change e3d5339: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6982 |
|
OK, it failed. the "on_null" thing does not seem to be platform agnostic. if you're using an LLM, make sure it runs the tests before submitting |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
change seems fine, but we could likely reuse an existing test for this
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6982
| "ALTER TABLE t1 ALTER COLUMN some_column DROP IDENTITY" | ||
| ) | ||
|
|
||
| def test_add_identity_to_column_no_existing_server_default(self): |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
how about we add a matrix to test_add_identity_to_column to control existing_server_default?
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6982
Per review feedback: reuse test_add_identity_to_column instead of a
separate test function, and drop Identity(on_null=True) from the
regression case since its rendering isn't consistent across all
tested SQLAlchemy versions. The {} (no options) case already covers
the regression (existing_server_default omitted vs explicit None)
without relying on a non-portable option.
|
Fixed - thanks a lot for your review |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 6e59421 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 6e59421 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6982 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6982
- tests/test_postgresql.py (line 452): Done
3 participants
Description
Operations.alter_columndefaultsexisting_server_defaulttoFalse(its internal "not specified" sentinel) when the caller omits it. The PostgreSQL identity-column DDL compiler (visit_identity_columninalembic/ddl/postgresql.py) only checkedis Noneto decide whether to renderADD GENERATED ... AS IDENTITY, soFalsefell through into the "diff against an existing identity" branch, computed an empty diff against the sentinel, and rendered an invalid, clause-lessALTER TABLE ... ALTER COLUMN id;.Motivation and Context
Fixes #1504. Any migration that sets
server_default=Identity(...)onalter_columnwithout also passingexisting_server_default(the common case, since there usually isn't a prior identity to describe) fails at execution time with a Postgres syntax error.How Has This Been Tested?
test_add_identity_to_column_no_existing_server_default) mirroring the existingtest_add_identity_to_columntest; confirmed it fails on the old code and passes with the fixtest_postgresql.pyidentity suite (13/13 pass) plus the rest of the offline and live-DB PostgreSQL suite - no regressionsflake8,black --check,mypyall clean on the changed files