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]
MIME-Version: 1.0
X-Mailer: MIME-tools 5.418 (Entity 5.418)
X-RT-Original-Encoding: utf-8
Content-Type: multipart/mixed; boundary="----------=_1187362986-9104-13"
Content-Length: 0
Content-Type: text/plain; charset="utf8"
Content-Disposition: inline
Content-Transfer-Encoding: binary
Content-Length: 944
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
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----------=_1187362985-9104-12"
X-Mailer: MIME-tools 5.418 (Entity 5.418)
Content-Length: 0
Content-Type: text/plain; charset="utf8"
Content-Disposition: inline
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 0
Content-Type: text/x-patch; name="patch.diff"
Content-Disposition: inline; filename="patch.diff"
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: ascii
Content-Length: 1048
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
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----------=_1187362888-9082-2"
X-Mailer: MIME-tools 5.418 (Entity 5.418)
Content-Length: 0
Content-Type: text/plain; charset="utf8"
Content-Disposition: inline
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 0
Content-Type: text/troff; name="96_multi_create_mysql.t"
Content-Disposition: inline; filename="96_multi_create_mysql.t"
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: ascii
Content-Length: 4130
#!/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; }
MIME-Version: 1.0
X-Mailer: MIME-tools 5.418 (Entity 5.418)
Content-Disposition: inline
Message-Id: <rt-3.6.HEAD-14648-1187746841-60.28875-0-0 [...] rt.cpan.org>
Content-Type: text/plain; charset="utf8"
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
X-RT-Original-Encoding: utf-8
Content-Length: 1098
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.
MIME-Version: 1.0
In-Reply-To: <rt-3.6.HEAD-14648-1187746841-60.28875-0-0 [...] rt.cpan.org>
X-Mailer: MIME-tools 5.426 (Entity 5.426)
Content-Disposition: inline
Charset: utf8
References: <rt-3.6.HEAD-14648-1187746841-60.28875-0-0 [...] rt.cpan.org>
Message-Id: <rt-3.6.HEAD-27678-1223429622-792.28875-0-0 [...] rt.cpan.org>
Content-Type: text/plain
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 1104
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.
MIME-Version: 1.0
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
Charset: utf8
Content-Type: text/plain
Message-ID: <rt-3.6.HEAD-13950-1244637810-565.28875-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 68


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.