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

Postgres backend Record is a Mapping but some Mapping methods are deprecated #407

Closed
ugtar opened this issue Oct 11, 2021 · 5 comments · Fixed by #408
Closed

Postgres backend Record is a Mapping but some Mapping methods are deprecated #407

ugtar opened this issue Oct 11, 2021 · 5 comments · Fixed by #408
Labels
clean up Refinement to existing functionality

Comments

@ugtar
Copy link

ugtar commented Oct 11, 2021

Since #299 upgraded to sqlalchemy 1.4, the postgres backend's Record object now mimics the behavior of sqlalchemy's Row which is meant to behave similarly to a NamedTuple (and inherits from collections.abc.Sequence) https://docs.sqlalchemy.org/en/14/changelog/migration_14.html#change-4710-core

Meanwhile, postgres backend's Record object inherits from collections.abc.Mapping and is therefore required to fulfill the Mapping interface, which includes keys() and values() which are now deprecated.

Sqlalchemy provides a mapping() method on Result which will cause it to return RowMapping objects rather than Row objects, and those look like Mappings.

I encountered this issue working with fastapi and pydantic. Returning Records as pydantic models worked in the past, but now produces a deprecation warning (and i guess will eventually stop working) since pydantic's builtin validator treats the Record as a Mapping and attempts to call dict(record)

@ugtar
Copy link
Author

ugtar commented Oct 11, 2021

the workaround I suppose is to do something like

results = await database.fetch_all(query)
return [r._mapping() for r in results]

instead of simply returning results as before. Still, I hope that having a correct type for Record will make that unnecessary.

@ugtar
Copy link
Author

ugtar commented Oct 12, 2021

FYI I did some more testing, and I think that changing the parent class of Record from collections.abc.Mapping to collections.abc.Sequence does not solve the particular issue with pydantic attempting to dict the Record.

@aminalaee
Copy link
Member

Sorry I'm a little confused now, I don't know much about Pedantic.
Do you mean that this works in Pydantic?

[r._mapping for r in results]

But calling dict(record) produces DeprecationWarning?

@ugtar
Copy link
Author

ugtar commented Oct 12, 2021 via email

@aminalaee
Copy link
Member

@ugtar Yes in that case you are right, Mapping should implement keys() and values(), items(), etc but it doesn't.
I don't see the point in keeping that a Mapping anymore.

@aminalaee aminalaee added the clean up Refinement to existing functionality label Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up Refinement to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants