From: Peter Rabbitson Date: Tue, 13 Sep 2016 04:41:16 +0000 (+0200) Subject: Fix silent failures on autoinc PK without an is_auto_increment attribute X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=dbsrgits%2FDBIx-Class.git;a=commitdiff_plain;h=7305f6f933813eaa1a4a7b65bfc5f158d0d65c4d Fix silent failures on autoinc PK without an is_auto_increment attribute Back in fabbd5cc the refactor of the subsystem left out this corner case, resulting in drivers without a functioning last_insert_id() to either fail to retrieve the autoinc value, or in case of the suboptimal ->_identity()-based implementations ( MSSQL / Sybase ) to *reuse* the last successfully retrieved value (sigh) Since the entire codepath is a hot mess, not changing much aside from an extra pass to enable implicit inferrence of an is_auto_increment setting. Several different codepaths in the test suite were used to ensure everything meshes correctly with retrieve_on_insert and similar --- diff --git a/Changes b/Changes index e402e5a..e40b03e 100644 --- a/Changes +++ b/Changes @@ -54,6 +54,9 @@ Revision history for DBIx::Class result source metadata (RT#107462) - Fix incorrect SQL generated with invalid {rows} on complex resultset operations, generally more robust handling of rows/offset attrs + - Fix silent failure to retrieve a primary key (RT#80283) or worse: + returning an incorrect value (RT#115381) in case a rdbms-side autoinc + column is declared as PK with the is_auto_increment attribute unset - Fix incorrect $storage state on unexpected RDBMS disconnects and other failure events, preventing clean reconnection (RT#110429) - Make sure exception objects stringifying to '' are properly handled diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 9da3bd9..1a9d792 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -2001,19 +2001,43 @@ sub insert { # they can be fused once again with the final return $to_insert = { %$to_insert, %$prefetched_values }; - # FIXME - we seem to assume undef values as non-supplied. This is wrong. - # Investigate what does it take to s/defined/exists/ my %pcols = map { $_ => 1 } $source->primary_columns; + my (%retrieve_cols, $autoinc_supplied, $retrieve_autoinc_col); + for my $col ($source->columns) { + + # first autoinc wins - this is why ->columns() in-order iteration is important + # + # FIXME - there ought to be a sanity-check for multiple is_auto_increment settings + # or something... + # if ($col_infos->{$col}{is_auto_increment}) { + + # FIXME - we seem to assume undef values as non-supplied. + # This is wrong. + # Investigate what does it take to s/defined/exists/ + # ( fails t/cdbi/copy.t amoong other things ) $autoinc_supplied ||= 1 if defined $to_insert->{$col}; + $retrieve_autoinc_col ||= $col unless $autoinc_supplied; } # nothing to retrieve when explicit values are supplied next if ( - defined $to_insert->{$col} and ! is_literal_value($to_insert->{$col}) + # FIXME - we seem to assume undef values as non-supplied. + # This is wrong. + # Investigate what does it take to s/defined/exists/ + # ( fails t/cdbi/copy.t amoong other things ) + defined $to_insert->{$col} + and + ( + # not a ref - cheaper to check before a call to is_literal_value() + ! length ref $to_insert->{$col} + or + # not a literal we *MAY* need to pull out ( see check below ) + ! is_literal_value( $to_insert->{$col} ) + ) ); # the 'scalar keys' is a trick to preserve the ->columns declaration order @@ -2024,6 +2048,35 @@ sub insert { ); }; + # corner case of a non-supplied PK which is *not* declared as autoinc + if ( + ! $autoinc_supplied + and + ! defined $retrieve_autoinc_col + and + # FIXME - first come-first serve, suboptimal... + ($retrieve_autoinc_col) = ( grep + { + $pcols{$_} + and + ! $col_infos->{$_}{retrieve_on_insert} + and + ! defined $col_infos->{$_}{is_auto_increment} + } + sort + { $retrieve_cols{$a} <=> $retrieve_cols{$b} } + keys %retrieve_cols + ) + ) { + carp_unique( + "Missing value for primary key column '$retrieve_autoinc_col' on " + . "@{[ $source->source_name ]} - perhaps you forgot to set its " + . "'is_auto_increment' attribute during add_columns()? Treating " + . "'$retrieve_autoinc_col' implicitly as an autoinc, and attempting " + . 'value retrieval' + ); + } + local $self->{_autoinc_supplied_for_op} = $autoinc_supplied; local $self->{_perform_autoinc_retrieval} = $retrieve_autoinc_col; diff --git a/t/46where_attribute.t b/t/46where_attribute.t index ba1c7d0..0fedbe7 100644 --- a/t/46where_attribute.t +++ b/t/46where_attribute.t @@ -4,6 +4,7 @@ use strict; use warnings; use Test::More; +use Test::Warn; use DBICTest; my $schema = DBICTest->init_schema(); @@ -22,9 +23,12 @@ is($programming_perl->id, 1, 'select from a resultset with find_or_create for ex # and inserts? my $see_spot; -$see_spot = eval { $owner->books->find_or_create({ title => "See Spot Run" }) }; -if ($@) { print $@ } -ok(!$@, 'find_or_create on resultset with attribute for non-existent entry did not throw'); +$see_spot = eval { + warnings_exist { + $owner->books->find_or_create({ title => "See Spot Run" }) + } qr/Missing value for primary key column 'id' on BooksInLibrary - perhaps you forgot to set its 'is_auto_increment'/; +}; +is ($@, '', 'find_or_create on resultset with attribute for non-existent entry did not throw'); ok(defined $see_spot, 'successfully did insert on resultset with attribute for non-existent entry'); my $see_spot_rs = $owner->books->search({ title => "See Spot Run" }); diff --git a/t/72pg.t b/t/72pg.t index 9d37930..1f0cc07 100644 --- a/t/72pg.t +++ b/t/72pg.t @@ -196,6 +196,9 @@ for my $use_insert_returning ($test_server_supports_insert_returning __PACKAGE__->column_info_from_storage(1); __PACKAGE__->set_primary_key('id'); + # FIXME - for some reason column_info_from_storage does not properly find + # the is_auto_increment setting... + __PACKAGE__->column_info('id')->{is_auto_increment} = 1; } SKIP: { skip "Need DBD::Pg 2.9.2 or newer for array tests", 4 if $DBD::Pg::VERSION < 2.009002; diff --git a/t/746mssql.t b/t/746mssql.t index d1b8773..5fc3d30 100644 --- a/t/746mssql.t +++ b/t/746mssql.t @@ -9,7 +9,6 @@ use Test::Exception; use Test::Warn; use Try::Tiny; - use DBICTest; my ($dsn, $user, $pass) = @ENV{map { "DBICTEST_MSSQL_ODBC_${_}" } qw/DSN USER PASS/}; @@ -519,6 +518,38 @@ SQL 'updated money value to NULL round-trip'; } } + +# Test leakage of PK on implicit retrieval + { + + my $next_owner = $schema->resultset('Owners')->get_column('id')->max + 1; + my $next_book = $schema->resultset('BooksInLibrary')->get_column('id')->max + 1; + + cmp_ok( + $next_owner, + '!=', + $next_book, + 'Preexisting auto-inc PKs staggered' + ); + + my $yet_another_owner = $schema->resultset('Owners')->create({ name => 'YAO' }); + my $yet_another_book; + warnings_exist { + $yet_another_book = $yet_another_owner->create_related( books => { title => 'YAB' }) + } qr/Missing value for primary key column 'id' on BooksInLibrary - perhaps you forgot to set its 'is_auto_increment'/; + + is( + $yet_another_owner->id, + $next_owner, + 'Expected Owner id' + ); + + is( + $yet_another_book->id, + $next_book, + 'Expected Book id' + ); + } } } diff --git a/t/cdbi/copy.t b/t/cdbi/copy.t index 2741aad..b122781 100644 --- a/t/cdbi/copy.t +++ b/t/cdbi/copy.t @@ -18,6 +18,11 @@ use lib 't/cdbi/testlib'; __PACKAGE__->set_table('Movies'); __PACKAGE__->columns(All => qw(id title)); + # Disables the implicit autoinc-on-non-supplied-pk behavior + # (and the warning that goes with it) + # This is the same behavior as it was pre 0.082900 + __PACKAGE__->column_info('id')->{is_auto_increment} = 0; + sub create_sql { return qq{ id INTEGER PRIMARY KEY AUTOINCREMENT, diff --git a/t/cdbi/testlib/Actor.pm b/t/cdbi/testlib/Actor.pm index 83a03b9..3bffd09 100644 --- a/t/cdbi/testlib/Actor.pm +++ b/t/cdbi/testlib/Actor.pm @@ -13,6 +13,11 @@ __PACKAGE__->columns(All => qw/ Name Film Salary /); __PACKAGE__->columns(TEMP => qw/ nonpersistent /); __PACKAGE__->add_constructor(salary_between => 'salary >= ? AND salary <= ?'); +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('id')->{is_auto_increment} = 0; + sub mutator_name_for { "set_$_[1]" } sub create_sql { diff --git a/t/cdbi/testlib/ActorAlias.pm b/t/cdbi/testlib/ActorAlias.pm index 862a410..5fb9456 100644 --- a/t/cdbi/testlib/ActorAlias.pm +++ b/t/cdbi/testlib/ActorAlias.pm @@ -13,6 +13,11 @@ __PACKAGE__->columns( All => qw/ actor alias / ); __PACKAGE__->has_a( actor => 'Actor' ); __PACKAGE__->has_a( alias => 'Actor' ); +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('id')->{is_auto_increment} = 0; + sub create_sql { return qq{ id INTEGER PRIMARY KEY, diff --git a/t/cdbi/testlib/ColumnObject.pm b/t/cdbi/testlib/ColumnObject.pm index 11eeb89..0811367 100644 --- a/t/cdbi/testlib/ColumnObject.pm +++ b/t/cdbi/testlib/ColumnObject.pm @@ -18,6 +18,11 @@ __PACKAGE__->columns( All => ( Class::DBI::Column->new('columnb' => {mutator => 'columnb_as_write'}), )); +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('id')->{is_auto_increment} = 0; + sub create_sql { return qq{ id INTEGER PRIMARY KEY, diff --git a/t/cdbi/testlib/Film.pm b/t/cdbi/testlib/Film.pm index 3bbd755..5c43f5a 100644 --- a/t/cdbi/testlib/Film.pm +++ b/t/cdbi/testlib/Film.pm @@ -12,6 +12,11 @@ __PACKAGE__->columns('Essential', qw( Title )); __PACKAGE__->columns('Directors', qw( Director CoDirector )); __PACKAGE__->columns('Other', qw( Rating NumExplodingSheep HasVomit )); +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('title')->{is_auto_increment} = 0; + sub create_sql { return qq{ title VARCHAR(255), diff --git a/t/cdbi/testlib/ImplicitInflate.pm b/t/cdbi/testlib/ImplicitInflate.pm index 610e835..14b2bf8 100644 --- a/t/cdbi/testlib/ImplicitInflate.pm +++ b/t/cdbi/testlib/ImplicitInflate.pm @@ -19,6 +19,12 @@ __PACKAGE__->has_a( update_datetime => 'MyDateStamp', ); + +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('id')->{is_auto_increment} = 0; + sub create_sql { # SQLite doesn't support Datetime datatypes. return qq{ diff --git a/t/cdbi/testlib/Log.pm b/t/cdbi/testlib/Log.pm index 362b61e..c17b9bb 100644 --- a/t/cdbi/testlib/Log.pm +++ b/t/cdbi/testlib/Log.pm @@ -17,6 +17,11 @@ __PACKAGE__->has_a( deflate => 'mysql_datetime' ); +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('id')->{is_auto_increment} = 0; + __PACKAGE__->add_trigger(before_create => \&set_dts); __PACKAGE__->add_trigger(before_update => \&set_dts); diff --git a/t/cdbi/testlib/MyFoo.pm b/t/cdbi/testlib/MyFoo.pm index 7df9c6f..fa45d7d 100644 --- a/t/cdbi/testlib/MyFoo.pm +++ b/t/cdbi/testlib/MyFoo.pm @@ -13,6 +13,12 @@ __PACKAGE__->has_a( inflate => sub { Date::Simple->new(shift) }, deflate => 'format', ); + +# Disables the implicit autoinc-on-non-supplied-pk behavior +# (and the warning that goes with it) +# This is the same behavior as it was pre 0.082900 +__PACKAGE__->column_info('myid')->{is_auto_increment} = 0; + #__PACKAGE__->find_column('tdate')->placeholder("IF(1, CURDATE(), ?)"); sub create_sql { diff --git a/t/lib/DBICTest/Schema/BooksInLibrary.pm b/t/lib/DBICTest/Schema/BooksInLibrary.pm index cd6f375..c69ea5d 100644 --- a/t/lib/DBICTest/Schema/BooksInLibrary.pm +++ b/t/lib/DBICTest/Schema/BooksInLibrary.pm @@ -9,8 +9,11 @@ use base qw/DBICTest::BaseResult/; __PACKAGE__->table('books'); __PACKAGE__->add_columns( 'id' => { + # part of a test (auto-retrieval of PK regardless of autoinc status) + # DO NOT define + #is_auto_increment => 1, + data_type => 'integer', - is_auto_increment => 1, }, 'source' => { data_type => 'varchar', diff --git a/t/lib/DBICTest/Schema/TimestampPrimaryKey.pm b/t/lib/DBICTest/Schema/TimestampPrimaryKey.pm index 8ec4cf9..a52f0db 100644 --- a/t/lib/DBICTest/Schema/TimestampPrimaryKey.pm +++ b/t/lib/DBICTest/Schema/TimestampPrimaryKey.pm @@ -12,6 +12,7 @@ __PACKAGE__->add_columns( 'id' => { data_type => 'timestamp', default_value => \'current_timestamp', + retrieve_on_insert => 1, }, ); diff --git a/t/row/inflate_result.t b/t/row/inflate_result.t index 9fa49ac..b650302 100644 --- a/t/row/inflate_result.t +++ b/t/row/inflate_result.t @@ -20,9 +20,8 @@ my $admin_class = __PACKAGE__ . '::Admin'; __PACKAGE__->table('users'); __PACKAGE__->add_columns( - qw/user_id email password - firstname lastname active - admin/ + user_id => { retrieve_on_insert => 1 }, + qw( email password firstname lastname active admin ), ); __PACKAGE__->set_primary_key('user_id');