-
Notifications
You must be signed in to change notification settings - Fork 4
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 to SQLAlchemy 2.0 #197
Conversation
@@ -53,41 +50,3 @@ def have_all_migrations_run(db_engine: sqlalchemy.engine.Engine) -> None: | |||
logger.info( | |||
f"The current migration head is up to date, {current_heads} and Alembic is expecting {expected_heads}" | |||
) | |||
|
|||
|
|||
def check_model_parity() -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used and was giving me some type errors. There's a better way to do this anyways as Alembic has a utility now for checking the model parity for you which I'll create a ticket and add later.
# FYI, execute many mode handles how SQLAlchemy handles doing a bunch of inserts/updates/deletes at once | ||
# https://docs.sqlalchemy.org/en/14/dialects/postgresql.html#psycopg2-fast-execution-helpers | ||
executemany_mode="batch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Batch mode no longer exists: https://docs.sqlalchemy.org/en/20/dialects/postgresql.html#psycopg2-fast-execution-helpers
There's another way to configure it to do a similar operation, but I wasn't sure if we wanted to keep it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One request to not access the private member _engine
of DBClient
, a question about establishing in a new connection in a finally clause, and a few other small suggestions, but otherwise LGTM!
Note for when we generate release notes, we might want to point out that this is a breaking change and provide resources for how to migrate (e.g. link to SQLAlchemy migration docs), similar to how we did it on template-infra (see
https://github.com/navapbc/template-infra/releases/tag/v0.3.0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great thanks!
Ticket
#82
Changes
Upgraded to SQLAlchemy 2.0, see context section for major changes in this version
Context for reviewers
SQLAlchemy 2.0 comes with an incredibly thorough migration guide: https://docs.sqlalchemy.org/en/20/changelog/migration_20.html
Thankfully 1.4 started nudging usage of SQLAlchemy to use the new approaches, so the changes needed weren't that large.
Noteworthy changes:
Mapped[...]
, and columns now get defined either withmapped_column
OR nothing with SQLAlchemy figuring out the type from the python type (similar to Dataclasses or Pydantic). This largely results in more minimal class definitions especially ones with a bunch of basic columns.with conn.begin()
blocks to function properly.execute
should be wrapped in thetext
classTesting
Since the DB hits virtually everything, I tested everything as thoroughly as possible.
Because of how the SQLAlchemy-stubs we removed work, they need to be completely deleted otherwise MyPy thinks they should run. If you are running outside of the docker container, run
poetry install --no-root --all-extras --with dev --sync
which will delete extra packages.Basics
Tests, formatting, linting, all working, only required a few fixes to make SQLAlchemy happy.
SQLAlchemy warnings
When running unit tests, SQLAlchemy will output warnings for deprecated features. The only still usable deprecated feature we had was our
get
queries which were adjusted. Prior to the fix, we would see this warning:Migrations
They still work, adding a few new columns generates what we would expect and uses the mapping config added to the Base class.
Swagger
Was able to successfully use the local swagger endpoints and create/update/read from the DB which was populated like so: