Permalink
Browse files

Don't blindly qualify columns when eager graphing

Qualifying all entries in the :order option when eager_graphing is
not correct behavior.  For example, it's legimate to order by a
function, which shouldn't be qualified.  Also, already qualified
symbols should not be qualified again.  Change the qualifying to
only qualify unqualified symbols and SQL::Identifiers, either of
which may be wrapped in an SQL::OrderedExpression.  Do not qualify
anything else.
  • Loading branch information...
1 parent 81d2c10 commit f7ed8c2d21c0aec99098d69af24d2b385dadc0cc @jeremyevans committed Oct 8, 2008
Showing with 28 additions and 5 deletions.
  1. +19 −1 lib/sequel_model/eager_loading.rb
  2. +9 −4 spec/sequel_model/eager_loading_spec.rb
@@ -147,7 +147,8 @@ def eager_graph_association(ds, model, ta, requirements, r, *associations)
ds = ds.graph(r[:join_table], use_jt_only_conditions ? r[:graph_join_table_only_conditions] : [[r[:left_key], model.primary_key.qualify(ta)]] + r[:graph_join_table_conditions], :select=>false, :table_alias=>ds.eager_unique_table_alias(ds, r[:join_table]), :join_type=>r[:graph_join_table_join_type], &r[:graph_join_table_block])
ds.graph(klass, use_only_conditions ? only_conditions : [[klass.primary_key, r[:right_key]]] + conditions, :select=>select, :table_alias=>assoc_table_alias, :join_type=>join_type, &graph_block)
end
- ds = ds.order_more(*(Array(r[:order]).map{|c| Sequel::SQL::QualifiedIdentifier.new(assoc_table_alias, c)})) if r[:order] and r[:order_eager_graph]
+
+ ds = ds.order_more(*Array(r[:order]).map{|c| eager_graph_qualify_order(assoc_table_alias, c)}) if r[:order] and r[:order_eager_graph]
eager_graph = ds.opts[:eager_graph]
eager_graph[:requirements][assoc_table_alias] = requirements.dup
eager_graph[:alias_association_name_map][assoc_table_alias] = assoc_name
@@ -331,6 +332,23 @@ def eager_graph_make_associations_unique(records, dependency_map, alias_map, typ
end
end
+ # Qualify the given expression if necessary. The only expressions which are qualified are
+ # unqualified symbols and identifiers, either of which may by sorted.
+ def eager_graph_qualify_order(table_alias, expression)
+ case expression
+ when Symbol
+ table, column, aliaz = split_symbol(expression)
+ raise(Sequel::Error, "Can't use an aliased expression in the :order option") if aliaz
+ table ? expression : Sequel::SQL::QualifiedIdentifier.new(table_alias, expression)
+ when Sequel::SQL::Identifier
+ Sequel::SQL::QualifiedIdentifier.new(table_alias, expression)
+ when Sequel::SQL::OrderedExpression
+ Sequel::SQL::OrderedExpression.new(eager_graph_qualify_order(table_alias, expression.expression), expression.descending)
+ else
+ expression
+ end
+ end
+
# Eagerly load all specified associations
def eager_load(a)
return if a.empty?
@@ -993,22 +993,27 @@ def ds.fetch_rows(sql, &block)
GraphAlbum.eager_graph(:previous_album=>{:previous_album=>:previous_album}).sql.should == "SELECT albums.id, albums.band_id, previous_album.id AS previous_album_id, previous_album.band_id AS previous_album_band_id, previous_album_0.id AS previous_album_0_id, previous_album_0.band_id AS previous_album_0_band_id, previous_album_1.id AS previous_album_1_id, previous_album_1.band_id AS previous_album_1_band_id FROM albums LEFT OUTER JOIN albums AS previous_album ON (previous_album.id = albums.previous_album_id) LEFT OUTER JOIN albums AS previous_album_0 ON (previous_album_0.id = previous_album.previous_album_id) LEFT OUTER JOIN albums AS previous_album_1 ON (previous_album_1.id = previous_album_0.previous_album_id)"
end
- it "should respect the associations :order" do
+ it "should respect the association's :order" do
GraphAlbum.one_to_many :right_tracks, :class=>'GraphTrack', :key=>:album_id, :order=>[:id, :album_id]
GraphAlbum.eager_graph(:right_tracks).sql.should == 'SELECT albums.id, albums.band_id, right_tracks.id AS right_tracks_id, right_tracks.album_id FROM albums LEFT OUTER JOIN tracks AS right_tracks ON (right_tracks.album_id = albums.id) ORDER BY right_tracks.id, right_tracks.album_id'
end
- it "should not respect the associations :order if :order_eager_graph is false" do
+ it "should only qualify unqualified symbols, identifiers, or ordered versions in association's :order" do
+ GraphAlbum.one_to_many :right_tracks, :class=>'GraphTrack', :key=>:album_id, :order=>[:blah__id.identifier, :blah__id.identifier.desc, :blah__id.desc, :blah__id, :album_id, :album_id.desc, 1, 'RANDOM()'.lit, :a.qualify(:b)]
+ GraphAlbum.eager_graph(:right_tracks).sql.should == 'SELECT albums.id, albums.band_id, right_tracks.id AS right_tracks_id, right_tracks.album_id FROM albums LEFT OUTER JOIN tracks AS right_tracks ON (right_tracks.album_id = albums.id) ORDER BY right_tracks.blah__id, right_tracks.blah__id DESC, blah.id DESC, blah.id, right_tracks.album_id, right_tracks.album_id DESC, 1, RANDOM(), b.a'
+ end
+
+ it "should not respect the association's :order if :order_eager_graph is false" do
GraphAlbum.one_to_many :right_tracks, :class=>'GraphTrack', :key=>:album_id, :order=>[:id, :album_id], :order_eager_graph=>false
GraphAlbum.eager_graph(:right_tracks).sql.should == 'SELECT albums.id, albums.band_id, right_tracks.id AS right_tracks_id, right_tracks.album_id FROM albums LEFT OUTER JOIN tracks AS right_tracks ON (right_tracks.album_id = albums.id)'
end
- it "should add the associations :order to the existing order" do
+ it "should add the association's :order to the existing order" do
GraphAlbum.one_to_many :right_tracks, :class=>'GraphTrack', :key=>:album_id, :order=>[:id, :album_id]
GraphAlbum.order(:band_id).eager_graph(:right_tracks).sql.should == 'SELECT albums.id, albums.band_id, right_tracks.id AS right_tracks_id, right_tracks.album_id FROM albums LEFT OUTER JOIN tracks AS right_tracks ON (right_tracks.album_id = albums.id) ORDER BY band_id, right_tracks.id, right_tracks.album_id'
end
- it "should add the associations :order for cascading associations" do
+ it "should add the association's :order for cascading associations" do
GraphBand.one_to_many :a_albums, :class=>'GraphAlbum', :key=>:band_id, :order=>:name
GraphAlbum.one_to_many :b_tracks, :class=>'GraphTrack', :key=>:album_id, :order=>[:id, :album_id]
GraphBand.eager_graph(:a_albums=>:b_tracks).sql.should == 'SELECT bands.id, bands.vocalist_id, a_albums.id AS a_albums_id, a_albums.band_id, b_tracks.id AS b_tracks_id, b_tracks.album_id FROM bands LEFT OUTER JOIN albums AS a_albums ON (a_albums.band_id = bands.id) LEFT OUTER JOIN tracks AS b_tracks ON (b_tracks.album_id = a_albums.id) ORDER BY a_albums.name, b_tracks.id, b_tracks.album_id'

0 comments on commit f7ed8c2

Please sign in to comment.