From: Peter Rabbitson Date: Wed, 22 Oct 2014 13:13:43 +0000 (+0200) Subject: Revert prematurely merged default insert support for Oracle (see below) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=f8135512c346d8cb6f3dc569f4bb11576e85f97b;p=dbsrgits%2FDBIx-Class-Historic.git Revert prematurely merged default insert support for Oracle (see below) Note: the incorrect implementation is not the fault of the original submitter (who can not be expected to know the vagaries of the execution path) nor is the fault of the original reviewer (who can not be expected to remember said vagaries for a storage they do not even use). I do not have recommendations on how to avoid this situation in the future. It is correct that the particular problem can not be solved on the level of SQLMaker::Oracle like it was solved for MySQL. However the current fix applies the default-on-empty way too early in the call stack - there is a lot of stuff happening between where we would ideally be applying the workaround and the current application site. With an Oracle storage the call-stack is roughly the following: ( ... non-storage code ) -> ::Storage::DBI::insert X -> ::Storage::DBI::_prefetch_autovalues X -> (loop over) ::Storage::DBI::Oracle::Generic::_sequence_fetch X -> determine if autoincs are accounted for ? -> assemble INSERT RETURNING container -> ::Storage::DBI::_execute -> ::Storage::DBI::Oracle::Generic::_prep_for_execute -> ::Storage::DBI::_prep_for_execute -> ::Storage::DBI::_gen_sql_bind -> ::SQLMaker::Oracle::insert(...) Setting up the "missing" default so early definitely affects the points marked with X and possibly affects the code marked with ?. The issues that would come out of this range from obvious failure modes to "this will be easily broken by a future reshuffle" gut feeling. The patch therefore needs reworking adding the extra pieces of logic either in a new _gen_sql_bind hook, or at the very least in the already existing conditional at the start of ::Storage::DBI::Oracle::Generic::_prep_for_execute (though a hook of _gen_sql_bind is preferable). Additionally the test needs to be expanded to exercise the case of pre-existing test cases that trigger _prefetch_autovalue (the tests involving the source SequenceTest in t/73oracle.t) And lastly an explicit exception should be added to SQLMaker::Oracle::insert to catch future changes to any code higher up, so that a refactor will not silently end up calling SQLMaker::Oracle::insert with an empty insert-list, throwing everything back to square one. This commit reverts the following two patches, with the intention to merge a fixed-up version shortly: - 236b59c8 - 1af6b968 --- diff --git a/AUTHORS b/AUTHORS index 350654c..180d485 100644 --- a/AUTHORS +++ b/AUTHORS @@ -105,7 +105,6 @@ jshirley: J. Shirley kaare: Kaare Rasmussen kd: Kieren Diment konobi: Scott McWhirter -Lasse Makholm lejeunerenard: Sean Zellmer littlesavage: Alexey Illarionov lukes: Luke Saunders diff --git a/Changes b/Changes index 547e8cb..a65fd89 100644 --- a/Changes +++ b/Changes @@ -1,7 +1,6 @@ Revision history for DBIx::Class * Fixes - - Fix $rs->create() with no values on Oracle - Silence with_deferred_fk_checks() warning on PostgreSQL 9.4 * Misc diff --git a/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm b/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm index a90755c..2b4ce75 100644 --- a/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm +++ b/lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm @@ -115,21 +115,6 @@ sub deployment_statements { $self->next::method($schema, $type, $version, $dir, $sqltargs, @rest); } -sub insert { - my ($self, $source, $to_insert) = @_; - - # Oracle does not understand INSERT INTO ... DEFAULT VALUES syntax - # Furthermore it does not have any way to insert without specifying any columns - # We can't fix this in SQLMaker::Oracle because it doesn't know which column to add to the statement - unless (%$to_insert) - { - my ($col) = $source->columns; - $to_insert = { $col => \'DEFAULT' }; - } - - return $self->next::method($source, $to_insert); -} - sub _dbh_last_insert_id { my ($self, $dbh, $source, @columns) = @_; my @ids = (); diff --git a/t/60core.t b/t/60core.t index d848a91..1f7e5f9 100644 --- a/t/60core.t +++ b/t/60core.t @@ -576,9 +576,6 @@ lives_ok (sub { my $newlink = $newbook->link}, "stringify to false value doesn't { my $new_artist = $schema->resultset('Artist')->new({}); isa_ok( $new_artist, 'DBIx::Class::Row', '$rs->new gives a row object' ); - lives_ok { $new_artist->insert() } 'inserting without specifying any columns works'; - $new_artist->discard_changes; - $new_artist->delete; } # make sure we got rid of the compat shims diff --git a/t/73oracle.t b/t/73oracle.t index f6da1a1..a8a1e70 100644 --- a/t/73oracle.t +++ b/t/73oracle.t @@ -201,13 +201,6 @@ sub _run_tests { like ($seq, qr/\.${q}artist_pk_seq${q}$/, 'Correct PK sequence selected for sqlt-like trigger'); } - lives_ok { - $new = $schema->resultset('Artist')->create({}); - $new->discard_changes; - ok $new->artistid, 'Created row has id' - } 'Create with empty hashref works'; - - # test LIMIT support for (1..6) { $schema->resultset('Artist')->create({ name => 'Artist ' . $_ });