Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix get_primary_keys return type #31

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Fix get_primary_keys return type #31

merged 2 commits into from
Feb 20, 2024

Conversation

hab6
Copy link
Contributor

@hab6 hab6 commented Feb 15, 2024

Description

Fix TypeError when querying primary key metadata using reflection.

Related issue #22

Stack dump excerpt and error:

  File "C:\work\.venv\lib\site-packages\sqlalchemy\engine\reflection.py",
        line 1703, in _reflect_pk
    for pk in pk_cons["constrained_columns"]
TypeError: list indices must be integers or slices, not str

Solution

The fix modifies base.py so that the return type from get_primary_keys (wrapped by get_pk_constraint) is now a dictionary (versus a list) and contains the primary key (i.e. constrained column) name.

Note: This solution works with SQLAlchemy version 2.0.27. I was unable to test with SQLAlchemy version 1.4.51 due to an unrelated error that will be addressed in a separate issue.

Testing

Packages and test cases used to verify the fix.

Without the fix, each test case failed with TypeError: list indices must be integers or slices, not str.

With the fix in place, each test case ran successfully.

Versions of selected Python packages used in testing

langchain           0.1.7
langchain-community 0.0.20
langchain-core      0.1.23
langsmith           0.0.87
pyodbc              5.1.0
pypyodbc            1.3.6
setuptools          63.2.0
SQLAlchemy          2.0.27
sqlalchemy-ingres   0.0.5.dev1

Test case 1

test1.py.txt (based on the example in comment)

Test case 2

test2.py.txt (based on the example in comment)

Notable SQLAlchemy related changes

changelog_06.rst - May 16, 2022

Added get_pk_constraint() to reflection.Inspector, similar
to get_primary_keys() except returns a dict that includes the
name of the constraint, for supported backends (PG so far).

changelog_08.rst - Oct 2, 2022

Inspector.get_primary_keys() is
deprecated; use Inspector.get_pk_constraint().

changelog_14.rst - Jan 2, 2024

Remove deprecated method ``get_primary_keys`` in the :class:`.Dialect`
and :class:`_reflection.Inspector` classes. Please refer to the
:meth:`.Dialect.get_pk_constraint`
and :meth:`_reflection.Inspector.get_primary_keys` methods.

Question to consider

Would it be valuable forget_pk_constraint/get_primary_keys to also return the constraint name (in addition to the column name)? I checked the iikeys table of my Actian X 11.2 data database and found these examples of constraint names for defined primary keys: $user__U0000025B0000, $addre_U0000025E0000, $famil_U000002E90000 I don't know whether these constraint names would be at all useful when retrieving primary key metadata.

@hab6 hab6 requested a review from clach04 February 15, 2024 22:47
lib/sqlalchemy_ingres/_version.py Outdated Show resolved Hide resolved
lib/sqlalchemy_ingres/base.py Show resolved Hide resolved
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.

2 participants