Users table add id column#287
Conversation
And key all reactor prefs on it instead of username Claude code assisted with this change.
Davis-A
force-pushed
the
users-table-add-id-column
branch
from
439b256 to
ccc99d8
Compare
performs the migration of the database to have user ids, and updates all the reactor state to re-key their preferences on an id instead of username. Claude code assisted with this change.
Davis-A
force-pushed
the
users-table-add-id-column
branch
from
ccc99d8 to
11802d4
Compare
|
Sorry, I'm keen to get to this, but it's been slightly below the fold for a few days. It's on my list, though. |
|
Meantime, how about you unbreak the build, so we have CI results? dzil won't build the dist without that fixed, so the tests can't run. |
|
I've pushed a fix for the PODNAME issue. |
rjbs
left a comment
rjbs
left a comment
There was a problem hiding this comment.
Right now, this code feels like it's going to add complexity to a bunch of places that will last forever, instead of being a clean cutover with minimal leftovers. I think we can accomplish this with left retained code.
| 'SELECT u.username, ui.identity_name, ui.identity_value | ||
| FROM user_identities ui | ||
| JOIN users u ON ui.user_id = u.id' | ||
| ); |
There was a problem hiding this comment.
I find this hunk of changes weird. The use of defined_kv above means that we will happily load users from the db who have no id field -- which would make sense if we did that to migrate, or if things fell back.
But instead, we then go to find their user identities by looking only identities that have non-null user ids, so any user who has no user id will have no identity.
I feel like the upgrade story here isn't quite coherent. Possibly I'm confused, but I don't think so. (Let's talk.) The migration program does the db schema upgrade. You won't have an id column until it's run. If that's the case, this code would crash at runtime: we'd be querying for a does-not-exist column.
If migration has run, then id exists and is populated. In that case, the defined_kv call doesn't make sense.
| q{VALUES (?,?,?,?)} | ||
| )); | ||
|
|
||
| state $user_insert_with_id_sth = $dbh->prepare(join(q{ }, |
There was a problem hiding this comment.
Technically safe exactly as is, but using a state sth with my dbh invites future trouble if the dbh were able to change once in a while. You'd have an sth pointing to a stale dbh. Better to just make this and other statement handles my.
I see now that you didn't make these state, but maybe while you're in here… ;-)
| ); | ||
| $user_id = $dbh->last_insert_id; | ||
| $user->_set_id($user_id); | ||
| } |
There was a problem hiding this comment.
In what case would we have a user with no id?
| } | ||
| $username_keyed{ $user->username } = $prefs->{$user_id}; | ||
| } | ||
| $self->_load_preferences(\%username_keyed); |
There was a problem hiding this comment.
Really not clear to me what this code (and the similar code above) is for. Is this meant to live on forever? Gets back to "do we have a world where users might have ids, but not must have ids?"
Feels like we should be planning for a world where every user always has an id, which eliminates… all? of this code? We get there by taking down Synergy, running the upgrade, then bringing up the new version.
2 participants
No description provided.