From: Robert Bohne Date: Thu, 10 Mar 2011 08:19:28 +0000 (+0100) Subject: Add retrieve_on_insert column_info flag, to autoretrieve RDBMS-side defaults X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=8b9473f535c78bab500ff7e1258bd9fcab3e4ff6;p=dbsrgits%2FDBIx-Class-Historic.git Add retrieve_on_insert column_info flag, to autoretrieve RDBMS-side defaults Also move the UniqueIdentifier code from insert() to _prefetch_autovalues() where it makes more logical sense --- diff --git a/Changes b/Changes index 63e49ef..b2a151e 100644 --- a/Changes +++ b/Changes @@ -3,6 +3,8 @@ Revision history for DBIx::Class * New Features / Changes - Add quote_names connection option. When set to true automatically sets quote_char and name_sep appropriate for your RDBMS + - Add retrieve_on_insert column info flag, allowing to retrieve any + column value instead of just autoinc primary keys - All limit dialects (except for the older Top and FetchFirst) are now using bind parameters for the limits/offsets, making DBI's prepare_cached useful across paged resutsets diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm index 0e9d1bd..d7bcd72 100644 --- a/lib/DBIx/Class/ResultSource.pm +++ b/lib/DBIx/Class/ResultSource.pm @@ -253,8 +253,20 @@ generate a new key value. If not specified, L will attempt to retrieve the name of the sequence from the database automatically. +=item retrieve_on_insert + + { retrieve_on_insert => 1 } + +For every column where this is set to true, DBIC will retrieve the RDBMS-side +value upon a new row insertion (normally only the autoincrement PK is +retrieved on insert). C is used automatically if +supported by the underlying storage, otherwise an extra SELECT statement is +executed to retrieve the missing data. + =item auto_nextval + { auto_nextval => 1 } + Set this to a true value for a column whose value is retrieved automatically from a sequence or function (if supported by your Storage driver.) For a sequence, if you do not use a trigger to get the nextval, you have to set the diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm index 239b327..44374f1 100644 --- a/lib/DBIx/Class/Row.pm +++ b/lib/DBIx/Class/Row.pm @@ -343,40 +343,23 @@ sub insert { warn "MC $self inserting (".join(', ', $self->get_columns).")\n"; }; + # perform the insert - the storage will return everything it is asked to + # (autoinc primary columns and any retrieve_on_insert columns) my %current_rowdata = $self->get_columns; - - # perform the insert - the storage may return some stuff for us right there - # my $returned_cols = $storage->insert( $source, - \%current_rowdata, + { %current_rowdata }, # what to insert, copy because the storage *will* change it ); for (keys %$returned_cols) { - $self->store_column( - $_, - ( $current_rowdata{$_} = $returned_cols->{$_} ) - ); - } - - # see if any of the pcols still need filling (or re-querying in case of scalarrefs) - my @missing_pri = grep - { ! defined $current_rowdata{$_} or ref $current_rowdata{$_} eq 'SCALAR' } - $self->primary_columns - ; - - if (@missing_pri) { - MULTICREATE_DEBUG and warn "MC $self fetching missing PKs ".join(', ', @missing_pri )."\n"; - - $self->throw_exception( "Missing primary key but Storage doesn't support last_insert_id" ) - unless $storage->can('last_insert_id'); - - my @pri_values = $storage->last_insert_id($self->result_source, @missing_pri); - - $self->throw_exception( "Can't get last insert id" ) - unless (@pri_values == @missing_pri); - - $self->store_column($missing_pri[$_] => $pri_values[$_]) for 0 .. $#missing_pri; + $self->store_column($_, $returned_cols->{$_}) + # this ensures we fire store_column only once + # (some asshats like overriding it) + if ( + (! defined $current_rowdata{$_}) + or + ( $current_rowdata{$_} ne $returned_cols->{$_}) + ); } $self->{_dirty_columns} = {}; diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index a4eb7c7..1c7ea76 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -1766,6 +1766,8 @@ sub _prefetch_autovalues { ! exists $to_insert->{$col} or ref $to_insert->{$col} eq 'SCALAR' + or + (ref $to_insert->{$col} eq 'REF' and ref ${$to_insert->{$col}} eq 'ARRAY') ) ) { $values{$col} = $self->_sequence_fetch( @@ -1785,33 +1787,43 @@ sub insert { my $prefetched_values = $self->_prefetch_autovalues($source, $to_insert); - # fuse the values + # fuse the values, but keep a separate list of prefetched_values so that + # they can be fused once again with the final return $to_insert = { %$to_insert, %$prefetched_values }; - # list of primary keys we try to fetch from the database - # both not-exsists and scalarrefs are considered - my %fetch_pks; - for ($source->primary_columns) { - $fetch_pks{$_} = scalar keys %fetch_pks # so we can preserve order for prettyness - if ! exists $to_insert->{$_} or ref $to_insert->{$_} eq 'SCALAR'; - } + my $col_infos = $source->columns_info; + my %pcols = map { $_ => 1 } $source->primary_columns; + my %retrieve_cols; + for my $col ($source->columns) { + # nothing to retrieve when explicit values are supplied + next if (defined $to_insert->{$col} and ! ( + ref $to_insert->{$col} eq 'SCALAR' + or + (ref $to_insert->{$col} eq 'REF' and ref ${$to_insert->{$col}} eq 'ARRAY') + )); + + # the 'scalar keys' is a trick to preserve the ->columns declaration order + $retrieve_cols{$col} = scalar keys %retrieve_cols if ( + $pcols{$col} + or + $col_infos->{$col}{retrieve_on_insert} + ); + }; my ($sqla_opts, @ir_container); - if ($self->_use_insert_returning) { + if (%retrieve_cols and $self->_use_insert_returning) { + $sqla_opts->{returning_container} = \@ir_container + if $self->_use_insert_returning_bound; - # retain order as declared in the resultsource - for (sort { $fetch_pks{$a} <=> $fetch_pks{$b} } keys %fetch_pks ) { - push @{$sqla_opts->{returning}}, $_; - $sqla_opts->{returning_container} = \@ir_container - if $self->_use_insert_returning_bound; - } + $sqla_opts->{returning} = [ + sort { $retrieve_cols{$a} <=> $retrieve_cols{$b} } keys %retrieve_cols + ]; } my ($rv, $sth) = $self->_execute('insert', $source, $to_insert, $sqla_opts); - my %returned_cols; - - if (my $retlist = $sqla_opts->{returning}) { + my %returned_cols = %$to_insert; + if (my $retlist = $sqla_opts->{returning}) { # if IR is supported - we will get everything in one set @ir_container = try { local $SIG{__WARN__} = sub {}; my @r = $sth->fetchrow_array; @@ -1821,11 +1833,45 @@ sub insert { @returned_cols{@$retlist} = @ir_container if @ir_container; } + else { + # pull in PK if needed and then everything else + if (my @missing_pri = grep { $pcols{$_} } keys %retrieve_cols) { + + $self->throw_exception( "Missing primary key but Storage doesn't support last_insert_id" ) + unless $self->can('last_insert_id'); + + my @pri_values = $self->last_insert_id($source, @missing_pri); + + $self->throw_exception( "Can't get last insert id" ) + unless (@pri_values == @missing_pri); + + @returned_cols{@missing_pri} = @pri_values; + delete $retrieve_cols{$_} for @missing_pri; + } + + # if there is more left to pull + if (%retrieve_cols) { + $self->throw_exception( + 'Unable to retrieve additional columns without a Primary Key on ' . $source->source_name + ) unless %pcols; + + my @left_to_fetch = sort { $retrieve_cols{$a} <=> $retrieve_cols{$b} } keys %retrieve_cols; + + my $cur = DBIx::Class::ResultSet->new($source, { + where => { map { $_ => $returned_cols{$_} } (keys %pcols) }, + select => \@left_to_fetch, + })->cursor; + + @returned_cols{@left_to_fetch} = $cur->next; + + $self->throw_exception('Duplicate row returned for PK-search after fresh insert') + if scalar $cur->next; + } + } return { %$prefetched_values, %returned_cols }; } - sub insert_bulk { my ($self, $source, $cols, $data) = @_; diff --git a/lib/DBIx/Class/Storage/DBI/UniqueIdentifier.pm b/lib/DBIx/Class/Storage/DBI/UniqueIdentifier.pm index 92e7c15..955529d 100644 --- a/lib/DBIx/Class/Storage/DBI/UniqueIdentifier.pm +++ b/lib/DBIx/Class/Storage/DBI/UniqueIdentifier.pm @@ -56,7 +56,7 @@ sub _is_guid_type { return $data_type =~ $GUID_TYPE; } -sub insert { +sub _prefetch_autovalues { my $self = shift; my ($source, $to_insert) = @_; @@ -64,8 +64,8 @@ sub insert { my %guid_cols; my @pk_cols = $source->primary_columns; - my %pk_cols; - @pk_cols{@pk_cols} = (); + my %pk_col_idx; + @pk_col_idx{@pk_cols} = (); my @pk_guids = grep { $col_info->{$_}{data_type} @@ -79,13 +79,11 @@ sub insert { $col_info->{$_}{data_type} =~ $GUID_TYPE && $col_info->{$_}{auto_nextval} - } grep { not exists $pk_cols{$_} } $source->columns; + } grep { not exists $pk_col_idx{$_} } $source->columns; my @get_guids_for = grep { not exists $to_insert->{$_} } (@pk_guids, @auto_guids); - my $updated_cols = {}; - for my $guid_col (@get_guids_for) { my $new_guid; @@ -99,18 +97,14 @@ sub insert { } if (ref $guid_method eq 'CODE') { - $new_guid = $guid_method->(); + $to_insert->{$guid_col} = $guid_method->(); } else { - ($new_guid) = $self->_get_dbh->selectrow_array("SELECT $guid_method"); + ($to_insert->{$guid_col}) = $self->_get_dbh->selectrow_array("SELECT $guid_method"); } - - $updated_cols->{$guid_col} = $to_insert->{$guid_col} = $new_guid; } - $updated_cols = { %$updated_cols, %{ $self->next::method(@_) } }; - - return $updated_cols; + return $self->next::method(@_); } =head1 AUTHOR diff --git a/t/19retrieve_on_insert.t b/t/19retrieve_on_insert.t new file mode 100644 index 0000000..d258180 --- /dev/null +++ b/t/19retrieve_on_insert.t @@ -0,0 +1,25 @@ +use strict; +use warnings; + +use Test::More; +use Test::Exception; +use lib qw(t/lib); +use DBICTest; + +my $schema = DBICTest->init_schema(); +$schema->storage->sql_maker->quote_char('"'); + +my $rs = $schema->resultset ('Artist'); + +my $obj; +lives_ok { $obj = $rs->create ({ name => 'artistA' }) } 'Default insert successful'; +is ($obj->rank, undef, 'Without retrieve_on_insert, check rank'); + +$rs->result_source->add_columns( + '+rank' => { retrieve_on_insert => 1 } +); + +lives_ok { $obj = $rs->create ({ name => 'artistB' }) } 'Default insert successful'; +is ($obj->rank, 13, 'With retrieve_on_insert, check rank'); + +done_testing; diff --git a/t/73oracle.t b/t/73oracle.t index 84847ae..ca30810 100644 --- a/t/73oracle.t +++ b/t/73oracle.t @@ -49,6 +49,12 @@ plan skip_all => 'Set $ENV{DBICTEST_ORA_DSN}, _USER and _PASS to run this test.' data_type => 'integer', is_auto_increment => 1, }, + 'default_value_col' => { + data_type => 'varchar', + size => 100, + is_nullable => 0, + retrieve_on_insert => 1, + } ); __PACKAGE__->set_primary_key(qw/ artistid autoinc_col /); @@ -172,6 +178,12 @@ sub _run_tests { is( $new->artistid, 3, "Oracle Auto-PK worked with fully-qualified tablename" ); is( $new->autoinc_col, 1000, "Oracle Auto-Inc overruled with fully-qualified tablename"); + + is( $new->default_value_col, 'default_value', $schema->storage->_use_insert_returning + ? 'Check retrieve_on_insert on default_value_col with INSERT ... RETURNING' + : 'Check retrieve_on_insert on default_value_col without INSERT ... RETURNING' + ); + SKIP: { skip 'not detecting sequences when using INSERT ... RETURNING', 1 if $schema->storage->_use_insert_returning; @@ -337,7 +349,7 @@ sub _run_tests { } 'with_deferred_fk_checks code survived'; is eval { $schema->resultset('Track')->find(999)->title }, 'deferred FK track', - 'code in with_deferred_fk_checks worked'; + 'code in with_deferred_fk_checks worked'; throws_ok { $schema->resultset('Track')->create({ @@ -504,7 +516,7 @@ sub _run_tests { skip 'not detecting cross-schema sequence name when using INSERT ... RETURNING', 1 if $schema->storage->_use_insert_returning; - # Oracle8i Reference Release 2 (8.1.6) + # Oracle8i Reference Release 2 (8.1.6) # http://download.oracle.com/docs/cd/A87860_01/doc/server.817/a76961/ch294.htm#993 # Oracle Database Reference 10g Release 2 (10.2) # http://download.oracle.com/docs/cd/B19306_01/server.102/b14237/statviews_2107.htm#sthref1297 @@ -519,6 +531,7 @@ sub _run_tests { # grand select privileges to the 2nd user $dbh->do("GRANT INSERT ON ${q}artist${q} TO " . uc $user2); + $dbh->do("GRANT SELECT ON ${q}artist${q} TO " . uc $user2); $dbh->do("GRANT SELECT ON ${q}artist_pk_seq${q} TO " . uc $user2); $dbh->do("GRANT SELECT ON ${q}artist_autoinc_seq${q} TO " . uc $user2); @@ -600,7 +613,7 @@ sub do_creates { # this one is always unquoted as per manually specified sequence => $dbh->do("CREATE SEQUENCE pkid2_seq START WITH 10 MAXVALUE 999999 MINVALUE 0"); - $dbh->do("CREATE TABLE ${q}artist${q} (${q}artistid${q} NUMBER(12), ${q}name${q} VARCHAR(255), ${q}autoinc_col${q} NUMBER(12), ${q}rank${q} NUMBER(38), ${q}charfield${q} VARCHAR2(10))"); + $dbh->do("CREATE TABLE ${q}artist${q} (${q}artistid${q} NUMBER(12), ${q}name${q} VARCHAR(255),${q}default_value_col${q} VARCHAR(255) DEFAULT 'default_value', ${q}autoinc_col${q} NUMBER(12), ${q}rank${q} NUMBER(38), ${q}charfield${q} VARCHAR2(10))"); $dbh->do("ALTER TABLE ${q}artist${q} ADD (CONSTRAINT ${q}artist_pk${q} PRIMARY KEY (${q}artistid${q}))"); $dbh->do("CREATE TABLE ${q}sequence_test${q} (${q}pkid1${q} NUMBER(12), ${q}pkid2${q} NUMBER(12), ${q}nonpkid${q} NUMBER(12), ${q}name${q} VARCHAR(255))");