From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 30 Aug 2012 13:32:50 +0000 (+0200)
Subject: Comments on code
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=refs%2Fheads%2Fpeople%2Filmari%2Fdeferred-exception-propagation;p=dbsrgits%2FDBIx-Class.git

Comments on code
---

diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index 3f9c2e8..80df9ca 100644
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -28,6 +28,9 @@ __PACKAGE__->mk_group_accessors('inherited' => qw/
   _unsatisfied_deferred_constraints_autorollback
 /);
 
+# I would change this to something that makes sense, like
+# _autorollback_on_deferred_constraint_violation
+# also setting it to an explicit 0 is silly
 # see with_deferred_fk_checks
 __PACKAGE__->_unsatisfied_deferred_constraints_autorollback(0);
 
@@ -863,6 +866,9 @@ in MySQL's case disabled entirely.
 sub with_deferred_fk_checks {
   my ($self, $sub) = @_;
 
+  # now this has a lot of logic built in, including an unconditional
+  # try/catch block on top. If code ends up nesting with_deferred_fk_checks
+  # blocks - lots of crap will go wrong. We need a test for nesting too.
   if ($self->can('_set_constraints_deferred') &&
       $self->can('_set_constraints_immediate')) {
 
@@ -871,6 +877,7 @@ sub with_deferred_fk_checks {
     return try {
       my $guard = $self->txn_scope_guard;
       $self->_set_constraints_deferred;
+      # why { $sub->() } and not just $sub ?
       preserve_context { $sub->() } after => sub {
         my $e;
         eval {
@@ -879,10 +886,22 @@ sub with_deferred_fk_checks {
         if ($@) {
           if ($self->_unsatisfied_deferred_constraints_autorollback) {
             $guard->{inactivated} = 1; # DO NOT ROLLBACK
+
+            # *POSSIBLY* a code smell
+            # perhaps this shouldn't be here at all, ::Storage::txn_commit
+            # (which is called on all commits, including guards) should know to
+            # check for "am I in a deferred state" and "do I auto-rollback"
+            # and then do all the reductions on its own
             $self->{transaction_depth}--;
           }
           $e = $@;
         }
+
+        # some engines (e.g. Pg) auto-unset deferred txns on rollback
+        # there should be an extra storage flag, and a provision to
+        # avoid this entire eval invoking a noop
+        # also the semantic is not clear what happens when an autorollback
+        # happens in the midst of a nested savepoint. Need more tests
         eval {
           $tried_to_reset_constraints = 1;
           $self->_set_constraints_immediate;
@@ -900,6 +919,8 @@ sub with_deferred_fk_checks {
     }
     catch {
       my $e = $_;
+
+      # same commend as on the eval above
       if (not $tried_to_reset_constraints) {
         eval {
           $self->_set_constraints_immediate;
diff --git a/lib/DBIx/Class/Storage/DBI/Informix.pm b/lib/DBIx/Class/Storage/DBI/Informix.pm
index dd6ffcd..dbd0ebd 100644
--- a/lib/DBIx/Class/Storage/DBI/Informix.pm
+++ b/lib/DBIx/Class/Storage/DBI/Informix.pm
@@ -50,12 +50,21 @@ sub _set_constraints_immediate {
 # fix it up here by doing a manual $dbh->do("COMMIT WORK"), propagating the
 # exception, and resetting the $dbh->{AutoCommit} attribute.
 
+# this crap is now exeuted on *every* commit, be it deferred constraints or not
+# moreover there is no DBD version check - the workaround will remain here
+# forever and ever until someone decides to profile it
+# so far the general way of doing this was to use the current CPAN available
+# version, and keep bumping it as DBDs are released with the bug outstanding.
+# of course having an explicit test for this helps - we do not have one
 sub _exec_txn_commit {
   my $self = shift;
 
   my $tried_resetting_autocommit = 0;
 
   try {
+    # what the fuck?! what's wrong with $dbh->commit for the *general* case?
+    # where is the detailed comment explaining why the violation of DBI
+    # internals? (the 2 lines above o not count as sufficient explanation)
     $self->_dbh->do('COMMIT WORK');
     if ($self->_dbh_autocommit && $self->transaction_depth == 1) {
       eval {
diff --git a/lib/DBIx/Class/Storage/DBI/Pg.pm b/lib/DBIx/Class/Storage/DBI/Pg.pm
index 709ec60..0028813 100644
--- a/lib/DBIx/Class/Storage/DBI/Pg.pm
+++ b/lib/DBIx/Class/Storage/DBI/Pg.pm
@@ -34,6 +34,8 @@ sub _set_constraints_deferred {
 # transaction" so that they can be checked.
 
 sub _set_constraints_immediate {
+  # the semantic is not clear what happens when an autorollback happens in the
+  # midst of a nested savepoint (txn_depth is set all the same). Moar tests
   $_[0]->_do_query('SET CONSTRAINTS ALL IMMEDIATE') if $_[0]->transaction_depth;
 }
 
diff --git a/lib/DBIx/Class/Storage/DBI/mysql.pm b/lib/DBIx/Class/Storage/DBI/mysql.pm
index 616d7d7..2eea92a 100644
--- a/lib/DBIx/Class/Storage/DBI/mysql.pm
+++ b/lib/DBIx/Class/Storage/DBI/mysql.pm
@@ -17,6 +17,8 @@ __PACKAGE__->_use_multicolumn_in (1);
 # We turn FOREIGN_KEY_CHECKS off, do a transaction, then turn them back on right
 # before the COMMIT so that they can be checked during the COMMIT.
 
+# nothing tests this hypothesis, why?
+
 sub with_deferred_fk_checks {
   my ($self, $sub) = @_;