From fd62170df69e2a6b7220420980588712f314a004 Mon Sep 17 00:00:00 2001 From: Colin Kuskie Date: Sat, 2 Aug 2008 22:48:32 +0000 Subject: [PATCH] Change getGroups to always return safe copies of data. Add tests to check for safe copies. Add a test for getGroupIdsRecursive. --- lib/WebGUI/User.pm | 47 +++++++++++++++++++++++++--------------------- t/User.t | 40 +++++++++++++++++++++++++-------------- 2 files changed, 52 insertions(+), 35 deletions(-) diff --git a/lib/WebGUI/User.pm b/lib/WebGUI/User.pm index c3c26c087..16baacf11 100644 --- a/lib/WebGUI/User.pm +++ b/lib/WebGUI/User.pm @@ -304,7 +304,8 @@ sub getFirstName { =head2 getGroups ( [ withoutExpired ] ) -Returns an array reference containing a list of groups this user is in. +Returns an array reference containing a list of groups this user is in. Group lookups are cached. +If a cached lookup is returned, it will be a safe copy of the data in the cache. =head3 withoutExpired @@ -313,26 +314,30 @@ If set to "1" then the listing will not include expired groupings. Defaults to " =cut sub getGroups { - my $self = shift; - my $withoutExpired = shift; - my $clause = ""; - if ($withoutExpired) { - $clause = "and expireDate>".$self->session->datetime->time(); - } - my $gotGroupsForUser = $self->session->stow->get("gotGroupsForUser"); - if (exists $gotGroupsForUser->{$self->userId}) { - return $gotGroupsForUser->{$self->userId}; - } else { - my @groups = $self->session->db->buildArray("select groupId from groupings where userId=? $clause", [$self->userId]); - my $isInGroup = $self->session->stow->get("isInGroup"); - foreach my $gid (@groups) { - $isInGroup->{$self->userId}{$gid} = 1; - } - $self->session->stow->set("isInGroup",$isInGroup); - $gotGroupsForUser->{$self->userId} = \@groups; - $self->session->stow->set("gotGroupsForUser",$gotGroupsForUser); - return \@groups; + my $self = shift; + my $withoutExpired = shift; + my $clause = ""; + if ($withoutExpired) { + $clause = "and expireDate>".$self->session->datetime->time(); + } + my $gotGroupsForUser = $self->session->stow->get("gotGroupsForUser"); + if (exists $gotGroupsForUser->{$self->userId}) { + my $cachedGroups = $gotGroupsForUser->{$self->userId}; + my @safeCopy = @{ $cachedGroups }; + return \@safeCopy; + } + else { + my @groups = $self->session->db->buildArray("select groupId from groupings where userId=? $clause", [$self->userId]); + my $isInGroup = $self->session->stow->get("isInGroup"); + foreach my $gid (@groups) { + $isInGroup->{$self->userId}{$gid} = 1; } + $self->session->stow->set("isInGroup",$isInGroup); + $gotGroupsForUser->{$self->userId} = \@groups; + $self->session->stow->set("gotGroupsForUser",$gotGroupsForUser); + my @safeGroups = @groups; + return \@safeGroups; + } } #---------------------------------------------------------------------------- @@ -357,7 +362,7 @@ sub getGroupIdsRecursive { $groupIds{ $groupGroupingId } = 1; } } - + return [ keys %groupIds ]; } diff --git a/t/User.t b/t/User.t index 0f8d5ee32..feed6770b 100644 --- a/t/User.t +++ b/t/User.t @@ -20,7 +20,7 @@ use WebGUI::Cache; use WebGUI::User; use WebGUI::ProfileField; -use Test::More tests => 130; # increment this value for each test you create +use Test::More tests => 132; # increment this value for each test you create use Test::Deep; my $session = WebGUI::Test->session; @@ -201,12 +201,6 @@ ok(!WebGUI::User->validUserId($session, 37), 'random illegal Id #2'); $user->delete; -#identifier() and uncache() -SKIP: { - skip("identifier() -- deprecated",1); - ok(undef, "identifier()"); -} - SKIP: { skip("uncache() -- Don't know how to test uncache()",1); ok(undef, "uncache"); @@ -451,12 +445,20 @@ $expiredGroup->expireOffset(-1000); $dude->addToGroups([$expiredGroup->getId]); my $dudeGroups = $dude->getGroups(); -cmp_bag($dudeGroups, ['12', '2', '7', $expiredGroup->getId], 'Dude belongs to Registered Users, Everyone and T.O.A'); +cmp_bag($dudeGroups, [12, 2, 7, $expiredGroup->getId], 'Dude belongs to Registered Users, Everyone and T.O.A'); ##Group lookups are cached, so we'll clear the cache by removing Dude from T.O.A. $dude->deleteFromGroups([12]); -$dudeGroups = $dude->getGroups(1); -cmp_bag($dudeGroups, ['2', '7'], 'Dude belongs to Registered Users, Everyone as unexpired group memberships'); +$dudeGroups = $dude->getGroups(1); ##This is the original call to getGroups; +cmp_bag($dudeGroups, [2, 7], 'Dude belongs to Registered Users, Everyone as unexpired group memberships'); + +##Safe copy check +push @{ $dudeGroups }, 'not a groupId'; +cmp_bag($dude->getGroups(1), [2, 7], 'Accessing the list of groups does not change the cached value'); + +my $dudeGroups2 = $dude->getGroups(1); ##This call gets a cached version. +push @{ $dudeGroups2 }, 'still not a groupId'; +cmp_bag($dude->getGroups(1), [2, 7], 'Accessing the cached list of groups does not change the cached value'); ################################################################ # @@ -570,12 +572,22 @@ foreach my $groupName (qw/red pink orange blue turquoise lightBlue purple/) { $groupSet{$groupName}->name($groupName); } -$groupSet{red }->addGroups( [ map { $groupSet{$_}->getId } qw/pink orange purple/ ] ); -$groupSet{blue}->addGroups( [ map { $groupSet{$_}->getId } qw/turquoise lightBlue purple/ ] ); -$groupSet{blue}->expireOffset(-2500); +$groupSet{blue}->expireOffset(-1500); + +$groupSet{purple}->addGroups( [ map { $groupSet{$_}->getId } qw/red blue pink/ ] ); +$groupSet{lightBlue}->addGroups( [ map { $groupSet{$_}->getId } qw/blue/ ] ); +$groupSet{turquoise}->addGroups( [ map { $groupSet{$_}->getId } qw/blue/ ] ); +$groupSet{pink}->addGroups( [ map { $groupSet{$_}->getId } qw/red/ ] ); +$groupSet{orange}->addGroups( [ map { $groupSet{$_}->getId } qw/red/ ] ); my $newFish = WebGUI::User->new($session, 'new'); -$newFish->addToGroups([ $groupSet{red}->getId, $groupSet{blue}->getId]); +$newFish->addToGroups([ $groupSet{red}->getId, $groupSet{blue}->getId ]); + +cmp_bag( + $newFish->getGroupIdsRecursive, + [ 2, 7, map { $_->getId } @groupSet{qw/red pink orange purple/} ], + 'getGroupIdsRecursive returns the correct set of groups, ignoring expire date and not duplicating groups' +); END { foreach my $account ($user, $dude, $buster, $buster3, $neighbor, $friend, $newFish) {