Skip Menu |
 

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

Report information
The Basics
Id: 28875
Status: resolved
Priority: 0/
Queue: DBIx-Class

People
Owner: Nobody in particular
Requestors: willert [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 0.08006
Fixed in: 0.08104



Subject: Bizarre bug in multi-create on MySQL [PATCH/TEST]
Download (untitled) / with headers
text/plain 944b
This took me a whole day to track down: in some circumstances repeated multi-creates on a MySQL storage backend will cause data corruption when the primary key in the related table is not explicitly set (e.g. to undef). I think the main culprit here is find_or_new_related in DBIx::Class::Relationship::Base. It calls find_related with not strictly illegal but at least useless arguments, maybe triggering a bug further down the stack (I still wonder why this doesn't happen with SQLite). Changing find_or_new_related to just pass on values that are either unique or primary keys resp. not calling find_related at all if no keys were given seems to fix this bug without altering the intended behavior. I've attached a testcase that demonstrates the bug and the fix and a patch to the current trunk that can be applied if there are no concerns about the fix (the testsuite still works after applying the patch). Cheers, Sebastian Willert
Subject: patch.diff
Download patch.diff
text/x-diff 1k
Index: lib/DBIx/Class/Relationship/Base.pm =================================================================== --- lib/DBIx/Class/Relationship/Base.pm (revision 3684) +++ lib/DBIx/Class/Relationship/Base.pm (working copy) @@ -288,7 +288,27 @@ sub find_or_new_related { my $self = shift; - return $self->find_related(@_) || $self->new_related(@_); + my $rel = shift; + my $attrs = shift; + + $self->throw_exception( "find_or_new_related needs a hashref" ) + unless ref $attrs eq 'HASH'; + + my $rel_src = $self->result_source->related_source($rel); + my @related_keys = map{ $rel_src->unique_constraint_columns($_) } + $rel_src->unique_constraint_names; + + # Build primary and unique key conditions for find_related + my %keyed_attrs; + for my $key ( @related_keys ) { + $keyed_attrs{$key} = $attrs->{$key} if exists $attrs->{$key}; + } + + my $new; + $new = $self->find_related( $rel, \%keyed_attrs ) if %keyed_attrs; + $new ||= $self->new_related( $rel, $attrs ); + + return $new; } =head2 find_or_create_related
Subject: 96_multi_create_mysql.t
#!/usr/bin/perl -w use strict; use warnings; package MCTest::Schema::Related; use base qw/DBIx::Class::Core/; __PACKAGE__->table('multi_create_mysql_related'); __PACKAGE__->add_columns( id => { data_type => "integer", is_auto_increment => 1, size => 11 }, ); __PACKAGE__->set_primary_key(qw/id/); package MCTest::Schema::Main; use base qw/DBIx::Class::Core/; __PACKAGE__->table( 'multi_create_mysql_main' ); __PACKAGE__->add_columns( id => { data_type => "integer", is_auto_increment => 1, size => 11 }, related => { data_type => "integer", size => 11 }, ); __PACKAGE__->set_primary_key( 'id' ); __PACKAGE__->belongs_to( related => q/MCTest::Schema::Related/ => undef, { cascade_delete => 1 } ); package MCTest::Schema; use base qw/DBIx::Class::Schema/; __PACKAGE__->load_classes(qw/Related Main/); package main; use Test::More; my ($dsn, $user, $pass) = @ENV{map { "DBICTEST_MYSQL_${_}" } qw/DSN USER PASS/}; plan skip_all=> 'Set $ENV{DBICTEST_MYSQL_DSN}, _USER and _PASS to run this test' unless $dsn && $user; plan tests => 10; sub init_schema { my $schema = MCTest::Schema->connect( $dsn, $user, $pass, { AutoCommit => 1 } ); $schema->deploy({ add_drop_table => 1 }); return $schema; } sub fix_multi_create { no strict 'refs'; no warnings 'redefine'; *{'DBIx::Class::Relationship::Base::find_or_new_related'} = sub{ my $self = shift; my $rel = shift; my $attrs = shift; $self->throw_exception( "find_or_new_related needs a hashref" ) unless ref $attrs eq 'HASH'; my $rel_src = $self->result_source->related_source($rel); my @related_keys = map{ $rel_src->unique_constraint_columns($_) } $rel_src->unique_constraint_names; # Build primary and unique key conditions for find_related my %keyed_attrs; for my $key ( @related_keys ) { $keyed_attrs{$key} = $attrs->{$key} if exists $attrs->{$key}; } my $new; $new = $self->find_related( $rel, \%keyed_attrs ) if %keyed_attrs; $new ||= $self->new_related( $rel, $attrs ); return $new; }; Class::C3::reinitialize(); } sub clear_table_content { # delete_all doesn't work with corrupted tables so we have # to use desperate measures to get rid of the data my $schema = shift; $schema->storage->dbh->do('truncate multi_create_mysql_main'); $schema->storage->dbh->do('truncate multi_create_mysql_related'); } my $schema = init_schema(); my ( $tpl, $cnt ) = map{ $schema->resultset($_) } qw/Main Related/; my $rel; $tpl->create({ related => {} }); my $main = $tpl->first; is( $main->id, 1, 'Single multi create (main)' ); is( $main->related->id, 1, 'Single multi create (related)' ); $tpl->create({ related => {} }); $main = $tpl->find(2); is( $main->id, 2, 'Double multi create (main)' ); is( $main->related->id, 2, 'Double multi create (related)' ); clear_table_content( $schema ); $tpl->create({ related => {} }) for 1 .. 4; is( $cnt->count, 4, 'Multi create in a loop' ); clear_table_content( $schema ); $tpl->create({ related => { id =>undef } }) for 1 .. 4; is( $cnt->count, 4, 'Multi create with explicit id' ); clear_table_content( $schema ); $rel = $cnt->create({}); $tpl->create({ related => $rel->id }); $tpl->create({ related => {} }) for 1 .. 4; is( $cnt->count, 5, 'Multi create with one exisitng entry' ); clear_table_content( $schema ); $rel = $cnt->create({}); $tpl->create({ related => $rel->id }); $rel = $cnt->create({}); $tpl->create({ related => $rel->id }); $tpl->create({ related => {} }) for 1 .. 4; is( $cnt->count, 6, 'Multi create with two exisitng entries' ); fix_multi_create(); clear_table_content( $schema ); $tpl->create({ related => {} }) for 1 .. 4; is( $cnt->count, 4, 'Multi create using fixed find_or_new_related' ); clear_table_content( $schema ); $tpl->create({ related => {} }) for 1 .. 4; $tpl->create({ related => $cnt->first->id }); is( $cnt->count, 4, 'Multi create with retrival of an preexisting row' ); # clean up our mess END { my $dbh = $schema->storage->dbh; $dbh->do("DROP TABLE multi_create_mysql_main") if $dbh; $dbh->do("DROP TABLE multi_create_mysql_related") if $dbh; }
On Fri Aug 17 11:03:19 2007, WILLERT wrote: Show quoted text
> This took me a whole day to track down: in some circumstances repeated > multi-creates on a MySQL storage backend will cause data corruption > when the primary key in the related table is not explicitly set > (e.g. to undef). > > I think the main culprit here is find_or_new_related in > DBIx::Class::Relationship::Base. It calls find_related with > not strictly illegal but at least useless arguments, maybe triggering > a bug further down the stack (I still wonder why this doesn't happen > with SQLite). Changing find_or_new_related to just pass on values > that are either unique or primary keys resp. not calling find_related > at all if no keys were given seems to fix this bug without altering > the intended behavior.
Your intent is good but the implementation is naff. A better approach would be to change it to call ->related_resultset($rel)->find_or_new and to look at fixing this at that level. I don't want to end up dumping specialist logic in Relationship::Base, it's intended to be a thin wrapper round related_resultset calls only.
This is a genuine bug, and is completely unrelated to MySQL. The fix is also very simple. What happens is we end up calling find({}, $attrs) which returns the first row available if any (I'd think it should actually die() on such invocations). In any case the fix for this lies in find_or_new(): Index: lib/DBIx/Class/ResultSet.pm =================================================================== --- lib/DBIx/Class/ResultSet.pm (revision 4908) +++ lib/DBIx/Class/ResultSet.pm (working copy) @@ -1682,8 +1682,10 @@ my $self = shift; my $attrs = (@_ > 1 && ref $_[$#_] eq 'HASH' ? pop(@_) : {}); my $hash = ref $_[0] eq 'HASH' ? shift : {@_}; - my $exists = $self->find($hash, $attrs); - return defined $exists ? $exists : $self->new_result($hash); + if (keys %$hash and my $row = $self->find($hash, $attrs) ) { + return $row; + } + return $self->new_result($hash); } =head2 create I have a full patch incorporating the supplied test (with some grooming) and some additional of my own. Testing however uncovered another bug, will discuss on IRC before going further.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.