Repository navigation
Conversation
Fixes sqlalchemy#1085: create_table and column render and DDL paths used if comment: which dropped empty string, so reflection returned None and autogenerate looped forever. Change to is not None so comment='' is emitted. Oracle special cased in comparators where '' equals NULL.
|
Thanks! Issue #1085 is now marked code review in progress and no longer open for pull requests, so this pull request holds the review for it and another one won't land on top of your work. If this pull request is abandoned, a maintainer can put open for pull requests back on #1085 to reopen it to others. |
|
Added tests in tests/test_autogen_render.py: empty comment render for column, table, and add_column, plus column and table comparator checks for distinct empty vs none on postgresql and equal on oracle. |
|
Refactored per review: added DefaultImpl.normalize_comment as no-op with OracleImpl override mapping empty string to None, comparators now use migration_context.impl hook instead of dialect name check. Tests updated to cover the hook. |
|
thanks. it seems mostly fine. I'll run the ci and assuming it's fine I'll bring it to gerrit where we usually do the review. thanks! |
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 60ffb5a of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 60ffb5a: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
Thanks for the update. It's close. Let us know if you prefer us to finish this.
- A test for oracle would be nice to have.
Let's wait for mike opinion on the tests before doing anything
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
| "nullable=True, comment=''))", | ||
| ) | ||
|
|
||
| def test_compare_column_empty_comment_distinct(self): |
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
not sure if this test and the following one should be moved to test_autogen_diffs. What do you think mike?
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
There was a problem hiding this comment.
these should be real autogendiff tests with __backend__ = True, they should run for every backend that supports comments, and yes they should be in test_autogen_diffs
|
Thanks for picking this up. Happy to add the Oracle test once Mike weighs in, otherwise glad for you to finish it. Just let me know which you prefer. |
| from types import SimpleNamespace | ||
|
|
||
| from alembic.autogenerate.compare import comments | ||
| from alembic.ddl.impl import DefaultImpl |
There was a problem hiding this comment.
we dont put imports in function bodies
There was a problem hiding this comment.
Done, moved all five imports to module level in both tests. The 5 empty comment tests pass.
Signed-off-by: vineoy <singhvinayak288@gmail.com>
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 c0c50f3 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset c0c50f3 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
IMO test_autogen_diffs should have a __backend__ test class CompareCommentTest that runs for every backend that supports comments.
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
| def test_render_add_column_with_empty_comment(self): | ||
| op_obj = ops.AddColumnOp("foo", Column("x", Integer, comment="")) | ||
| eq_ignore_whitespace( | ||
| autogenerate.render_op_text(self.autogen_context, op_obj), |
There was a problem hiding this comment.
Michael Bayer (zzzeek) wrote:
absoutely part of autogen_diffs, it's testing _compare_column_comment so that's diffs
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
|
ive left new responses to those comments. let me know if you intend to keep going, we can pull it in anytime |
Per review: compare tests should be real autogendiff tests with __backend__ = True running for every backend that supports comments, in test_autogen_diffs. Remove mocked SimpleNamespace tests from test_autogen_render.py. Oracle normalizes empty string to NULL so expects no diff there.
|
updated: moved the compare coverage to CompareCommentTest in test_autogen_diffs.py with backend = True / requires comments, Oracle expects no diff. Render tests stay in test_autogen_render.py. Let me know if you want tweaks. |
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 5eacb86 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 5eacb86 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Federico Caselli (CaselIT) wrote:
just some nitpick on the test, nothing that can't be ignored
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
alembic/testing/suite/test_autogen_comments.py (line 359):
doesn't this need @config.requirements.allows_empty_table_comments like in the class above?
alembic/testing/suite/test_autogen_comments.py (line 380):
the empty to non-empty could be used also in the class above?
alembic/testing/suite/test_autogen_comments.py (line 406):
here to @config.requirements.allows_empty_table_comments
tests/requirements.py (line 22):
sqlite? do we even reflect comments on it?
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
AI Robot 1 (ai-robot-1) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
| eq_(diffs[0][0], "remove_fk") | ||
|
|
||
|
|
||
| class CompareCommentTest(AutogenFixtureTest, TestBase): |
There was a problem hiding this comment.
AI Robot 1 (ai-robot-1) wrote:
Done in patch set 4. These tests are now in alembic/testing/suite/test_autogen_comments.py, and tests/test_autogen_diffs.py is back to its main-branch state.
On real databases, Oracle isn't the only backend where an empty comment can't be stored. PostgreSQL drops the comment on COMMENT ON ... IS '', and MySQL/MariaDB treat an empty comment as no comment. All three reflect comment='' back as None. That is why the earlier compare tests failed on PostgreSQL, MySQL and MSSQL in CI. So instead of an Oracle-only test, there are two suite classes:
- AutogenerateEmptyCommentsTest, which requires
allows_empty_comments(SQL Server). The table-level tests here also requireallows_empty_table_comments, which is currently off because SQLAlchemy's mssql dialect reflects an empty table comment as None. - AutogenerateEmptyCommentsAreNullTest, which requires
empty_comments_are_null(PostgreSQL, MySQL, MariaDB, Oracle). It checks that '' vs None produces no diff.
On the implementation side, DefaultImpl has a new supports_empty_comments flag, set to False for the PostgreSQL, MySQL and Oracle impls, and normalize_comment() maps '' to None when the flag is False.
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
AI Robot 1 (ai-robot-1) wrote:
code review left on gerrit
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016
alembic/testing/suite/test_autogen_comments.py (line 359):
Done. Patch set 5 drops the allows_empty_comments / allows_empty_table_comments / empty_comments_are_null requirements entirely. PostgreSQL, MySQL/MariaDB and Oracle can't store an empty comment distinctly from no comment, so the comparators now treat '' the same as None on every backend. There's a single AutogenerateEmptyCommentsTest class that requires only comments, and it runs on SQL Server too.
alembic/testing/suite/test_autogen_comments.py (line 380):
Done. Patch set 5 has a single AutogenerateEmptyCommentsTest class, and it includes the empty-to-non-empty and non-empty-to-empty tests for both columns and tables.
alembic/testing/suite/test_autogen_comments.py (line 406):
Done. allows_empty_table_comments no longer exists in patch set 5. Empty comments are treated as None on every backend, so this table test runs wherever comments are supported.
tests/requirements.py (line 22):
Done. Patch set 5 removes these requirements, so tests/requirements.py is unchanged from main. On the SQLite question: no, SQLite reports supports_comments=False, so the comments requirement already skips it.
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/7016 has been merged. Congratulations! :) |
4 participants
Fixes #1085
Root cause: create_table and column render plus DDL paths used if comment: so empty string was never emitted. Reflection returned None, comparator correctly fired STOP every run, phantom migration loop.
Change to is not None in:
Oracle: empty string equals NULL there, so comparators in autogenerate/compare/comments.py normalize empty string to None only for oracle dialect. Other backends keep None vs empty string distinct.
Tests: test_autogen_render, test_op, test_autogen_diffs green. Manual render of Table comment='' and Column comment='' now emits comment=''.