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 (file)
--- a/AUTHORS
+++ b/AUTHORS
@@ -105,7 +105,6 @@ jshirley: J. Shirley <jshirley@gmail.com>
 kaare: Kaare Rasmussen
 kd: Kieren Diment <diment@gmail.com>
 konobi: Scott McWhirter <konobi@cpan.org>
-Lasse Makholm <lasse@unity3d.com>
 lejeunerenard: Sean Zellmer <sean@lejeunerenard.com>
 littlesavage: Alexey Illarionov <littlesavage@orionet.ru>
 lukes: Luke Saunders <luke.saunders@gmail.com>

diff --git a/Changes b/Changes
index 547e8cb..a65fd89 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 ' . $_ });