Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Expand the association_pks plugin to handle composite primary keys. #544

Closed
wants to merge 1 commit into from

2 participants

@chanks

This isn't done yet, I'm just looking for some guidance. I'm trying to get the association_pks plugin to support models with composite primary keys, and so far I've done right-side CPKs for both one-to-many and many-to-many associations. I don't think changing the actual plugin will be difficult, since three of my four test cases passed without even needing to modify the plugin, and the fourth required only minor changes.

The problem is testing, since to be thorough I'd like to support and test for (right|left|both)-side cpks on (one_to_many|many_to_many) associations, and I think the testing will get to be a mess. I think the mocking technique for returning records will become hard to use if I start cramming more queries into it, and I don't know what a good alternative would be. My method of testing the insert sql on the many_to_many setter test is also really hacky, and I'm not sure what to do there either (when I did some similar work in the nested_attributes plugin I was able to just override _insert in the model being tested, but there isn't a model here to do the same).

Suggestions?

@jeremyevans
Owner

If testing gets ugly it gets ugly, that's not a big deal as long as the implementation works. Sequel's test code isn't going to win awards for beauty. :) My philosophy is that specs/tests exist mainly to prevent regressions.

In addition to updating the extension specs, you should also update the integration tests in plugin_test.rb to handle composite keys (on left/right/both sides). For complex tasks, I usually write the integration tests before the extension specs, since integration tests are easier to write (you are only checking for correct results as opposed to SQL).

From a brief review of your pull request, what you have so far looks good. I'm closing now, but please push the rest of your patches and let me know when you are finished and I'll reopen, review and test.

Note that in the future, it's best not to submit a pull request until you have fully implemented the feature. If you have only partially implemented the feature and need feedback, just post on the sequel-talk Google Group. The advantage with that is that other people can also have a chance to review and offer advice.

@jeremyevans jeremyevans closed this
@jeremyevans
Owner

Support for this got merged in 166b035.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 3, 2012
  1. @chanks
This page is out of date. Refresh to see the latest.
View
6 lib/sequel/plugins/association_pks.rb
@@ -61,7 +61,11 @@ def def_many_to_many(opts)
ds = _join_table_dataset(opts).filter(opts[:left_key]=>send(opts[:left_primary_key]))
ds.exclude(opts[:right_key]=>pks).delete
pks -= ds.select_map(opts[:right_key])
- pks.each{|pk| ds.insert(opts[:left_key]=>send(opts[:left_primary_key]), opts[:right_key]=>pk)}
+ pks.each do |pk|
+ insert = Hash[Array(opts[:right_key]).zip(Array(pk))]
+ insert[opts[:left_key]] = send(opts[:left_primary_key])
+ ds.insert(insert)
+ end
end
end
end
View
44 spec/extensions/association_pks_spec.rb
@@ -12,14 +12,25 @@
a << {:tag_id=>2} if $1 != '3'
a << {:tag_id=>3} if $1 == '2'
a
+ when "SELECT first_name, last_name FROM vocalists WHERE (vocalists.album_id = 1)"
+ [{:first_name=>"F1", :last_name=>"L1"}, {:first_name=>"F2", :last_name=>"L2"}]
+ when /SELECT first_name, last_name FROM albums_vocalists WHERE \(album_id = (\d)\)/
+ a = []
+ a << {:first_name=>"F1", :last_name=>"L1"} if $1 == '1'
+ a << {:first_name=>"F2", :last_name=>"L2"} if $1 != '3'
+ a << {:first_name=>"F3", :last_name=>"L3"} if $1 == '2'
+ a
end
end)
@Artist = Class.new(Sequel::Model(@db[:artists]))
@Artist.columns :id
- @Album= Class.new(Sequel::Model(@db[:albums]))
+ @Album = Class.new(Sequel::Model(@db[:albums]))
@Album.columns :id, :artist_id
@Tag = Class.new(Sequel::Model(@db[:tags]))
@Tag.columns :id
+ @Vocalist = Class.new(Sequel::Model(@db[:vocalists]))
+ @Vocalist.columns :first_name, :last_name, :album_id
+ @Vocalist.set_primary_key [:first_name, :last_name]
@Artist.plugin :association_pks
@Album.plugin :association_pks
@Artist.one_to_many :albums, :class=>@Album, :key=>:artist_id
@@ -60,6 +71,37 @@
sqls.length.should == 3
end
+ specify "should return correct associated cpks for one_to_many associations" do
+ @Album.one_to_many :vocalists, :class=>@Vocalist, :key=>:album_id
+ @Album.load(:id=>1).vocalist_pks.should == [["F1", "L1"], ["F2", "L2"]]
+ @Album.load(:id=>2).vocalist_pks.should == []
+ end
+
+ specify "should return correct associated cpks for many_to_many associations" do
+ @Album.many_to_many :vocalists, :class=>@Vocalist, :join_table=>:albums_vocalists, :left_key=>:album_id, :right_key=>[:first_name, :last_name]
+ @Album.load(:id=>1).vocalist_pks.should == [["F1", "L1"], ["F2", "L2"]]
+ @Album.load(:id=>2).vocalist_pks.should == [["F2", "L2"], ["F3", "L3"]]
+ @Album.load(:id=>3).vocalist_pks.should == []
+ end
+
+ specify "should set associated cpks correctly for a one_to_many association" do
+ @Album.one_to_many :vocalists, :class=>@Vocalist, :key=>:album_id
+ @Album.load(:id=>1).vocalist_pks = [["F1", "L1"], ["F2", "L2"]]
+ @db.sqls.should == ["UPDATE vocalists SET album_id = 1 WHERE ((first_name, last_name) IN (('F1', 'L1'), ('F2', 'L2')))",
+ "UPDATE vocalists SET album_id = NULL WHERE ((vocalists.album_id = 1) AND ((first_name, last_name) NOT IN (('F1', 'L1'), ('F2', 'L2'))))"]
+ end
+
+ specify "should set associated cpks correctly for a many_to_many association" do
+ @Album.many_to_many :vocalists, :class=>@Vocalist, :join_table=>:albums_vocalists, :left_key=>:album_id, :right_key=>[:first_name, :last_name]
+ @Album.load(:id=>2).vocalist_pks = [["F1", "L1"], ["F2", "L2"]]
+ sqls = @db.sqls
+ sqls[0].should == "DELETE FROM albums_vocalists WHERE ((album_id = 2) AND ((first_name, last_name) NOT IN (('F1', 'L1'), ('F2', 'L2'))))"
+ sqls[1].should == 'SELECT first_name, last_name FROM albums_vocalists WHERE (album_id = 2)'
+ sqls[2] =~ /INSERT INTO albums_vocalists \((.*)\) VALUES \((.*)\)/
+ Hash[$1.split(', ').zip($2.split(', '))].should == {"first_name"=>"'F1'", "last_name"=>"'L1'", "album_id"=>"2"}
+ sqls.length.should == 3
+ end
+
specify "should use transactions if the object is configured to use transactions" do
artist = @Artist.load(:id=>1)
artist.use_transactions = true
Something went wrong with that request. Please try again.