Skip Menu |

This queue is for tickets about the Class-DBI CPAN distribution.

Report information
The Basics
Id: 17805
Status: open
Priority: 0/
Queue: Class-DBI

People
Owner: Nobody in particular
Requestors: evdb [...] ecclestoad.co.uk
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: v3.0.9
Fixed in: (no value)



Subject: Object not stored in objct index when created if PK comes from a sequence.
This is from a mail I sent to the CDBI mailing list yesterday. Hello, a while back I posted a test script (attached) that showed up a bug in the object index that means that a new row whose PK comes from a sequence is not added to the index correctly, although this may only affect users of postgres. In the thread that followed a solution came to light that fixes this problem in 3.0.12: # Line 578 or so of Class/DBI.pm 3.0.14 # now that we have a complete primary key, add this to the # object index if ( $Weaken_Is_Available ) { $self = $class->_init( $self ); } Could this fix be added to the next release. Currently it very difficult to work with CDBI and new objects, especially in tests where you create one, change it and then find that it does not work. The previous discussion is archived here: http://lists.digitalcraftsmen.net/pipermail/classdbi/2005-November/000514.html Cheers, Edmund.
Subject: class-dbi-sequence-object-index-bug.t
# This is a test to show unexpected behaviour with relation to 'only # one object in memory'. If a row is added to the database and the # value of it's primary key comes from a sequence then it is not # fetched properly from the object cache. The same behaviour is not # shown if a value is explicitly given for the primary key. I think # this is the problem anyway... # The tests below assume that there is a postgres database 'cdbi_test' # - SQLite does not have sequences and so I don't think that these # tests can be run against it. # Please let me know if there is anything else that I can do to help # you withthis bug ( at least I think it is a bug ). # Edmund von der Burg # evdb@ecclestoad.co.uk # http://ecclestoad.co.uk # see: # http://lists.digitalcraftsmen.net/pipermail/classdbi/2005-November/000514.html # for more details. use strict; use warnings; use Data::Dumper; use Test::More 'no_plan'; ############################################################################ package CDBI; use base 'Class::DBI'; CDBI->connection("dbi:Pg:dbname=cdbi_test"); CDBI->sequence('cdbi_id_seq'); ############################################################################ package Parent; use base 'CDBI'; use Data::Dumper; Parent->table('parents'); Parent->columns( Primary => qw(cdbi_id) ); Parent->columns( Others => qw(name) ); Parent->has_many( children => 'Child' ); sub create_sql { '' } ############################################################################ package Child; use base 'CDBI'; Child->table('children'); Child->columns( Primary => qw/cdbi_id/ ); Child->columns( Others => qw/parent/ ); Child->has_a( parent => 'Parent' ); sub create_sql { '' } ############################################################################ package main; diag "\$Class::DBI::VERSION: '" . $Class::DBI::VERSION . "'"; create_tables(); for my $cdbi_id ( 1_000_000, undef ) { diag ""; diag "using cdbi_id: ", defined $cdbi_id ? $cdbi_id : 'undef'; diag ""; # create a test parent. my $parent = Parent->create( { cdbi_id => $cdbi_id, name => 'test parent' } ); ok $parent, 'create test parent'; # create a test child. my $child = Child->create( { parent => $parent, } ); ok $child, 'create test child'; # do a test to see if an update to the parent is reflected in the # child's parent. is $parent->name, $child->parent->name, "names the same"; ok $parent->name('new name'), "change name"; is $parent->name, $child->parent->name, "names the same"; ok $parent->update, "update"; is $parent->name, $child->parent->name, "names the same"; ok $parent->dbi_commit, "dbi_commit"; is( $parent->name, $child->parent->name, "names the same" ) || diag Dumper { '$parent' => $parent, '$child->parent' => $child->parent }; # Cleanup. $parent->delete; Parent->dbi_commit; } delete_tables(); CDBI->dbi_commit; ############################################################################ sub create_tables { my $dbh = CDBI->db_Main; $SIG{__WARN__} = sub { warn @_ unless $_[0] =~ m/^NOTICE/ }; # diag "Creating tables and sequence"; $dbh->do( <<"" ); create sequence cdbi_id_seq $dbh->do( <<"" ); create table parents ( cdbi_id int8 primary key, name text not null ) $dbh->do( <<"" ); create table children ( cdbi_id int8 primary key, parent int8 not null references parents ) } sub delete_tables { my $dbh = CDBI->db_Main; # diag "Destroying tables and sequence"; $dbh->do("drop table $_") for qw( children parents ); $dbh->do("drop sequence cdbi_id_seq"); }
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Wed, 22 Feb 2006 15:58:39 +0000
To: Guest via RT <bug-Class-DBI [...] rt.cpan.org>
From: Tony Bowden <tony [...] tmtm.com>
On Wed, Feb 22, 2006 at 10:36:30AM -0500, Guest via RT wrote: Show quoted text
> Could this fix be added to the next release.
Do you have a failing test case? If you do, I can put out a fix pretty quickly. If not you'll have to wait until I get a chance to make one up. Thanks, Tony
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Wed, 22 Feb 2006 16:27:13 +0000
To: bug-Class-DBI [...] rt.cpan.org
From: "Edmund von der Burg" <edmund.vonderburg [...] gmail.com>
The test script that I sent along fails on my system when the PK comes from the sequence. I am running 3.0.14 with Postgres 8.1. I'm not sure how best to create a test script that can be packaged and released though as it requires a Postgres database to run. Perhaps the test script I sent along should be tweaked to make it look for an environment variable (eg 'CDBI_TEST_POSTGRES_DSN' )and skip the tests if it is not found. As the test script (should) clean up after itself it could be run against an existing database without prompting the user but that will probably cause lots of pain. Cheers, Edmund. On 22/02/06, tony@tmtm.com via RT <bug-Class-DBI@rt.cpan.org> wrote: Show quoted text
> Do you have a failing test case? > > If you do, I can put out a fix pretty quickly. If not you'll have to > wait until I get a chance to make one up.
-- In reality I'm evdb@ecclestoad.co.uk - http://ecclestoad.co.uk
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Wed, 22 Feb 2006 17:31:30 +0000
To: "evdb [...] ecclestoad.co.uk via RT" <bug-Class-DBI [...] rt.cpan.org>
From: Tony Bowden <tony [...] tmtm.com>
On Wed, Feb 22, 2006 at 11:28:07AM -0500, evdb@ecclestoad.co.uk via RT wrote: Show quoted text
> I'm not sure how best to create a test script that can be packaged and > released though as it requires a Postgres database to run. Perhaps the > test script I sent along should be tweaked to make it look for an > environment variable (eg 'CDBI_TEST_POSTGRES_DSN' )and skip the tests > if it is not found.
Sorry - I missed the attachment as RT.cpan doesn't forward those on. There's already a Pg test in the CDBI distribution. Perhaps you could add this into that? Tony
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Thu, 23 Feb 2006 09:16:05 +0000
To: bug-Class-DBI [...] rt.cpan.org
From: "Edmund von der Burg" <edmund.vonderburg [...] gmail.com>
I did not realize that there was a Pg test script. I'll take the tests that I have and see if I can add them to that. Cheers, Edmund. On 22/02/06, tony@tmtm.com via RT <bug-Class-DBI@rt.cpan.org> wrote: Show quoted text
> On Wed, Feb 22, 2006 at 11:28:07AM -0500, evdb@ecclestoad.co.uk via RT wrote:
> > I'm not sure how best to create a test script that can be packaged and > > released though as it requires a Postgres database to run. Perhaps the > > test script I sent along should be tweaked to make it look for an > > environment variable (eg 'CDBI_TEST_POSTGRES_DSN' )and skip the tests > > if it is not found.
> > > Sorry - I missed the attachment as RT.cpan doesn't forward those on. > > There's already a Pg test in the CDBI distribution. Perhaps you could > add this into that? > > Tony > > >
-- In reality I'm evdb@ecclestoad.co.uk - http://ecclestoad.co.uk
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Thu, 23 Feb 2006 16:55:44 +0000
To: bug-Class-DBI [...] rt.cpan.org
From: "Edmund von der Burg" <edmund.vonderburg [...] gmail.com>
Show quoted text
> > There's already a Pg test in the CDBI distribution. Perhaps you could > > add this into that?
I've created a new test file '27-pg_sequences.t' but it is based on the 'PgBase' that you use in Binary.pm so it should behave : ) Class::DBI fails the test, but passes after the code suggested in the test is added. Thanks for looking at this so quickly, anything else I can do give me a shout. Cheers, Edmund. -- In reality I'm evdb@ecclestoad.co.uk - http://ecclestoad.co.uk

Message body is not shown because sender requested not to inline it.

Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Fri, 3 Mar 2006 08:49:07 +0000
To: "evdb [...] ecclestoad.co.uk via RT" <bug-Class-DBI [...] rt.cpan.org>
From: Tony Bowden <tony [...] kasei.com>
On Thu, Feb 23, 2006 at 11:56:44AM -0500, evdb@ecclestoad.co.uk via RT wrote: Show quoted text
> Thanks for looking at this so quickly, anything else I can do give me a shout.
Just as an update- I haven't forgotten about this. I'm hoping to use the opportunity to tidy up the entire object index code and move it out into another class which can be switched out for an alternative / subclassed etc. This means it will take slightly longer, but I will try to ensure that there is a fix in the new release, even if I just have to fall back on yours for the time being. Thanks, Tony
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Fri, 3 Mar 2006 09:21:36 +0000
To: bug-Class-DBI [...] rt.cpan.org
From: "Edmund von der Burg" <edmund.vonderburg [...] gmail.com>
No problems - I have a hack in place on my local system. Refactoring the object index sounds like a good idea. I was thinking a while back that it might be interesting to put the index into something like MMap so that it is accessible from several processes, although I decided that that was probably a dangerous thing to consider : ) Whilst you're hitting the code one thing that would be really useful (for me at least) would be able to do something like this: my $film = Film->insert({...}); # Update film over web interface ... # check that the film has been updated. $film->refresh; is $film->title, 'Jaws 19', '...'; Quite often I change the values in the database using some other process (eg updating over web) and then want to refresh the values in my local object. At the moment I tend to use: $film = $film->retrieve( $film->id ); which is clunky. Something like $film->refresh would be very useful. I really appreciate that you are maintaining CDBI. Cheers, Edmund. On 03/03/06, Tony Bowden via RT <bug-Class-DBI@rt.cpan.org> wrote: Show quoted text
> On Thu, Feb 23, 2006 at 11:56:44AM -0500, evdb@ecclestoad.co.uk via RT wrote:
> > Thanks for looking at this so quickly, anything else I can do give me a shout.
> > Just as an update- I haven't forgotten about this. I'm hoping to use the > opportunity to tidy up the entire object index code and move it out into > another class which can be switched out for an alternative / subclassed > etc. > > This means it will take slightly longer, but I will try to ensure that > there is a fix in the new release, even if I just have to fall back on > yours for the time being. > > Thanks, > > Tony > >
-- In reality I'm evdb@ecclestoad.co.uk - http://ecclestoad.co.uk
From: "Ryan Gerry" <gerryster [...] gmail.com>
I also independently discovered this issue with the object index and creation without a primary key. I am using MySQL with a table that has an auto incrementing column. I was about to submit the bug when I found that the issue had already been submitted! I agree that separating out the object indexing code into another class would be a good idea. I also wanted to let you know that this object indexing idea has been documented as a design pattern in Martin Fowler's book "Patterns of Enterprise Application Architecture". Fowler calls this an "Identity Map". Here is a link to the synopsis of this pattern, but the full text is available in the book: http://www.martinfowler.com/eaaCatalog/identityMap.html I hope this link can provide you with inspiration as you proceed with the refactoring. The challenges with this pattern include concurrency issues. However, if the identity map only concerns itself with the uniqueness of object in memory of the current process or thread then these issues should be mitigated. From my experience, Ruby On Rail's ActiveRecord is missing this feature. On Fri Mar 03 03:49:20 2006, tony@kasei.com wrote: Show quoted text
> On Thu, Feb 23, 2006 at 11:56:44AM -0500, evdb@ecclestoad.co.uk via RT > wrote:
> > Thanks for looking at this so quickly, anything else I can do give
> me a shout. > > Just as an update- I haven't forgotten about this. I'm hoping to use > the > opportunity to tidy up the entire object index code and move it out > into > another class which can be switched out for an alternative / > subclassed > etc. > > This means it will take slightly longer, but I will try to ensure that > there is a fix in the new release, even if I just have to fall back on > yours for the time being. > > Thanks, > > Tony
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Tue, 14 Mar 2006 06:45:30 +0000
To: Guest via RT <bug-Class-DBI [...] rt.cpan.org>
From: Tony Bowden <tony [...] kasei.com>
On Mon, Mar 13, 2006 at 05:50:16PM -0500, Guest via RT wrote: Show quoted text
> I also independently discovered this issue with the object index and > creation without a primary key. I am using MySQL with a table that has > an auto incrementing column. I was about to submit the bug when I found > that the issue had already been submitted!
A test case for that would be useful if you could put one together. However, this one surprises me as I believed the code coped with that. Is this still present in the latest CPAN release? Thanks, Tony
Subject: Object not stored in object index when created if PK comes from a sequence.
From: "Ryan Gerry" <gerryster [...] gmail.com>
Show quoted text
> > A test case for that would be useful if you could put one together. > However, this one surprises me as I believed the code coped with that. > Is this still present in the latest CPAN release? >
Attached is a test case which demonstrates the issue for MySQL. MySQL differs from Postgres in that it uses auto_increment columsn instead of sequences, but the basic idea is that same. The meat of the script is in the line: is(refaddr($obj1), refaddr($obj2), 'objects 1 and 2 are equivalent'); However, I provided an update case to exemplify the problem. I tested with Class-DBI-v3.0.14 (latest CPAN release). Here are my test results: perl ./class-dbi-sequence-object-index-bug-mysql.t 1..18 ok 1 - correct name ok 2 - correct value ok 3 - objects 1 and 2 are equivilent ok 4 - same field: id = 1 ok 5 - same field: name = foo ok 6 - same field: value = bar ok 7 - same field: id = 1 ok 8 - same field: name = foo ok 9 - same field: value = baz ok 10 - correct name ok 11 - correct value not ok 12 - objects 1 and 2 are equivilant # Failed test 'objects 1 and 2 are equivilant' # in ./class-dbi-sequence-object-index-bug-mysql.t at line 68. # got: '9292480' # expected: '9293092' ok 13 - same field: id = 2 ok 14 - same field: name = foo ok 15 - same field: value = bar ok 16 - same field: id = 2 ok 17 - same field: name = foo not ok 18 - same field: value = bar # Failed test 'same field: value = bar' # in ./class-dbi-sequence-object-index-bug-mysql.t at line 85. # got: 'baz' # expected: 'bar' # Looks like you failed 2 tests of 18.
# Test case for http://rt.cpan.org/Public/Bug/Display.html?id=17805 and MySQL. # Tested with MySQL 4.1.14-max-log. However, the MySQL version should not be # very important since no advanced functionality is used. # # Connection information should be supplied using the DBI_DSN, DBI_USER, and # DBI_PASS environment variables. # # The basic problem is that insert could add the object to the object cache # when the object's table contains an auto incrementing primary key. This is # because this information is avialable when the row is inserted. use warnings; use strict; use Test::More tests => 18; use Class::DBI; use Scalar::Util qw(refaddr); # used to find the memory location of CDBI objects my $TABLE = 'test_CDBI'; #:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::. package CDBI; use base 'Class::DBI'; __PACKAGE__->table($TABLE); __PACKAGE__->columns(Primary => qw/id/); __PACKAGE__->columns(Essential => qw/name value/); # use environment variables $DBI_DSN, $DBI_USER, and $DBI_PASS if avialable __PACKAGE__->connection($ENV{DBI_DSN} || 'DBI:mysql:database=test', $ENV{DBI_USER} || 'root', $ENV{DBI_PASS}); package main; #:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::. sub startup { # create a table for the test class my $dbh = CDBI->db_Main; $dbh->do( <<"" ); CREATE TEMPORARY TABLE $TABLE ( id int(11) NOT NULL auto_increment PRIMARY KEY, name varchar(50), value varchar(50) ) } startup(); # NOTE: unless the unique key is passed to create (id => 1 in this case), # the identity map will not work and retrieving this object will result in # this object being loaded into memory twice # # The first value of $attr represents the passing case, the second is the # failing case. foreach my $attr ({ id => 1, name => 'foo', value => 'bar'}, { name => 'foo', value => 'bar'} ) { my $obj1 = CDBI->insert($attr); is($obj1->name, 'foo', 'correct name'); is($obj1->value, 'bar', 'correct value'); # retreive the object - expect a reference to $obj1 to be returned my $obj2 = CDBI->retrieve( $obj1->id ); # the two objects should be equivilant in memory # refaddr is helpful for getting the memory address since simply Class::DBI # overloads every operation. is(refaddr($obj1), refaddr($obj2), 'objects 1 and 2 are equivalent'); # test all of the common fields: check_fields($obj1, $obj2, qw/id name value/); # If the two objects are equal in memroy, updating one should update both. # This is not the case when the unique id is not passed to insert $obj2->value('baz'); $obj2->update; check_fields($obj1, $obj2, qw/id name value/); } #:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::. sub check_fields { my ($obj1, $obj2, @fields) = @_; foreach my $field (@fields) { is($obj2->$field, $obj1->$field, "same field: $field = " . $obj1->$field); } }
This isn't as simple as it appeared. It causes spurious warnings with t/11 and t/22. 22 seems to be related to attributes being set in before create triggers. I'm not sure what 11 is yet. I'll have another look later, but it means I couldn't put this in the latest release. Tony
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Sun, 20 Aug 2006 11:41:34 +0100
To: bug-Class-DBI [...] rt.cpan.org
From: "Edmund von der Burg" <evdb [...] ecclestoad.co.uk>
Sorry that it is causing trouble : ) Once i made the changes that I suggested it worked for me - but I probably didn't exercise the code as thoroughly as you are. If there is anything that I can do to help please let me know. Cheers, Edmund On 20/08/06, via RT <bug-Class-DBI@rt.cpan.org> wrote: Show quoted text
> > <URL: http://rt.cpan.org/Ticket/Display.html?id=17805 > > > This isn't as simple as it appeared. It causes spurious warnings with > t/11 and t/22. 22 seems to be related to attributes being set in before > create triggers. I'm not sure what 11 is yet. > > I'll have another look later, but it means I couldn't put this in the > latest release. > > Tony > >
-- In reality I'm evdb@ecclestoad.co.uk - http://ecclestoad.co.uk
Subject: Re: [rt.cpan.org #17805] Object not stored in objct index when created if PK comes from a sequence.
Date: Sun, 20 Aug 2006 12:12:36 +0100
To: "evdb [...] ecclestoad.co.uk via RT" <bug-Class-DBI [...] rt.cpan.org>
From: Tony Bowden <tony [...] kasei.com>
On Sun, Aug 20, 2006 at 06:41:48AM -0400, evdb@ecclestoad.co.uk via RT wrote: Show quoted text
> Sorry that it is causing trouble : )
Well, the original problem still needs fixed, so I wouldn't worry too much! :) Show quoted text
> Once i made the changes that I suggested it worked for me - but I > probably didn't exercise the code as thoroughly as you are. If there > is anything that I can do to help please let me know.
Well, if you fancy trying some debugging, just apply the patch and run the existing test suite and see if you can spot why those two tests are now issuing warnings. Each are warning that an object is going out of scope with unsaved changes, so there's something very fishy going on. I suspect this is a sign of a deeper problem somewhere that would bite people hard if we left it in. But I can't see at a glance what it is, and I need to leave now. Otherwise I'll try to have a deeper look tomorrow. Thanks, Tony
From: roman.daniel [...] gtsnovera.cz
As far as I can tell, the bug is still present (3.0.17). It occurs whenever create is called without primary key set - i.e. primary key is either autoincrement column or comes from sequence. I attach simple test reproducing the bug on SQLite database. I don't have a deep understanding of CDBI code structure but by looking at: sub _insert { my ($proto, $data) = @_; my $class = ref $proto || $proto; my $self = $class->_init($data); $self->call_trigger('before_create'); $self->call_trigger('deflate_for_create'); $self->_prepopulate_id if $self->_undefined_primary; ... } I think that after id is prepopulated, the Live_Objects cache is not updated - only access to the cache is within _init call. On Sun Aug 20 07:14:15 2006, tony@kasei.com wrote: Show quoted text
> On Sun, Aug 20, 2006 at 06:41:48AM -0400, evdb@ecclestoad.co.uk via RT > wrote:
> > Sorry that it is causing trouble : )
> > Well, the original problem still needs fixed, so I wouldn't worry too > much! :) >
> > Once i made the changes that I suggested it worked for me - but I > > probably didn't exercise the code as thoroughly as you are. If there > > is anything that I can do to help please let me know.
> > Well, if you fancy trying some debugging, just apply the patch and run > the existing test suite and see if you can spot why those two tests > are > now issuing warnings. Each are warning that an object is going out of > scope with unsaved changes, so there's something very fishy going on. > I > suspect this is a sign of a deeper problem somewhere that would bite > people hard if we left it in. But I can't see at a glance what it is, > and I need to leave now. > > Otherwise I'll try to have a deeper look tomorrow. > > Thanks, > > Tony
use strict; use File::Temp; use DBI; use Scalar::Util qw(refaddr); use Test::More 'tests' => 2; my $db_fh = File::Temp->new; my $dbfile = $db_fh->filename; my $dbh = DBI->connect( "dbi:SQLite:dbname=$dbfile", '', '', { 'RaiseError' => 1 } ); $dbh->do(<<"END_CREATE_TABLE"); create table simplest ( id integer primary key autoincrement, other_column integer ); END_CREATE_TABLE { package Simplest; use base qw(Class::DBI); __PACKAGE__->table('simplest'); __PACKAGE__->columns( 'Primary' => qw(id) ); __PACKAGE__->columns( 'Essential' => qw(other_column) ); sub db_Main { return $dbh } } my $r_created = Simplest->create( { 'other_column' => 208 } ); my $r_retrieved1 = Simplest->retrieve( $r_created->id ); my $r_retrieved2 = Simplest->retrieve( $r_created->id ); cmp_ok( refaddr($r_created), '==', refaddr($r_retrieved1), 'Retrieve returns created object' ); cmp_ok( refaddr($r_retrieved1), '==', refaddr($r_retrieved2), 'Retrieved objects are same' ); # vim: expandtab:shiftwidth=4:tabstop=4:softtabstop=0:textwidth=78: ~ ~