Submitting a patch to the Open Source project: composite_primary_keys

Not so far ago I have found a weird bug in the Open Source Ruby gem called composite_primary_keys, occurred when you specify :primary_key option for has_one or has_many association. There are two ways to get it fixed: submit an issue and wait till someone will work out this problem or fix it by yourself and then pull request to get the patch merged into the core. This is a great library and I use it in almost all my project, so I decided to help the author and fix this bug by myself. Here I will show you how to do that.

I have discovered, that composite_primary_keys breaks my SQL queries when :primary_key option specified both for has_many and has_one associations.

Step 0. Reproducing the bug

First of all we need to reproduce a bug. Please note: if you know where the problem is, you can skip this step. Let’s start from a simple example:

1	rails cpk_bug && cd cpk_bug

Now we will add dependencies to the config/environment.rb:

1	config.gem 'composite_primary_keys'

and to the config/environments/test.rb:

1
2
3

config.gem 'rspec', :lib => 'spec'
config.gem 'rspec-rails', :lib => false
config.gem 'factory_girl'

Okay. Now we are ready to start. Let’s generate some migrations:

1
2
3
4
5

script/generate rspec_model document upload_request_id:integer title:string description:text
script/generate rspec_model upload_request filename:string state:integer
script/generate rspec_model copyright_request upload_request_id:integer explanation:text
rm -rf spec/fixtures
rake db:migrate && rake db:test:clone

Let’s create our factories:

1
2
3
4
5
6
7
8
9
10
11
12
13
14

Factory.define :copyright_request do |cr|
cr.explanation "This document is copyrighted by O'Reilly"
end

Factory.define :document do |d|
d.sequence(:title) { |n| "Document #{n}" }
d.description { |a| "The perfect description for the document '#{a.title}'" }
d.association :upload_request
end

Factory.define :upload_request do |ur|
ur.sequence(:filename) { |n| "file#{'%03d' % n}.pdf" }
ur.state 0
end

And our spec which should fail:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

require 'spec/spec_helper'

describe Document do
context 'when has copyright requests' do
before :each do
# Fake upload request used to desynchronise document and
# upload request IDs
Factory(:upload_request)
end

it 'should not have any copyright requests when just created' do
@document = Factory(:document)
@document.copyright_requests.should == []
end

it 'should return a list of copyright requests from #copyright_requests' do
@document = Factory(:document)
@request1 = Factory(:copyright_request, :upload_request => @document.upload_request)

@document.copyright_requests.should == [@request1]
end
end
end

Ok, it fails because we haven’t defined our associations on models. Let’s do it:

1
2
3
4
5
6
7
8
9
10
11
12

class Document < ActiveRecord::Base
belongs_to :upload_request

has_many :copyright_requests, :primary_key => :upload_request_id, :foreign_key => :upload_request_id
end

class UploadRequest < ActiveRecord::Base
end

class CopyrightRequest < ActiveRecord::Base
belongs_to :upload_request
end

Woohoo! Specs are failing with an error we’re trying to reproduce:

1
2
3
4
5
6
7
8
9
10
11
12

~/cpk_bug$ spec spec
..F.

1)
'Document when has copyright requests should return a list of copyright requests from #copyright_requests' FAILED
expected: [#<CopyrightRequest id: 1, upload_request_id: 2, explanation: "This document is copyrighted by O'Reilly", created_at: "2009-12-07 11:19:11", updated_at: "2009-12-07 11:19:11">],
got: [] (using ==)
/Users/kpumuk/cpk_bug/spec/models/document_spec.rb:20:

Finished in 0.155708 seconds

4 examples, 1 failure

Step 1. Setting up an environment for composite_primary_keys gem

In general to submit a patch to the Open Source project hosted by GitHub you have to perform the following steps: fork the repository on GitHub, write tests which fail, write a patch, ensure it works, push your changes to your fork repository, and submit a pull request. Let’s do just that!

To fork the repository I will use a perfect github gem by Dr Nic (BTW, he is the author of composite_primary_keys!)

1
2
3
4

sudo gem install github
gh clone drnic/composite_primary_keys
cd composite_primary_keys
gh fork

Now let’s configure our test environment and run tests:

1
2
3

rake local:setup
rake mysql:build_databases
rake test_mysql

You should see something like this:

1
2
3
4
5
6

Using native MySQL
Started
...................................................................................
Finished in 0.487679 seconds.

83 tests, 262 assertions, 0 failures, 0 errors

If you have any problems, check the test/README_tests.txt file for help.

Step 2. Reproducing failing tests inside composite_primary_keys test suite

We are doing TDD, right? So before any fixes we have to write a failing test first. Gem we’re hacking has a powerful test suite with many database tables created, so all we need is just to add associations to one of models, which will cover our issue.

First, add this to the test/fixtures/membership.rb model:

1 2	has_many :readings, :primary_key => :user_id, :foreign_key => :user_id has_one :reading, :primary_key => :user_id, :foreign_key => :user_id, :order => 'id DESC'

And this tests set goes to the test/test_associations.rb:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

def test_has_many_with_primary_key
@membership = Membership.find([1, 1])

assert_equal 2, @membership.readings.size
end

def test_has_one_with_primary_key
@membership = Membership.find([1, 1])

assert_equal 2, @membership.reading.id
end

def test_joins_has_many_with_primary_key
@membership = Membership.find(:first, :joins => :readings, :conditions => { :readings => { :id => 1 } })

assert_equal [1, 1], @membership.id
end

def test_joins_has_one_with_primary_key
@membership = Membership.find(:first, :joins => :reading, :conditions => { :readings => { :id => 2 } })

assert_equal [1, 1], @membership.id
end

Now rake test_mysql produces following error (there are 4 of them, I will show only the first one):

1
2
3
4

1) Error:
test_has_many_with_primary_key(TestAssociations):
ActiveRecord::StatementInvalid: Mysql::Error: Operand should contain 1 column(s): SELECT * FROM `readings` WHERE (`readings`.`user_id` = 1,1)
...

Well, that are the errors we are working on. Time to fix them!

Step 3. Fixing the bug

I will not explain how I fixed that, you can check my commit for details. Here is the diff:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36

diff --git a/lib/composite_primary_keys/associations.rb b/lib/composite_primary_keys/associations.rb
index 6b63664..9a9e173 100644
--- a/lib/composite_primary_keys/associations.rb
+++ b/lib/composite_primary_keys/associations.rb
@@ -180,11 +180,12 @@ module ActiveRecord::Associations::ClassMethods
raise AssociationNotSupported, "Polymorphic joins not supported for composite keys"
else
foreign_key = options[:foreign_key] || reflection.active_record.name.foreign_key
+ primary_key = options[:primary_key] || parent.primary_key
" LEFT OUTER JOIN %s ON %s " % [
table_name_and_alias,
composite_join_clause(
full_keys(aliased_table_name, foreign_key),
- full_keys(parent.aliased_table_name, parent.primary_key)),
+ full_keys(parent.aliased_table_name, primary_key)),
]
end
when :belongs_to
@@ -338,7 +339,7 @@ module ActiveRecord::Associations
@finder_sql << " AND (#{conditions})" if conditions

else
- @finder_sql = full_columns_equals(@reflection.klass.table_name, @reflection.primary_key_name, @owner.quoted_id)
+ @finder_sql = full_columns_equals(@reflection.klass.table_name, @reflection.primary_key_name, owner_quoted_id)
@finder_sql << " AND (#{conditions})" if conditions
end

@@ -386,7 +387,7 @@ module ActiveRecord::Associations
"#{@reflection.klass.quoted_table_name}.#{@reflection.options[:as]}_id = #{@owner.quoted_id} AND " +
"#{@reflection.klass.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
else
- @finder_sql = full_columns_equals(@reflection.klass.table_name, @reflection.primary_key_name, @owner.quoted_id)
+ @finder_sql = full_columns_equals(@reflection.klass.table_name, @reflection.primary_key_name, owner_quoted_id)
end

@finder_sql << " AND (#{conditions})" if conditions

Run tests to get the following output:

1
2
3
4
5
6
7

~/cpk_bug/composite_primary_keys (master)$ rake test_mysql
Using native MySQL
Started
.......................................................................................
Finished in 0.511129 seconds.

87 tests, 266 assertions, 0 failures, 0 errors

We are done for now!

Step 4. Committing changes and pulling request

It’s time to commit our changes now:

1
2
3
4
5
6
7
8
9
10
11
12
13

~/cpk_bug/composite_primary_keys (master)$ git add .
~/cpk_bug/composite_primary_keys (master)$ git commit -m 'Fixed several bugs in has_one and has_many associations when :primary_key specified'
[master 3e29891] Fixed several bugs in has_one and has_many associations when :primary_key specified
3 files changed, 31 insertions(+), 3 deletions(-)
~/cpk_bug/composite_primary_keys (master)$ git push
Counting objects: 16, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 2.14 KiB, done.
Total 9 (delta 7), reused 0 (delta 0)
To git@github.com:kpumuk/composite_primary_keys.git
050d832..3e29891 HEAD -> master
~/cpk_bug/composite_primary_keys (master)$ gh home

Okay, the next step is a pull request. Last command opened a browser window with your fork. Navigate to the latest commit and press the “Pull Request” button (at the time of writing this article gh pull-request didn’t worked, and you can try to fix by yourself to understand the workflow):

Patching the composite_primary_keys gem

That’s all, we have contributed to the community! Today Darrin Holst merged my commit into the core, and you can find it here. Not all things went smooth (I forgot to add a fixture, so tests were failing on first run), but he helped me a lot to get it working. That’s how Open Source works: we help each other to develop high quality software.

Credits

First of all, thanks to Dr Nic for the great plugin, one of the best piece of functionality I can’t imagine life without. Thanks to Darrin Holst for his patience and great help in debugging tests problem, and also for merging my commit into the composite_primary_keys core. Thanks to GitHub for the great Open Source code hosting solution, which makes working on Open Source projects so exciting.

Do you have comments or suggestions? You are welcome! Also, I will be happy if you follow me in Twitter.

Dmytro Shteflyuk's Home