From: Matt S Trout <mst@shadowcat.co.uk>
Date: Fri, 20 Apr 2012 09:17:45 +0000 (+0000)
Subject: order stability testing
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=db3abd3401b1906adbe2a58a62405b389b63f875;p=dbsrgits%2FDBIx-Class-Historic.git

order stability testing
---

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 43c06fb..ed2a3f3 100644
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -1371,11 +1371,9 @@ sub _construct_objects {
       my @order_by_dq = $self->_extract_by_from_order_by(
         $conv->_order_by_to_dq($attrs->{order_by})
       );
-      my @ord_cols = map {
-        $_->{type} eq DQ_IDENTIFIER
-          ? join('.',@{$_->{elements}})
-          : '__TOTALLY_BOGUS_DUDE__'
-      } @order_by_dq;
+      my @ord_cols = map { join('.',@{$_->{elements}}) }
+                       grep { $_->{type} eq DQ_IDENTIFIER }
+                         @order_by_dq;
 
       my $colinfos = $st->_resolve_column_info($attrs->{from}, \@ord_cols);
 
diff --git a/lib/DBIx/Class/SQLMaker.pm b/lib/DBIx/Class/SQLMaker.pm
index 83f5da8..88ede1e 100644
--- a/lib/DBIx/Class/SQLMaker.pm
+++ b/lib/DBIx/Class/SQLMaker.pm
@@ -49,6 +49,12 @@ use Moo;
 
 has limit_dialect => (is => 'rw', trigger => sub { shift->clear_renderer });
 
+has limit_requires_order_by_stability_check
+  => (is => 'rw', default => sub { 0 });
+
+has limit_enforces_order_by_stability
+  => (is => 'rw', default => sub { 0 });
+
 # for when I need a normalized l/r pair
 sub _quote_chars {
   map
@@ -57,7 +63,9 @@ sub _quote_chars {
   ;
 }
 
-sub converter_class { 'DBIx::Class::SQLMaker::Converter' }
+sub _build_converter_class {
+  Module::Runtime::use_module('DBIx::Class::SQLMaker::Converter')
+}
 
 # FIXME when we bring in the storage weaklink, check its schema
 # weaklink and channel through $schema->throw_exception
@@ -129,7 +137,39 @@ sub select {
     $limit = $self->__max_int;
   }
 
-  my ($sql, @bind) = $self->next::method ($table, $fields, $where, $rs_attrs->{order_by}, { %{$rs_attrs}, limit => $limit, offset => $offset } );
+  my %final_attrs = (%{$rs_attrs}, limit => $limit, offset => $offset);
+
+  if ($offset and $self->limit_requires_order_by_stability_check) {
+     my $source = $rs_attrs->{_rsroot_rsrc};
+    unless (
+      $final_attrs{order_is_stable}
+      = $source->schema->storage
+               ->_order_by_is_stable(
+                   @final_attrs{qw(from order_by where)}
+                 )
+    ) {
+      if ($self->limit_enforces_order_by_stability) {
+        if ($self->converter->_order_by_to_dq($final_attrs{order_by})) {
+          $self->throw_exception(
+            'Current limit/offset implementation requires a stable order for offset'
+          );
+        }
+        if (my $ident_cols = $source->_identifying_column_set) {
+          $final_attrs{order_by} = [
+            map "$final_attrs{alias}.$_", @$ident_cols
+          ];
+          $final_attrs{order_is_stable} = 1;
+        } else {
+          $self->throw_exception(sprintf(
+            'Unable to auto-construct stable order criteria for "skimming type" 
+limit '
+          . "dialect based on source '%s'", $source->name) );
+        }
+      }
+    }
+  }
+
+  my ($sql, @bind) = $self->next::method ($table, $fields, $where, $final_attrs{order_by}, \%final_attrs );
 
   $sql .= $self->_lock_select ($rs_attrs->{for})
     if $rs_attrs->{for};
diff --git a/lib/DBIx/Class/SQLMaker/Converter.pm b/lib/DBIx/Class/SQLMaker/Converter.pm
index d927c75..82fd8b9 100644
--- a/lib/DBIx/Class/SQLMaker/Converter.pm
+++ b/lib/DBIx/Class/SQLMaker/Converter.pm
@@ -3,7 +3,6 @@ package DBIx::Class::SQLMaker::Converter;
 use Data::Query::Constants qw(DQ_ALIAS DQ_GROUP DQ_WHERE DQ_JOIN DQ_SLICE);
 use Moo;
 
-require SQL::Abstract::Converter; # XXX Moo bug caused by the local
 extends 'SQL::Abstract::Converter';
 
 around _select_to_dq => sub {
@@ -27,6 +26,9 @@ around _select_to_dq => sub {
         })
       : ()
     ),
+    ($attrs->{order_is_stable}
+      ? (order_is_stable => 1)
+      : ())
   };
 };
 
diff --git a/lib/DBIx/Class/SQLMaker/SQLite.pm b/lib/DBIx/Class/SQLMaker/SQLite.pm
index f905b46..ce719ba 100644
--- a/lib/DBIx/Class/SQLMaker/SQLite.pm
+++ b/lib/DBIx/Class/SQLMaker/SQLite.pm
@@ -3,7 +3,9 @@ package # Hide from PAUSE
 
 use base qw( DBIx::Class::SQLMaker );
 
-sub renderer_class { 'Data::Query::Renderer::SQL::SQLite' }
+sub _build_renderer_class {
+  Module::Runtime::use_module('Data::Query::Renderer::SQL::SQLite')
+}
 
 #
 # SQLite does not understand SELECT ... FOR UPDATE
diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm
index a5ce102..633836a 100644
--- a/lib/DBIx/Class/Storage/DBIHacks.pm
+++ b/lib/DBIx/Class/Storage/DBIHacks.pm
@@ -17,7 +17,7 @@ use List::Util 'first';
 use Scalar::Util 'blessed';
 use Sub::Name 'subname';
 use Data::Query::Constants qw(
-  DQ_ALIAS DQ_JOIN DQ_IDENTIFIER DQ_ORDER DQ_LITERAL
+  DQ_ALIAS DQ_JOIN DQ_IDENTIFIER DQ_ORDER DQ_LITERAL DQ_OPERATOR
 );
 use namespace::clean;
 
@@ -705,9 +705,27 @@ sub _extract_order_criteria {
 sub _order_by_is_stable {
   my ($self, $ident, $order_by, $where) = @_;
 
+  my @ident_dq;
+  my $conv = $self->sql_maker->converter;
+
+  $self->_scan_identifiers(
+    sub { push @ident_dq, $_[0] }, $conv->_order_by_to_dq($order_by)
+  );
+
+  if ($where) {
+    # old _extract_fixed_condition_columns logic
+    $self->_scan_nodes({
+      DQ_OPERATOR ,=> sub {
+        if (($_[0]->{operator}{'SQL.Naive'}||'') eq '=') {
+          push @ident_dq,
+            grep { $_->{type} eq DQ_IDENTIFIER } @{$_[0]->{args}};
+        }
+      }
+    }, $conv->_where_to_dq($where));
+  }
+
   my $colinfo = $self->_resolve_column_info($ident, [
-    (map { $_->[0] } $self->_extract_order_criteria($order_by)),
-    $where ? @{$self->_extract_fixed_condition_columns($where)} :(),
+    map { join('.', @{$_->{elements}}) } @ident_dq
   ]);
 
   return undef unless keys %$colinfo;
@@ -723,41 +741,4 @@ sub _order_by_is_stable {
   return undef;
 }
 
-# returns an arrayref of column names which *definitely* have som
-# sort of non-nullable equality requested in the given condition
-# specification. This is used to figure out if a resultset is
-# constrained to a column which is part of a unique constraint,
-# which in turn allows us to better predict how ordering will behave
-# etc.
-#
-# this is a rudimentary, incomplete, and error-prone extractor
-# however this is OK - it is conservative, and if we can not find
-# something that is in fact there - the stack will recover gracefully
-# Also - DQ and the mst it rode in on will save us all RSN!!!
-sub _extract_fixed_condition_columns {
-  my ($self, $where, $nested) = @_;
-
-  return unless ref $where eq 'HASH';
-
-  my @cols;
-  for my $lhs (keys %$where) {
-    if ($lhs =~ /^\-and$/i) {
-      push @cols, ref $where->{$lhs} eq 'ARRAY'
-        ? ( map { $self->_extract_fixed_condition_columns($_, 1) } @{$where->{$lhs}} )
-        : $self->_extract_fixed_condition_columns($where->{$lhs}, 1)
-      ;
-    }
-    elsif ($lhs !~ /^\-/) {
-      my $val = $where->{$lhs};
-
-      push @cols, $lhs if (defined $val and (
-        ! ref $val
-          or
-        (ref $val eq 'HASH' and keys %$val == 1 and defined $val->{'='})
-      ));
-    }
-  }
-  return $nested ? @cols : \@cols;
-}
-
 1;
diff --git a/t/sqlmaker/limit_dialects/fetch_first.t b/t/sqlmaker/limit_dialects/fetch_first.t
index 302201c..b7dc463 100644
--- a/t/sqlmaker/limit_dialects/fetch_first.t
+++ b/t/sqlmaker/limit_dialects/fetch_first.t
@@ -10,7 +10,14 @@ my $schema = DBICTest->init_schema;
 
 # based on toplimit.t
 delete $schema->storage->_sql_maker->{_cached_syntax};
-$schema->storage->_sql_maker->limit_dialect ('FetchFirst');
+$schema->storage->_sql_maker->renderer_class(
+  Moo::Role->create_class_with_roles(qw(
+    Data::Query::Renderer::SQL::Naive
+    Data::Query::Renderer::SQL::Slice::FetchFirst
+  ))
+);
+$schema->storage->_sql_maker->limit_requires_order_by_stability_check(1);
+$schema->storage->_sql_maker->limit_enforces_order_by_stability(1);
 
 my $books_45_and_owners = $schema->resultset ('BooksInLibrary')->search ({}, {
   prefetch => 'owner', rows => 2, offset => 3,