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

Fix for query bind nil bug #4844

Closed
wants to merge 2 commits into from
Closed

Fix for query bind nil bug #4844

wants to merge 2 commits into from

Conversation

sferik
Copy link
Contributor

@sferik sferik commented Feb 2, 2012

In cases where the query bind is nil, exec_explain raises the following error:

NoMethodError: undefined method `empty?' for nil:NilClass

This patch changes unless bind.empty? to unless bind.blank? and adds a test case for a nil bind.

@fxn
Copy link
Member

fxn commented Feb 2, 2012

Thanks :).

In principle I am hesitant to add that check, because it may hide that some code is not passing the bindings in its instrumentation call. Binds are a collection and so when no binds are used there's a collection, empty, but not nil.

There was a recent issue related to CACHE queries that I fixed in de16100 and will ship in 3.2.2. Have you seen any other use case?

@sferik
Copy link
Contributor Author

sferik commented Feb 2, 2012

The cases where I was seeing this error appear were all using find with an :include, for example:

User.find(
  current_user.id,
  :include => [
    :credit_card,
    :user_image,
    :sent_offers => {
      :sender => :user_image,
      :receiver => :user_image,
      :listing => [:listing_image, :listing_images]
    },
    :received_offers => {
      :sender => :user_image,
      :receiver => :user_image,
      :listing => [:listing_image, :listing_images]
    }
  ]
)

@fxn
Copy link
Member

fxn commented Feb 2, 2012

I cannot reproduce this failure on 3-2-stable with user has many posts. See the session:

fxn@rails:~/tmp/test-4844 $ grep threshold config/environments/development.rb 
  config.active_record.auto_explain_threshold_in_seconds = 0
fxn@rails:~/tmp/test-4844 $ script/rails c
Loading development environment (Rails 3.2.1)
1.9.3-p0 :001 > User.find(1, :include => :posts)
  User Load (3.3ms)  SELECT "users".* FROM "users" WHERE "users"."id" = ? LIMIT 1  [["id", 1]]
  Post Load (0.5ms)  SELECT "posts".* FROM "posts" WHERE "posts"."user_id" IN (1)
  EXPLAIN (0.6ms)  EXPLAIN QUERY PLAN SELECT "users".* FROM "users" WHERE "users"."id" = ? LIMIT 1  [["id", 1]]
  EXPLAIN (0.1ms)  EXPLAIN QUERY PLAN SELECT "posts".* FROM "posts" WHERE "posts"."user_id" IN (1)
EXPLAIN for: SELECT  "users".* FROM "users"  WHERE "users"."id" = ? LIMIT 1 [["id", 1]]
0|0|0|SEARCH TABLE users USING INTEGER PRIMARY KEY (rowid=?) (~1 rows)

EXPLAIN for: SELECT "posts".* FROM "posts"  WHERE "posts"."user_id" IN (1)
0|0|0|SCAN TABLE posts (~100000 rows)
0|0|0|EXECUTE LIST SUBQUERY 1

 => #<User id: 1, created_at: "2012-01-31 07:59:45", updated_at: "2012-01-31 07:59:45"> 

Could you post a way to reproduce the error?

@iHiD
Copy link
Contributor

iHiD commented Feb 10, 2012

I was just getting this issue on 3.2.1. I changed to 3-2-stable and it's fixed it. Good work.

@fxn
Copy link
Member

fxn commented Feb 10, 2012

@iHiD excellent, thanks very much for the feedback.

The CACHE notification have binds now (and anyway we are no longer monitoring CACHE notifications). So I think we can close this issue.

@sferik if you find this error again please feel free to reopen.

@fxn fxn closed this Feb 10, 2012
@sferik
Copy link
Contributor Author

sferik commented Feb 10, 2012

Sounds good. Thanks for following up.

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