Skip to content

Fix rendering of Column objects in dialect kwargs like postgresql_inc…#1802

Open
Ajaysingh-2003 wants to merge 1 commit intosqlalchemy:mainfrom
Ajaysingh-2003:fix-postgresql-include-index
Open

Fix rendering of Column objects in dialect kwargs like postgresql_inc…#1802
Ajaysingh-2003 wants to merge 1 commit intosqlalchemy:mainfrom
Ajaysingh-2003:fix-postgresql-include-index

Conversation

@Ajaysingh-2003
Copy link
Copy Markdown

Fixes #1258

This PR updates _render_dialect_kwargs_items in alembic/autogenerate/render.py to properly handle list/tuple values containing Column objects, such as postgresql_include.

The implementation mirrors the logic used in _get_index_rendered_expressions, ensuring consistent rendering behavior.

Changes:

  • Proper handling of Column objects inside dialect kwargs
  • Fix invalid Python code generation in migrations

Tests:

  • Added tests covering both create_index and drop_index cases

This ensures Alembic generates valid migration scripts when using postgresql_include.

Copy link
Copy Markdown
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. looks almost ok:

idx = Index(
"test_active_code_idx",
t.c.active,
somedialect_include=[t.c.code],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's try a tuple here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, both tests now use tuples.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested only one to that we kept leveraging both code paths in the test. if it's not too much of a both I would prefer if one of the two test kept using list, so that ensure that both work.

thanks!

Copy link
Copy Markdown
Author

@Ajaysingh-2003 Ajaysingh-2003 Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I updated test_render_drop_index to use a Tuple as requested, while keeping test_render_add_index as a List to comprehensively test both code paths. The tests compile properly.

@Ajaysingh-2003 Ajaysingh-2003 force-pushed the fix-postgresql-include-index branch from fefa96d to 20a50c9 Compare April 8, 2026 10:17
@Ajaysingh-2003
Copy link
Copy Markdown
Author

thanks. looks almost ok:

Done — added the changenote in docs/build/unreleased/1258.rst and switched the tests to use tuples.

@Ajaysingh-2003 Ajaysingh-2003 force-pushed the fix-postgresql-include-index branch from 20a50c9 to b6b8326 Compare April 8, 2026 13:22
@Ajaysingh-2003 Ajaysingh-2003 force-pushed the fix-postgresql-include-index branch from b6b8326 to 2a0441c Compare April 8, 2026 13:30
@CaselIT
Copy link
Copy Markdown
Member

CaselIT commented Apr 8, 2026

the fails seem unrelated. thanks!

@CaselIT CaselIT requested a review from sqla-tester April 8, 2026 13:45
Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 2a0441c of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Copy Markdown
Collaborator

New Gerrit review created for change 2a0441c: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Index postgresql_include renders the columns incorrectly

3 participants