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

Wrong Eager Loading Detected if use before_all or let_it_be #666

Open
chaomao opened this issue Aug 15, 2023 · 2 comments
Open

Wrong Eager Loading Detected if use before_all or let_it_be #666

chaomao opened this issue Aug 15, 2023 · 2 comments

Comments

@chaomao
Copy link

chaomao commented Aug 15, 2023

Hi, zm

recently I start refactoring our test, move large object creation from let to let_it_be, which will change object creation from before_each hook into before_all hook, then I found some tests failed due to eager loading detected, like below

RSpec.describe Post, type: :model do
  # post has_many comments
  # comment belongs_to post
  # creator belongs_to comment
  let_it_be(:post) { create(:post) }
  let_it_be(:comment) { create(:comment, post: post)}

  # before(:all) do # same as let_it_be
  #   puts "start before all"
  #   @post = create(:post)
  #   @comment = create(:comment, post: @post)
  #   puts "end before all"
  # end

  it 'ok' do
    creator = create(:creator, comment_id: comment.id)
    comment.post # ok
  end

  it 'raise N+1 query issue' do
    creator = create(:creator, comment: comment)
    comment.post # will raise eager loading Comment => [:post]
  end
end

which will raise error USE eager loading detected, Comment => [:post], Add to your query: .includes([:post]), reproducable repo is here. I don't think the detection is correct :(

I read the source code of bullet and found the issue is the object created in before_all hook didn't add into Bullet::Detector::NPlusOneQuery::impossible_objects, because Bullet.start_request is invoked in before_each hook.

# n_plus_one_query.rb
        def add_impossible_object(object)
          return unless Bullet.start? # will return when object creation in `before_all` hook
          return unless Bullet.n_plus_one_query_enable?
          return unless object.bullet_primary_key_value

          Bullet.debug('Detector::NPlusOneQuery#add_impossible_object', "object: #{object.bullet_key}")
          impossible_objects.add object.bullet_key
        end

# spec_helper.rb, will start Bullet in before_each hook.
if Bullet.enable?
  RSpec.configure do |config|
    config.before do
      Bullet.start_request
    end

    config.after do
      Bullet.perform_out_of_channel_notifications if Bullet.notification?
      Bullet.end_request
    end
  end
end

so my questions are:

  1. is there a way to avoid eager loading detected error when I use before_all or let_it_be? I really want to move some large object creation out of each test, which will significantly improve performance
  2. no matter the problem is solved or not, I hope the documentation can cover this scenario, it may help other people on it :)
@flyerhzm
Copy link
Owner

@chaomao The solution comes to my mind is

  1. preserve all bullet objects, possible_objects, impossible_objects, etc., after before_all
  2. restore all bullet objects before before_each or each test.
  3. reset all bullet objects except preserved objects after each test or after_each.
  4. reset all preserved bullet objects after after_all.

But it's not simple and I'm not sure if I have time to do it right now.

Or you can skip the bullet request for some tests which contains before_all or let_it_be.

@chaomao
Copy link
Author

chaomao commented Aug 24, 2023

thanks for the reply, I will skip bullet request for some tests for now :)

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

No branches or pull requests

2 participants