Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise an exception when model without primary key calls .find_with_ids #12549

Merged
merged 1 commit into from Oct 21, 2013

Conversation

makimoto
Copy link
Contributor

I fixed exception handling for .find of models without primary key.

Now .find by models without primary key raises StatementInvalid exception by the invalid SQL query, but I think an exception should be raised before building a query.

irb(main):001:0> Post.find(1)
  Post Load (0.3ms)  SELECT  `posts`.* FROM `posts`  WHERE `posts`.`` = 1 LIMIT 1
Mysql2::Error: Unknown column 'posts.' in 'where clause': SELECT  `posts`.* FROM `posts`  WHERE `posts`.`` = 1 LIMIT 1
ActiveRecord::StatementInvalid: Mysql2::Error: Unknown column 'posts.' in 'where clause': SELECT  `posts`.* FROM `posts`  WHERE `posts`.`` = 1 LIMIT 1
    from /Users/makimoto/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/bundler/gems/rails-dd37ff81d131/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:293:in `query'

By this PR AR::UnknownPrimaryKey are raised when Model.primary_key is nil and Model.find calls.

irb(main):001:0> Post.find(1)
ActiveRecord::UnknownPrimaryKey: Unknown primary key for table posts in model Post.
    from /Users/makimoto/.rbenv/versions/2.0.0-p247/lib/ruby/gems/2.0.0/bundler/gems/rails-dd37ff81d131/activerecord/lib/active_record/relation/finder_methods.rb:301:in `find_with_ids'

Toy.primary_key = nil
begin
Toy.find(1)
rescue ActiveRecord::UnknownPrimaryKey => e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do yu think about using: assert_raises instead of rescue?

@pftg
Copy link
Contributor

pftg commented Oct 15, 2013

To complete PR needs to add Changelog entry.

@makimoto
Copy link
Contributor Author

@pftg done. thanks!

@@ -860,6 +860,15 @@ def test_find_one_message_with_custom_primary_key
Toy.reset_primary_key
end

def test_find_without_primary_key
Toy.primary_key = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use one of the existing models, that does in fact not have a primary key? Maybe Matey?

EDIT: This way we don't need to mess with global state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know Matey.
I use it. 17c08fd

@senny
Copy link
Member

senny commented Oct 21, 2013

@makimoto the PR does not apply, can you push a rebased version?

@makimoto
Copy link
Contributor Author

ok, rebased and pushed.

senny added a commit that referenced this pull request Oct 21, 2013
Raise an exception when model without primary key calls .find_with_ids
@senny senny merged commit 7364156 into rails:master Oct 21, 2013
@senny
Copy link
Member

senny commented Oct 21, 2013

@makimoto thank you for your contribution 💛

@makimoto makimoto deleted the raise-when-find-without-pk branch October 21, 2013 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants