Skip to content

Commit

Permalink
Implement cascaded deletion of cached group members on DB level
Browse files Browse the repository at this point in the history
The performance is obviously better to clean up stuff on DB level. As
Via is a foreign key, it can't be 0 any more.
  • Loading branch information
sunnavy committed Apr 3, 2024
1 parent ffdaf3e commit f22f1b1
Show file tree
Hide file tree
Showing 11 changed files with 22 additions and 47 deletions.
3 changes: 2 additions & 1 deletion etc/schema.Oracle
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@ CREATE TABLE CachedGroupMembers (
MemberId NUMBER(11,0),
Via NUMBER(11,0),
ImmediateParentId NUMBER(11,0),
Disabled NUMBER(11,0) DEFAULT 0 NOT NULL
Disabled NUMBER(11,0) DEFAULT 0 NOT NULL,
FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE
);
CREATE INDEX DisGrouMem ON CachedGroupMembers (GroupId, MemberId, Disabled);
CREATE INDEX CachedGroupMembers2 ON CachedGroupMembers (MemberId, GroupId, Disabled);
Expand Down
1 change: 1 addition & 0 deletions etc/schema.Pg
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ CREATE TABLE CachedGroupMembers (
Via int,
ImmediateParentId int,
Disabled integer NOT NULL DEFAULT 0 ,
FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE,
PRIMARY KEY (id)

);
Expand Down
3 changes: 2 additions & 1 deletion etc/schema.SQLite
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,12 @@ create table CachedGroupMembers (
MemberId int,
Via int,
ImmediateParentId int,
Disabled int2 NOT NULL DEFAULT 0 # if this cached group member is a member of this group by way of a disabled
Disabled int2 NOT NULL DEFAULT 0, # if this cached group member is a member of this group by way of a disabled
# group or this group is disabled, this will be set to 1
# this allows us to not find members of disabled subgroups when listing off
# group members recursively.
# Also, this allows us to have the ACL system elide members of disabled groups
FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE


) ;
Expand Down
1 change: 1 addition & 0 deletions etc/schema.mysql
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ create table CachedGroupMembers (
# this allows us to not find members of disabled subgroups when listing off
# group members recursively.
# Also, this allows us to have the ACL system elide members of disabled groups
FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE,
PRIMARY KEY (id)
) ENGINE=InnoDB CHARACTER SET utf8mb4;

Expand Down
1 change: 1 addition & 0 deletions etc/upgrade/5.0.6/schema.Oracle
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE CachedGroupMembers ADD FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE;
1 change: 1 addition & 0 deletions etc/upgrade/5.0.6/schema.Pg
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE CachedGroupMembers ADD FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE;
1 change: 1 addition & 0 deletions etc/upgrade/5.0.6/schema.mysql
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE CachedGroupMembers ADD FOREIGN KEY (Via) REFERENCES CachedGroupMembers(id) ON DELETE CASCADE;
47 changes: 7 additions & 40 deletions lib/RT/CachedGroupMember.pm
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ sub Create {
my %args = ( Group => '',
Member => '',
ImmediateParent => '',
Via => '0',
Via => undef,
Disabled => '0',
@_ );

Expand Down Expand Up @@ -141,7 +141,12 @@ sub Create {
. $args{'Via'} );
return (undef); #this will percolate up and bail out of the transaction
}
if ( $self->__Value('Via') == 0 ) {

# Check $args{Via} instead of __Value('Via') to avoid cache issues on
# SQLite that may reuse ids: t/validator/group_members.t fails with
# __Value('Via')

if ( !$args{Via} ) {
my ( $vid, $vmsg ) = $self->__Set( Field => 'Via', Value => $id );
unless ($vid) {
$RT::Logger->warning( "Due to a via error, couldn't create "
Expand Down Expand Up @@ -181,44 +186,6 @@ sub Create {



=head2 Delete
Deletes the current CachedGroupMember from the group it's in and
cascades the delete to all submembers.
=cut

sub Delete {
my $self = shift;


my $member = $self->MemberObj();
if ( $member->IsGroup ) {
my $deletable = RT::CachedGroupMembers->new( $self->CurrentUser );

$deletable->Limit( FIELD => 'id',
OPERATOR => '!=',
VALUE => $self->id );
$deletable->Limit( FIELD => 'Via',
OPERATOR => '=',
VALUE => $self->id );

while ( my $kid = $deletable->Next ) {
my ($ok, $msg) = $kid->Delete();
unless ($ok) {
$RT::Logger->error(
"Couldn't delete CachedGroupMember " . $kid->Id );
return ($ok, $msg);
}
}
}
my ($ok, $msg) = $self->SUPER::Delete();
$RT::Logger->error( "Couldn't delete CachedGroupMember " . $self->Id ) unless $ok;
return ($ok, $msg);
}



=head2 SetDisabled
SetDisableds the current CachedGroupMember from the group it's in and cascades
Expand Down
4 changes: 2 additions & 2 deletions lib/RT/GroupMember.pm
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ sub _InsertCGM {
Member => $self->MemberObj,
Group => $self->GroupObj,
ImmediateParent => $self->GroupObj,
Via => '0'
Via => undef,
);


Expand Down Expand Up @@ -289,7 +289,7 @@ sub _StashUser {
Member => $args{'Member'},
Group => $args{'Group'},
ImmediateParent => $args{'Group'},
Via => '0'
Via => undef,
);

unless ($cached_id) {
Expand Down
1 change: 1 addition & 0 deletions lib/RT/Handle.pm
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ sub Connect {
}
elsif ( $db_type eq 'SQLite' ) {
$self->dbh->{sqlite_see_if_its_a_number} = 1;
$self->dbh->do('PRAGMA foreign_keys = ON'); # ON DELETE CASCADE for CachedGroupMembers needs it
}

$self->dbh->{'LongReadLen'} = RT->Config->Get('MaxAttachmentSize');
Expand Down
6 changes: 3 additions & 3 deletions lib/RT/Migrate/Importer.pm
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ sub CloseStream {
# Groups
$self->RunSQL(<<'EOF');
INSERT INTO CachedGroupMembers (GroupId, MemberId, Via, ImmediateParentId, Disabled)
SELECT Groups.id, Groups.id, 0, Groups.id, Principals.Disabled FROM Groups
SELECT Groups.id, Groups.id, NULL, Groups.id, Principals.Disabled FROM Groups
LEFT JOIN Principals ON ( Groups.id = Principals.id )
LEFT JOIN CachedGroupMembers ON (
Groups.id = CachedGroupMembers.GroupId
Expand All @@ -645,7 +645,7 @@ EOF
# GroupMembers
$self->RunSQL(<<'EOF');
INSERT INTO CachedGroupMembers (GroupId, MemberId, Via, ImmediateParentId, Disabled)
SELECT GroupMembers.GroupId, GroupMembers.MemberId, 0, GroupMembers.GroupId, Principals.Disabled FROM GroupMembers
SELECT GroupMembers.GroupId, GroupMembers.MemberId, NULL, GroupMembers.GroupId, Principals.Disabled FROM GroupMembers
LEFT JOIN Principals ON ( GroupMembers.GroupId = Principals.id )
LEFT JOIN CachedGroupMembers ON (
GroupMembers.GroupId = CachedGroupMembers.GroupId
Expand All @@ -657,7 +657,7 @@ EOF

# Fixup Via
$self->RunSQL(<<'EOF');
UPDATE CachedGroupMembers SET Via=id WHERE Via=0
UPDATE CachedGroupMembers SET Via=id WHERE Via IS NULL
EOF

# Cascaded GroupMembers, use the same SQL in rt-validator
Expand Down

0 comments on commit f22f1b1

Please sign in to comment.