jeremyevans / sequel

Sequel: The Database Toolkit for Ruby
http://sequel.jeremyevans.net
Other
5k stars 1.07k forks source link

Datasets from eager_graph get mutated sometimes #392

Closed brian-goldman closed 13 years ago

brian-goldman commented 13 years ago

It's my understanding (please correct me if I'm wrong) that a Dataset instance is supposed to be immutable in terms of what underlying dataset it represents, and that you make changes to them by chaining methods to create new instances.

Here's a script I created that seems to violate that assumption.

require 'sequel'

db = Sequel.sqlite

db.create_table :x do
  Integer :id
end

db.create_table :y do
  Integer :x_id
end

db.create_table :z do
  Integer :x_id
end

class X < Sequel::Model
  set_dataset :x
  one_to_many :y, :class => :Y, :key => :x_id, :primary_key => :id
  one_to_many :z, :class => :Z, :key => :x_id, :primary_key => :id
end

class Y < Sequel::Model
  set_dataset :y
end

class Z < Sequel::Model
  set_dataset :z
end

ds = X.eager_graph(:y)

puts ds.sql
puts ds.eager_graph(:z).sql
puts ds.eager_graph(:z).sql

When I run this with Ruby 1.9.2 and Sequel 3.27.0, I get this output:

SELECT `x`.`id`, `y`.`x_id` FROM `x` LEFT OUTER JOIN `y` ON (`y`.`x_id` = `x`.`id`)
SELECT `x`.`id`, `y`.`x_id`, `z`.`x_id` AS 'z_x_id' FROM `x` LEFT OUTER JOIN `y` ON (`y`.`x_id` = `x`.`id`) LEFT OUTER JOIN `z` ON (`z`.`x_id` = `x`.`id`)
/Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/dataset/graph.rb:98:in `block in graph': this alias has already been been used, please specify a different alias (Sequel::Error)
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/dataset/graph.rb:103:in `call'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/dataset/graph.rb:103:in `graph'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:964:in `block in def_one_to_many'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:1500:in `call'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:1500:in `eager_graph_association'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:1528:in `block in eager_graph_associations'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:1525:in `each'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:1525:in `eager_graph_associations'
    from /Users/bgoldman/.rvm/gems/ruby-1.9.2-p290/gems/sequel-3.27.0/lib/sequel/model/associations.rb:1461:in `eager_graph'
    from sequel_bug.rb:35:in `<main>'
jeremyevans commented 13 years ago

This definitely appears to be a bug. Somewhere in eager_graph, it's not completely copying the previous datastructure before modifying it.

jeremyevans commented 13 years ago

Looks like there were problems both in the model eager graph code and the dataset graph code with it not duping the existing datastructures before modifying them. I've fixed both, and I'm running the fixes through the test suites now. Assuming no problems, they should be pushed to github within an hour.