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.

Sign up for GitHub

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

Jump to bottom

refactor(robot-server): Add a comment to note a mistaken type in migration #12126

Merged
merged 1 commit into from
Feb 10, 2023
Merged

refactor(robot-server): Add a comment to note a mistaken type in migration #12126

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
refactor(robot-server): Note mistaken type in migration
  • Loading branch information
@SyntaxColoring
SyntaxColoring committed Feb 9, 2023
commit ca8083fcd3953f91b16371dca6a70ae33ac107ac
5 changes: 5 additions & 0 deletions robot-server/robot_server/persistence/_migrations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ def _migrate_0_to_1(transaction: sqlalchemy.engine.Connection) -> None:
"""
add_summary_column = sqlalchemy.text("ALTER TABLE run ADD state_summary BLOB")
add_commands_column = sqlalchemy.text("ALTER TABLE run ADD commands BLOB")
# NOTE: The column type of `STRING` here is mistaken. SQLite won't recognize it,
# so this column's type affinity will mistakenly default to `NUMERIC`.
# It should be `VARCHAR`, to match what SQLAlchemy uses when creating a new
# database from scratch. Fortunately, for this particular column, this inconsistency
# is harmless in practice because of SQLite's weak typing.
Comment on lines +126 to +130
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a signal that we need to either:

  • Test that the "migrate from older database" path creates the exact same schema as the "create database from scratch" path.
  • Completely remove the "create database from scratch path" in favor of a chain of migrations, so a mismatch is structurally impossible. Maybe Alembic can help with this.

Alembic would have helped with other things here, too. For example, it would have been proper to change the migrated columns' types from BLOB to VARCHAR, but I couldn't because the column type because SQLite makes that ridiculously difficult to do manually. Alembic, meanwhile, has specific support for this.

add_status_column = sqlalchemy.text("ALTER TABLE run ADD engine_status STRING")
add_updated_at_column = sqlalchemy.text("ALTER TABLE run ADD _updated_at DATETIME")

Expand Down