-
Notifications
You must be signed in to change notification settings - Fork 28
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
rsc: Basic attempt at uuid as id #1493
Conversation
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.
By drop sqlite support you mean "drop sqlite support for rsc" right?
If so I think this seems like a clean change, modulo my comment.
@@ -65,7 +65,7 @@ impl MigrationTrait for Migration { | |||
.table(LocalBlobStore::Table) | |||
.col( | |||
ColumnDef::new(LocalBlobStore::Id) | |||
.integer() | |||
.uuid() | |||
.not_null() | |||
.primary_key(), |
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.
Does this need the default call pointing to gen_random_uuid
?
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.
Nice call out, in this case no but only for a sneaky reason.
LocalBlobStore is a "dependent table" of the BlobStore (kind of like a subclass) and its ID is a foreign key to BlobStore's ID. That means the ID must always be manually set to and already existing BlobStore ID
Yeah lol, changing that for wake would be a very large project |
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
This change is updates all models/tables to use UUIDs. This is important for concurrently/db destruction/multi-client as we can't guarentee that two different clients won't ever get the same integer id representing 2 different jobs. With this change, even if bad db stuff happens we ensure running clients don't confuse jobs as every new job will always get a unique id regardless of the previous state of the db.
It properly generates new UUIDs by default but it takes a hard dependence on Postgres. To land this PR we would have to drop sqlite3 support which has now been completed.
This change rewrites history but we just did that in the last PR so might as well get them all in at once
The standalone tests have been updated and now properly use a sandboxed postgres