Revert prematurely merged default insert support for Oracle (see below)
authorPeter Rabbitson <ribasushi@cpan.org>
Wed, 22 Oct 2014 13:13:43 +0000 (15:13 +0200)
committerPeter Rabbitson <ribasushi@cpan.org>
Wed, 22 Oct 2014 19:27:51 +0000 (21:27 +0200)
commitf8135512c346d8cb6f3dc569f4bb11576e85f97b
treeca6e118e539f3d521b98f0c93436c8ea1546d986
parentcf52a9ad15a73aedfc1822643d63a9aa7982c72c
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
AUTHORS
Changes
lib/DBIx/Class/Storage/DBI/Oracle/Generic.pm
t/60core.t
t/73oracle.t