Skip to content

Users table add id column #287

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

Open
Davis-A wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Users table add id column #287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e15f1d
Database: add an id column to the users table
Davis-A Mar 20, 2026
11802d4
database migration script
Davis-A Mar 20, 2026
fe4978e
migrate-user-ids: add a PODNAME comment
rjbs Apr 24, 2026
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
17 changes: 10 additions & 7 deletions lib/Synergy/Environment.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,31 +126,34 @@ has state_dbh => (
);

sub _maybe_create_state_tables ($self) {
$self->state_dbh->do(q{
my $dbh = $self->state_dbh;

$dbh->do(q{
CREATE TABLE IF NOT EXISTS synergy_state (
reactor_name TEXT PRIMARY KEY,
stored_at INTEGER NOT NULL,
json TEXT NOT NULL
);
});

$self->state_dbh->do(q{
$dbh->do(q{
CREATE TABLE IF NOT EXISTS users (
username TEXT PRIMARY KEY,
id INTEGER PRIMARY KEY,
username TEXT UNIQUE NOT NULL,
is_master INTEGER DEFAULT 0,
is_virtual INTEGER DEFAULT 0,
is_deleted INTEGER DEFAULT 0
);
});

$self->state_dbh->do(q{
$dbh->do(q{
CREATE TABLE IF NOT EXISTS user_identities (
id INTEGER PRIMARY KEY,
username TEXT NOT NULL,
user_id INTEGER NOT NULL,
identity_name TEXT NOT NULL,
identity_value TEXT NOT NULL,
FOREIGN KEY (username) REFERENCES users(username) ON DELETE CASCADE,
CONSTRAINT constraint_username_identity UNIQUE (username, identity_name),
FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE,
CONSTRAINT constraint_user_identity UNIQUE (user_id, identity_name),
UNIQUE (identity_name, identity_value)
);
});
Expand Down
29 changes: 24 additions & 5 deletions lib/Synergy/Reactor/Status.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,23 +50,42 @@ listener chatter => async sub ($self, $event) {
return;
};

sub _remap_to_user_ids ($self, $hashref) {
my $ud = $self->hub->user_directory;
my %id_keyed;
for my $username (keys %$hashref) {
my $user = $ud->user_named($username) or next;
$id_keyed{ $user->id } = $hashref->{$username};
}
return \%id_keyed;
}

sub _remap_from_user_ids ($self, $hashref) {
my $ud = $self->hub->user_directory;
my %username_keyed;
for my $user_id (keys %$hashref) {
my $user = $ud->user_by_id($user_id) or next;
$username_keyed{ $user->username } = $hashref->{$user_id};
}
return \%username_keyed;
}

sub state ($self) {
return {
chatter => $self->_last_chatter,
doings => $self->_user_doings,
chatter => $self->_remap_to_user_ids($self->_last_chatter),
doings => $self->_remap_to_user_ids($self->_user_doings),
};
}

after register_with_hub => sub ($self, @) {
if (my $state = $self->fetch_state) {
if ($state->{chatter}) {
$self->_last_chatter->%* = $state->{chatter}->%*;
$self->_last_chatter->%* = $self->_remap_from_user_ids($state->{chatter})->%*;
}

if ($state->{doings}) {
my $doings = $self->_user_doings;

%$doings = $state->{doings}->%*;
%$doings = $self->_remap_from_user_ids($state->{doings})->%*;
}
}
};
Expand Down
18 changes: 15 additions & 3 deletions lib/Synergy/Reactor/TimeClock.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,15 @@ has clock_out_channel => (
);

sub state ($self) {
my $ud = $self->hub->user_directory;
my %times_by_id;
for my $username (keys %{ $self->user_last_report_times }) {
my $user = $ud->user_named($username) or next;
$times_by_id{ $user->id } = $self->user_last_report_times->{$username};
}
return {
last_report_time => $self->last_report_time,
user_last_report_times => $self->user_last_report_times,
last_report_time => $self->last_report_time,
user_last_report_times => \%times_by_id,
};
}

Expand All @@ -127,7 +133,13 @@ after register_with_hub => sub ($self, @) {
}

if (my $times = $state->{user_last_report_times}) {
$self->_set_user_last_report_times($times);
my $ud = $self->hub->user_directory;
my %times_by_username;
for my $user_id (keys %$times) {
my $user = $ud->user_by_id($user_id) or next;
$times_by_username{ $user->username } = $times->{$user_id};
}
$self->_set_user_last_report_times(\%times_by_username);
}
}
};
Expand Down
20 changes: 17 additions & 3 deletions lib/Synergy/Reactor/Vestaboard.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,9 +418,15 @@ sub _validate_design ($self, $design) {
# secret: { expires_at: EPOCH-SEC, value: STRING }

sub state ($self) {
my $ud = $self->hub->user_directory;
my %user_state_by_id;
for my $username (keys %{ $self->_user_state }) {
my $user = $ud->user_named($username) or next;
$user_state_by_id{ $user->id } = $self->_user_state->{$username};
}
return {
user => $self->_user_state,
lock => $self->_lock_state,
user => \%user_state_by_id,
lock => $self->_lock_state,
current_characters => $self->_current_characters,
};
}
Expand Down Expand Up @@ -477,7 +483,15 @@ after register_with_hub => sub ($self, @) {
}

if (my $state = $self->fetch_state) {
$self->_set_user_state($state->{user});
if (my $user_state = $state->{user}) {
my $ud = $self->hub->user_directory;
my %by_username;
for my $user_id (keys %$user_state) {
my $user = $ud->user_by_id($user_id) or next;
$by_username{ $user->username } = $user_state->{$user_id};
}
$self->_set_user_state(\%by_username);
}
$self->_set_lock_state($state->{lock});
$self->_set_current_characters($state->{current_characters})
if $self->_current_characters;
Expand Down
33 changes: 31 additions & 2 deletions lib/Synergy/Role/HasPreferences.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,28 @@ role {
return $value;
};

# Returns the user directory for preference key translation. For reactors
# this is hub->user_directory; for UserDirectory itself, it is $self.
method _pref_user_directory => sub ($self) {
return $self->hub->user_directory if $self->can('hub');
return $self;
};

around state => sub ($orig, $self, @rest) {
my $state = $self->$orig(@rest);
$state->{preferences} = $self->user_preferences;

my $ud = $self->_pref_user_directory;
my %id_keyed;
for my $username (keys %all_user_prefs) {
my $user = $ud->user_named($username);
unless ($user) {
$Logger->log(["HasPreferences: skipping prefs for unknown user %s during save", $username]);
next;
}
$id_keyed{ $user->id } = $all_user_prefs{$username};
}

$state->{preferences} = \%id_keyed;
return $state;
};

Expand All @@ -186,7 +205,17 @@ role {
my $state = $self->$orig(@rest);

if (my $prefs = $state->{preferences}) {
$self->_load_preferences($prefs);
my $ud = $self->_pref_user_directory;
my %username_keyed;
for my $user_id (keys %$prefs) {
my $user = $ud->user_by_id($user_id);
unless ($user) {
$Logger->log(["HasPreferences: skipping prefs for unknown user id %s during load", $user_id]);
next;
}
$username_keyed{ $user->username } = $prefs->{$user_id};
}
$self->_load_preferences(\%username_keyed);

@rjbs rjbs Apr 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

}

return $state;
Expand Down
6 changes: 6 additions & 0 deletions lib/Synergy/User.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ has is_virtual => (
default => 0,
);

has id => (
is => 'ro',
isa => 'Int',
writer => '_set_id',
);

has username => (
is => 'ro',
isa => 'Str',
Expand Down
86 changes: 70 additions & 16 deletions lib/Synergy/UserDirectory.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use Synergy::User;
use Synergy::Util qw(known_alphabets read_config_file day_name_from_abbr);
use Synergy::Logger '$Logger';
use Lingua::EN::Inflect qw(WORDLIST);
use List::Util qw(first shuffle all);
use List::Util qw(shuffle all);
use DateTime;
use Defined::KV;
use Try::Tiny;
Expand Down Expand Up @@ -76,8 +76,32 @@ has _active_users => (
},
);

after _set_user => sub ($self, @) { $self->_clear_active_users };
after _set_users => sub ($self, @) { $self->_clear_active_users };
has _users_by_id => (
isa => 'HashRef',
traits => [ 'Hash' ],
handles => {
user_by_id => 'get',
},
lazy => 1,
clearer => '_clear_users_by_id',
default => sub ($self) {
my %by_id;
for my $user ($self->all_users) {
next unless defined $user->id;
$by_id{ $user->id } = $user;
}
return \%by_id;
},
);

after _set_user => sub ($self, @) {
$self->_clear_active_users;
$self->_clear_users_by_id;
};
after _set_users => sub ($self, @) {
$self->_clear_active_users;
$self->_clear_users_by_id;
};

sub state ($self) { return {} }

Expand Down Expand Up @@ -129,24 +153,27 @@ sub load_users_from_database ($self) {
my $dbh = $self->env->state_dbh;
my %users;

# load prefs
$self->fetch_state($self->name);

my $user_sth = $dbh->prepare('SELECT * FROM users');
$user_sth->execute;

while (my $row = $user_sth->fetchrow_hashref) {
my $username = $row->{username};
$users{$username} = Synergy::User->new({
my $user = Synergy::User->new({
directory => $self,
defined_kv(id => $row->{id}),
username => $username,
defined_kv(is_master => $row->{is_master}),
defined_kv(is_virtual => $row->{is_virtual}),
defined_kv(deleted => $row->{is_deleted}),
});
$users{$username} = $user;
}

my $identity_sth = $dbh->prepare('SELECT * FROM user_identities');
my $identity_sth = $dbh->prepare(
'SELECT u.username, ui.identity_name, ui.identity_value
FROM user_identities ui
JOIN users u ON ui.user_id = u.id'
);

@rjbs rjbs Apr 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

$identity_sth->execute;

while (my $row = $identity_sth->fetchrow_hashref) {
Expand All @@ -161,7 +188,13 @@ sub load_users_from_database ($self) {
$user->add_identity($row->{identity_name}, $row->{identity_value});
}

# _set_users must come before fetch_state so HasPreferences can translate
# stored user ids back to usernames when loading preferences.
$self->_set_users(\%users);

# load prefs
$self->fetch_state($self->name);

return \%users;
}

Expand Down Expand Up @@ -201,8 +234,14 @@ sub register_user ($self, $user) {
q{VALUES (?,?,?,?)}
));

state $user_insert_with_id_sth = $dbh->prepare(join(q{ },

@rjbs rjbs Apr 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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… ;-)

q{INSERT INTO users},
q{ (id, username, is_master, is_virtual, is_deleted)},
q{VALUES (?,?,?,?,?)}
));

state $identity_insert_sth = $dbh->prepare(join(q{ },
q{INSERT INTO user_identities (username, identity_name, identity_value)},
q{INSERT INTO user_identities (user_id, identity_name, identity_value)},
q{VALUES (?,?,?)}
));

Expand All @@ -212,15 +251,29 @@ sub register_user ($self, $user) {
my $ok = 0;
$dbh->begin_work;
try {
$user_insert_sth->execute(
$user->username,
$user->is_master,
$user->is_virtual,
$user->is_deleted,
);
my $user_id;
if ($user->id) {
$user_insert_with_id_sth->execute(
$user->id,
$user->username,
$user->is_master,
$user->is_virtual,
$user->is_deleted,
);
$user_id = $user->id;
} else {
$user_insert_sth->execute(
$user->username,
$user->is_master,
$user->is_virtual,
$user->is_deleted,
);
$user_id = $dbh->last_insert_id;
$user->_set_id($user_id);
}

@rjbs rjbs Apr 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In what case would we have a user with no id?


for my $pair ($user->identity_pairs) {
$identity_insert_sth->execute($user->username, $pair->[0], $pair->[1]);
$identity_insert_sth->execute($user_id, $pair->[0], $pair->[1]);
}

$self->_set_user($user->username, $user);
Expand All @@ -243,6 +296,7 @@ sub reload_user ($self, $username, $data) {
my $new_user = Synergy::User->new({
%$old,
%$data,
id => $old->id,
username => $username,
});

Expand Down