Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

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

Closed
wants to merge 1 commit into from

2 participants

Chris Hanks Jeremy Evans
Chris Hanks

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?

Jeremy Evans

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.

Jeremy Evans

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

Showing 1 unique commit by 1 author.

Sep 03, 2012
Chris Hanks chanks Expand the association_pks plugin to handle right-side composite prim…
…ary keys.
dabe291
This page is out of date. Refresh to see the latest.
6 lib/sequel/plugins/association_pks.rb
@@ -61,7 +61,11 @@ def def_many_to_many(opts)
61 61 ds = _join_table_dataset(opts).filter(opts[:left_key]=>send(opts[:left_primary_key]))
62 62 ds.exclude(opts[:right_key]=>pks).delete
63 63 pks -= ds.select_map(opts[:right_key])
64   - pks.each{|pk| ds.insert(opts[:left_key]=>send(opts[:left_primary_key]), opts[:right_key]=>pk)}
  64 + pks.each do |pk|
  65 + insert = Hash[Array(opts[:right_key]).zip(Array(pk))]
  66 + insert[opts[:left_key]] = send(opts[:left_primary_key])
  67 + ds.insert(insert)
  68 + end
65 69 end
66 70 end
67 71 end
44 spec/extensions/association_pks_spec.rb
@@ -12,14 +12,25 @@
12 12 a << {:tag_id=>2} if $1 != '3'
13 13 a << {:tag_id=>3} if $1 == '2'
14 14 a
  15 + when "SELECT first_name, last_name FROM vocalists WHERE (vocalists.album_id = 1)"
  16 + [{:first_name=>"F1", :last_name=>"L1"}, {:first_name=>"F2", :last_name=>"L2"}]
  17 + when /SELECT first_name, last_name FROM albums_vocalists WHERE \(album_id = (\d)\)/
  18 + a = []
  19 + a << {:first_name=>"F1", :last_name=>"L1"} if $1 == '1'
  20 + a << {:first_name=>"F2", :last_name=>"L2"} if $1 != '3'
  21 + a << {:first_name=>"F3", :last_name=>"L3"} if $1 == '2'
  22 + a
15 23 end
16 24 end)
17 25 @Artist = Class.new(Sequel::Model(@db[:artists]))
18 26 @Artist.columns :id
19   - @Album= Class.new(Sequel::Model(@db[:albums]))
  27 + @Album = Class.new(Sequel::Model(@db[:albums]))
20 28 @Album.columns :id, :artist_id
21 29 @Tag = Class.new(Sequel::Model(@db[:tags]))
22 30 @Tag.columns :id
  31 + @Vocalist = Class.new(Sequel::Model(@db[:vocalists]))
  32 + @Vocalist.columns :first_name, :last_name, :album_id
  33 + @Vocalist.set_primary_key [:first_name, :last_name]
23 34 @Artist.plugin :association_pks
24 35 @Album.plugin :association_pks
25 36 @Artist.one_to_many :albums, :class=>@Album, :key=>:artist_id
@@ -60,6 +71,37 @@
60 71 sqls.length.should == 3
61 72 end
62 73
  74 + specify "should return correct associated cpks for one_to_many associations" do
  75 + @Album.one_to_many :vocalists, :class=>@Vocalist, :key=>:album_id
  76 + @Album.load(:id=>1).vocalist_pks.should == [["F1", "L1"], ["F2", "L2"]]
  77 + @Album.load(:id=>2).vocalist_pks.should == []
  78 + end
  79 +
  80 + specify "should return correct associated cpks for many_to_many associations" do
  81 + @Album.many_to_many :vocalists, :class=>@Vocalist, :join_table=>:albums_vocalists, :left_key=>:album_id, :right_key=>[:first_name, :last_name]
  82 + @Album.load(:id=>1).vocalist_pks.should == [["F1", "L1"], ["F2", "L2"]]
  83 + @Album.load(:id=>2).vocalist_pks.should == [["F2", "L2"], ["F3", "L3"]]
  84 + @Album.load(:id=>3).vocalist_pks.should == []
  85 + end
  86 +
  87 + specify "should set associated cpks correctly for a one_to_many association" do
  88 + @Album.one_to_many :vocalists, :class=>@Vocalist, :key=>:album_id
  89 + @Album.load(:id=>1).vocalist_pks = [["F1", "L1"], ["F2", "L2"]]
  90 + @db.sqls.should == ["UPDATE vocalists SET album_id = 1 WHERE ((first_name, last_name) IN (('F1', 'L1'), ('F2', 'L2')))",
  91 + "UPDATE vocalists SET album_id = NULL WHERE ((vocalists.album_id = 1) AND ((first_name, last_name) NOT IN (('F1', 'L1'), ('F2', 'L2'))))"]
  92 + end
  93 +
  94 + specify "should set associated cpks correctly for a many_to_many association" do
  95 + @Album.many_to_many :vocalists, :class=>@Vocalist, :join_table=>:albums_vocalists, :left_key=>:album_id, :right_key=>[:first_name, :last_name]
  96 + @Album.load(:id=>2).vocalist_pks = [["F1", "L1"], ["F2", "L2"]]
  97 + sqls = @db.sqls
  98 + sqls[0].should == "DELETE FROM albums_vocalists WHERE ((album_id = 2) AND ((first_name, last_name) NOT IN (('F1', 'L1'), ('F2', 'L2'))))"
  99 + sqls[1].should == 'SELECT first_name, last_name FROM albums_vocalists WHERE (album_id = 2)'
  100 + sqls[2] =~ /INSERT INTO albums_vocalists \((.*)\) VALUES \((.*)\)/
  101 + Hash[$1.split(', ').zip($2.split(', '))].should == {"first_name"=>"'F1'", "last_name"=>"'L1'", "album_id"=>"2"}
  102 + sqls.length.should == 3
  103 + end
  104 +
63 105 specify "should use transactions if the object is configured to use transactions" do
64 106 artist = @Artist.load(:id=>1)
65 107 artist.use_transactions = true

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.