From: Peter Rabbitson Date: Tue, 14 Feb 2012 22:05:04 +0000 (+0100) Subject: Fix embarrassing leak triggered on calling new_related on an uninserted object X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=0a03206ab54cce5df165c4703bcf8a43252e46ba;hp=66441708b7337cde35fa7f618e23df0c155cd741;p=dbsrgits%2FDBIx-Class-Historic.git Fix embarrassing leak triggered on calling new_related on an uninserted object Reason is a typo-ed hash element in a weaken() call back in 68f3b0dd. Add a bunch of extra tests to catch this, and also better validate the new_related behavior. --- diff --git a/Changes b/Changes index 1e3dfbd..7b3d781 100644 --- a/Changes +++ b/Changes @@ -13,6 +13,7 @@ Revision history for DBIx::Class - A number of corner case fixes of void context populate() with \[] - Fix corner case of forked children disconnecting the parents DBI handle + - Fix leakage of $schema on in-memory new_related() calls * Misc - Codebase is now trailing-whitespace-free diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm index 2b43e47..fdbec40 100644 --- a/lib/DBIx/Class/Relationship/Base.pm +++ b/lib/DBIx/Class/Relationship/Base.pm @@ -461,11 +461,9 @@ sub related_resultset { my $reverse = $source->reverse_relationship_info($rel); foreach my $rev_rel (keys %$reverse) { if ($reverse->{$rev_rel}{attrs}{accessor} && $reverse->{$rev_rel}{attrs}{accessor} eq 'multi') { - $attrs->{related_objects}{$rev_rel} = [ $self ]; - weaken $attrs->{related_object}{$rev_rel}[0]; + weaken($attrs->{related_objects}{$rev_rel}[0] = $self); } else { - $attrs->{related_objects}{$rev_rel} = $self; - weaken $attrs->{related_object}{$rev_rel}; + weaken($attrs->{related_objects}{$rev_rel} = $self); } } } diff --git a/t/52leaks.t b/t/52leaks.t index 5614252..ea055a6 100644 --- a/t/52leaks.t +++ b/t/52leaks.t @@ -179,9 +179,43 @@ my @compose_ns_classes; $schema->txn_rollback; } + # prefetching + my $cds_rs = $schema->resultset('CD'); + my $cds_with_artist = $cds_rs->search({}, { prefetch => 'artist' }); + my $cds_with_tracks = $cds_rs->search({}, { prefetch => 'tracks' }); + my $cds_with_stuff = $cds_rs->search({}, { prefetch => [ 'genre', { artist => { cds => { tracks => 'cd_single' } } } ] }); + + # implicit pref + my $cds_with_impl_artist = $cds_rs->search({}, { columns => [qw/me.title artist.name/], join => 'artist' }); + + # get_column + my $getcol_rs = $cds_rs->get_column('me.cdid'); + my $pref_getcol_rs = $cds_with_stuff->get_column('me.cdid'); + + # fire the column getters + my @throwaway = $pref_getcol_rs->all; + my $base_collection = { resultset => $rs, + pref_precursor => $cds_rs, + + pref_rs_single => $cds_with_artist, + pref_rs_multi => $cds_with_tracks, + pref_rs_nested => $cds_with_stuff, + + pref_rs_implicit => $cds_with_impl_artist, + + pref_row_single => $cds_with_artist->next, + pref_row_multi => $cds_with_tracks->next, + pref_row_nested => $cds_with_stuff->next, + + # even though this does not leak Storable croaks on it :((( + #pref_row_implicit => $cds_with_impl_artist->next, + + get_column_rs_plain => $getcol_rs, + get_column_rs_pref => $pref_getcol_rs, + # twice so that we make sure only one H::M object spawned chained_resultset => $rs->search_rs ({}, { '+columns' => [ 'foo' ] } ), chained_resultset2 => $rs->search_rs ({}, { '+columns' => [ 'bar' ] } ), @@ -193,7 +227,6 @@ my @compose_ns_classes; result_source_handle => $rs->result_source->handle, pager_explicit_count => $pager_explicit_count, - }; require Storable; @@ -201,6 +234,7 @@ my @compose_ns_classes; %$base_collection, refrozen => Storable::dclone( $base_collection ), rerefrozen => Storable::dclone( Storable::dclone( $base_collection ) ), + pref_row_implicit => $cds_with_impl_artist->next, schema => $schema, storage => $storage, sql_maker => $storage->sql_maker, diff --git a/t/multi_create/in_memory.t b/t/multi_create/in_memory.t index f1bb0d7..25d290a 100644 --- a/t/multi_create/in_memory.t +++ b/t/multi_create/in_memory.t @@ -23,22 +23,20 @@ my $schema = DBICTest->init_schema(); { my $new_artist = $schema->resultset("Artist")->new_result({ 'name' => 'Depeche Mode' }); my $new_related_cd = $new_artist->new_related('cds', { 'title' => 'Leave in Silence', 'year' => 1982}); - eval { + lives_ok { $new_artist->insert; $new_related_cd->insert; - }; - is ($@, '', 'Staged insertion successful'); + } 'Staged insertion successful'; ok($new_artist->in_storage, 'artist inserted'); ok($new_related_cd->in_storage, 'new_related_cd inserted'); } { - my $new_artist = $schema->resultset("Artist")->new_result({ 'name' => 'Depeche Mode' }); + my $new_artist = $schema->resultset("Artist")->new_result({ 'name' => 'Mode Depeche' }); my $new_related_cd = $new_artist->new_related('cds', { 'title' => 'Leave Slightly Noisily', 'year' => 1982}); - eval { + lives_ok { $new_related_cd->insert; - }; - is ($@, '', 'CD insertion survives by finding artist'); + } 'CD insertion survives by finding artist'; ok($new_artist->in_storage, 'artist inserted'); ok($new_related_cd->in_storage, 'new_related_cd inserted'); } @@ -48,10 +46,9 @@ my $schema = DBICTest->init_schema(); my $new_artist = $schema->resultset("Artist")->new ({ 'name' => 'Depeche Mode 2: Insertion Boogaloo' }); $new_cd->artist ($new_artist); - eval { + lives_ok { $new_cd->insert; - }; - is ($@, '', 'CD insertion survives by inserting artist'); + } 'CD insertion survives by inserting artist'; ok($new_cd->in_storage, 'new_related_cd inserted'); ok($new_artist->in_storage, 'artist inserted'); @@ -60,6 +57,21 @@ my $schema = DBICTest->init_schema(); is ($retrieved_cd->artist->name, 'Depeche Mode 2: Insertion Boogaloo', 'Correct artist attached to cd'); } +{ + my $new_cd = $schema->resultset('CD')->new ({ 'title' => 'Leave screaming Off Key in the nude', 'year' => 1982}); + my $new_related_artist = $new_cd->new_related( artist => { 'name' => 'Depeche Mode 3: Insertion Boogaloo' }); + lives_ok { + $new_related_artist->insert; + $new_cd->insert; + } 'CD insertion survives after inserting artist'; + ok($new_cd->in_storage, 'cd inserted'); + ok($new_related_artist->in_storage, 'artist inserted'); + + my $retrieved_cd = $schema->resultset('CD')->find ({ 'title' => 'Leave screaming Off Key in the nude'}); + ok ($retrieved_cd, 'CD found in db'); + is ($retrieved_cd->artist->name, 'Depeche Mode 3: Insertion Boogaloo', 'Correct artist attached to cd'); +} + # test both sides of a 1:(1|0) { for my $reldir ('might_have', 'belongs_to') { @@ -102,13 +114,13 @@ my $schema = DBICTest->init_schema(); [ { artist => 1, - cdid => 9, + cdid => 10, genreid => undef, single_track => undef, title => "$reldir: Latest cd", tracks => [ { - cd => 9, + cd => 10, last_updated_at => undef, last_updated_on => undef, position => 1, @@ -120,7 +132,7 @@ my $schema = DBICTest->init_schema(); }, { artist => 1, - cdid => 10, + cdid => 11, genreid => undef, single_track => 19, title => "$reldir: Awesome first single",