Move the rows/offset sanity checks from DBI to _resolved_attrs

Doing this much earlier should tie all loose ends wrt rows => 0 (sigh...)

diff --git a/Changes b/Changes
index 13de347..8f02e8d 100644 (file)
--- a/Changes
+++ b/Changes
@@ -20,6 +20,8 @@ Revision history for DBIx::Class
     * Fixes
         - Ensure failing on_connect* / on_disconnect* are dealt with properly,
           notably on_connect* failures now properly abort the entire connect
+        - Fix incorrect SQL generated with invalid {rows} on complex resultset
+          operations, generally more robust handling of rows/offset attrs
         - Make sure exception objects stringifying to '' are properly handled
           and warned about (GH#15)
         - Fix corner case of stringify-only overloaded objects being used in

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 3d1a3a1..3da8a79 100644 (file)
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -3531,6 +3531,19 @@ sub _resolved_attrs {
   $self->throw_exception("Specifying distinct => 1 in conjunction with collapse => 1 is unsupported")
     if $attrs->{collapse} and $attrs->{distinct};
 
+
+  # Sanity check the paging attributes
+  # SQLMaker does it too, but in case of a software_limit we'll never get there
+  if (defined $attrs->{offset}) {
+    $self->throw_exception('A supplied offset attribute must be a non-negative integer')
+      if ( $attrs->{offset} =~ /[^0-9]/ or $attrs->{offset} < 0 );
+  }
+  if (defined $attrs->{rows}) {
+    $self->throw_exception("The rows attribute must be a positive integer if present")
+      if ( $attrs->{rows} =~ /[^0-9]/ or $attrs->{rows} <= 0 );
+  }
+
+
   # default selection list
   $attrs->{columns} = [ $source->columns ]
     unless List::Util::first { exists $attrs->{$_} } qw/columns cols select as/;

diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index 7908e3f..65293c3 100644 (file)
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -2427,21 +2427,9 @@ sub _select_args {
     where => $where,
   };
 
-  # Sanity check the attributes (SQLMaker does it too, but
-  # in case of a software_limit we'll never reach there)
-  if (defined $attrs->{offset}) {
-    $self->throw_exception('A supplied offset attribute must be a non-negative integer')
-      if ( $attrs->{offset} =~ /[^0-9]/ or $attrs->{offset} < 0 );
-  }
-
-  if (defined $attrs->{rows}) {
-    $self->throw_exception("The rows attribute must be a positive integer if present")
-      if ( $attrs->{rows} =~ /[^0-9]/ or $attrs->{rows} <= 0 );
-  }
-  elsif ($attrs->{offset}) {
-    # MySQL actually recommends this approach.  I cringe.
-    $attrs->{rows} = $sql_maker->__max_int;
-  }
+  # MySQL actually recommends this approach.  I cringe.
+  $attrs->{rows} ||= $sql_maker->__max_int
+    if $attrs->{offset};
 
   # see if we will need to tear the prefetch apart to satisfy group_by == select
   # this is *extremely tricky* to get right, I am still not sure I did

diff --git a/t/resultset/update_delete.t b/t/resultset/update_delete.t
index d13848c..f49fb0e 100644 (file)
--- a/t/resultset/update_delete.t
+++ b/t/resultset/update_delete.t
@@ -286,6 +286,10 @@ cmp_ok ($tkfk_cnt, '>', 1, 'More than 1 row left');
 $tkfks->search ({}, { rows => 1 })->delete;
 is ($tkfks->count, $tkfk_cnt -= 1, 'Only one row deleted');
 
+throws_ok {
+  $tkfks->search ({}, { rows => 0 })->delete
+} qr/rows attribute must be a positive integer/;
+is ($tkfks->count, $tkfk_cnt, 'Nothing deleted');
 
 # check with sql-equality, as sqlite will accept most bad sql just fine
 {