Skip to content

fix(sqlalchemy-spanner): include the constraint name in reflected primary keys - #18845

Open
rocksnow1942 wants to merge 2 commits into
googleapis:mainfrom
rocksnow1942:fix-18841-pk-constraint-name
Open

rocksnow1942 wants to merge 2 commits into
googleapis:mainfrom
rocksnow1942:fix-18841-pk-constraint-name

Conversation

@rocksnow1942

Copy link
Copy Markdown

get_pk_constraint() and get_multi_pk_constraint() returned primary keys as {"constrained_columns": [...]}, without the name key that SQLAlchemy's ReflectedPrimaryKeyConstraint requires. Since Alembic 1.18.0, autogenerate loads reflection data with the get_multi_*() methods and reads const["name"] from each constraint, so alembic revision --autogenerate and alembic check failed with KeyError: 'name' on any Spanner database that has a table. #18841 has a repro.

This PR selects tc.constraint_name in get_multi_pk_constraint() and returns it as name. Spanner names primary keys PK_<table>.

The existing test_get_multi_pk_constraint compliance test didn't catch this, because the Spanner override of _check_list only compares keys that are in both dicts, so this also adds test_get_pk_constraint_name.

Tested with the unit and mockserver tests, the compliance suite against the emulator, and the Alembic 1.20 repro from #18841.

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #18841

…mary keys

get_pk_constraint() and get_multi_pk_constraint() returned primary keys
without the "name" key that ReflectedPrimaryKeyConstraint requires. Alembic
1.18 and later read it during autogenerate, which then failed with
KeyError: 'name' on any database that has a table. Return the constraint
name.

Fixes googleapis#18841

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Spanner dialect's get_multi_pk_constraint method to fetch and include the primary key constraint name (tc.constraint_name) in the returned dictionary, which is necessary for Alembic autogeneration. Unit and integration tests have been added to verify this behavior. The review feedback suggests optimizing the dictionary lookup in get_multi_pk_constraint by replacing dict.get() with a membership check to avoid unnecessary dictionary and list allocations on each iteration.

Comment thread packages/sqlalchemy-spanner/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py Outdated
…t_multi_pk_constraint

Instead of building a default dict for every row.
@rocksnow1942
rocksnow1942 marked this pull request as ready for review October 24, 2026 09:53
@rocksnow1942
rocksnow1942 requested a review from a team as a code owner October 24, 2026 09:53

This branch has not been deployed

No deployments
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.

[sqlalchemy-spanner]: reflected primary keys have no "name" key, so Alembic >= 1.18 autogenerate fails with KeyError: 'name'

1 participant