-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Upgrade SQLAlchemy to v2 #8410
Upgrade SQLAlchemy to v2 #8410
Conversation
@wardi @smotornyuk I merged master and got a lot of scary typing failures that luckily went away after adapting the tracking model definition (6325fa4) This PR looks good to me. I think it would be good to merge this and start working with it on master going forward to identify potential issues way ahead of a 2.12 release. |
@wardi @smotornyuk ping any objections to merge this? |
Nothing here, I finished all the things I planned for this PR. There are a number of improvements that can be done with new version of SQLalchemy, but I'd like to make them in a separate PR, to avoid blocking this dependency upgrade |
When CKAN v2.11 is out and its code is completely compatible with SQLAlcheny v2, extension maintainers can start upgrading their extensions. I'll add a page to the wiki with upgrade tips.
As for the core, we can now upgrade to SQLAlchemy v2. The main part of this upgrade includes only changes to the version inside the requirements file. It's enough to install the latest SQLAlchemy and nothing will be broken.
But v2 of SQLAlchemy also includes built-in type-definition, so we no longer need any mypy or sqlalchemy-stubs addons. So the secondary feature of this PR is making our typing compatible with new types definitions.
ℹ️ When upgrading CKAN locally, you have to remove external type definitions, or they will cause incorrect type detection. Run
pip uninstall -y sqlalchemy2-stubs sqlalchemy-stubs
For anyone who is going to review this PR, here are the most common types of changes:
== False
it's better to use== false()
. The result is the same, but the latter is accepted by linters. I changed this expression only inside files that were changed due to other reasons.model.Session.query
is properly typed now, so we don't need explicit types when assigning its result into a variable.x: Query[...] = model.Session.query(...)
transformed intox = model.Session.query(...)
model.Session.query
now always returns a sequence of theRow
object.list[tuple[TYPE]]
transformed intoSequence[Row[tuple[TYPE]]]
. Sometimeslist
is used instead of theSequence
, to reduce number of changes. But for new code it's recommended to useSequence
..as_mutable(JSONB)
- this type signature is not yet completed and requires# type: ignore
commentSelf
,.query(cls)
must be used instead of static form, like.query(Tag)
, becauseTag
class is not final and can be extended by child classpgcode
property exists in SQLAlchemy exceptions only when working with PostgreSQL. Type checker doesn't know that we are not going to use other DB engines, so any access topgcode
must be wrapped into a typecast expression.