-
Notifications
You must be signed in to change notification settings - Fork 379
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(migration): Migrate reports with appropriate default checker ID #4191
fix(migration): Migrate reports with appropriate default checker ID #4191
Conversation
93b11da
to
47851be
Compare
I noticed that instead of utilizing SQLAlchemy's built in methods, raw SQL queries were executed directly. While this approach can work, it may introduce potential risks (for example when table or column name is changed). Would it be possible to refactor the queries? |
...ort/versions/c3dad71f8e6b_store_information_about_enabled_and_disabled_checkers_for_a_run.py
Outdated
Show resolved
Hide resolved
That is very common during migration: [1], [2], [3]. When this script is executing, the schema in the database is not available in an ORM-y way, and there is a very high chance that the schema in the database "right now" does not match what the db model definitions in the Python package contain.
Not even Report = Base.classes.reports # 'reports' is the table name!
Checker = Base.classes.checkers in the earlier code. I can try changing this specific query to use |
The first row in a table with an `AUTOINCREMENT` ID has the value 1, not 0. This was synchronised in the migration script to have `server_default="1"` such that all default constructed `report` rows have a **valid** `FOREIGN KEY` to the `checkers` table (the `__FAKE__` checker). But the hand-written upgrade query filtered for `0` to get a batch of "unhandled rows", essentially preventing appropriate migration.
47851be
to
3cfb1c2
Compare
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.
LGTM
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.
LGTM
The first row in a table with an
AUTOINCREMENT
ID has the value 1, not 0. This was synchronised in the migration script to haveserver_default="1"
such that all default constructedreport
rows have a validFOREIGN KEY
to thecheckers
table (the__FAKE__
checker). But the hand-written upgrade query filtered for0
to get a batch of "unhandled rows", essentially preventing appropriate migration.