views:

26

answers:

2

Just briefly, I have run into a dreaded 2(n) queries problem. If n = the number of skills in the database, then my characters#edit form will take 2(n) queries to load the page. It will SELECT a PlayerSkill (the join table) once every skill, and it will look up the Skill once per skill.

Here is some code which I believe is pertinent to the situation. In essence, the models, views, and controllers involved in this process, less the model validations and less the actions I'm not concerned about.

The controller:

  # GET /characters/1/edit
  def edit
    @character = Character.find(params[:id], :include => {:player_skills => :skill})
    stub_player_skills
  end

  private
    def stub_player_skills
      @skills = Skill.find(:all)
      @skills.each do |skill|
        if (skill.player_skills.empty?)
          ps = @character.player_skills.build(:skill_id => skill.id, :name => skill.name)
        end
      end
    end

The model(s):

class Character < ActiveRecord::Base
  belongs_to :user
  belongs_to :campaign
  has_many :sheets, :dependent => :destroy
  has_many :tokens, :dependent => :destroy

  has_many :player_skills, :dependent => :destroy
  has_many :skills, :through => :player_skills
  accepts_nested_attributes_for :player_skills, :allow_destroy => true
end

The offending view (HAML):

%h1
  Editing Character

- form_for @character do |f|
  = f.error_messages
  %p
    = f.label :name
    %br
    = f.text_field :name
  %p
    = f.label :race
    %br
    = f.text_field :race
  %p
    = f.label :char_class
    %br
    = f.text_field :char_class
  %p
    -f.fields_for :player_skills do |ps|
      =ps.object.skill.name
      =ps.text_field :level
      =ps.hidden_field :skill_id
      -unless ps.object.new_record?
        =ps.check_box '_destroy'
        =ps.label '_destroy', 'Remove'
      %br
  %p
    = f.submit

My understanding of the situation is that eager loading exists to grab the association in (roughly) a single extra query.

I need to properly apply eager loading in two areas, and I am just at a loss regarding how to do it.

In the stub_player_skills method, it needs to create a PlayerSkill object assuming the character does not already have one. It could benefit from eager loading here because it loops through each skill in the database. This is where the first "n-queries" are coming from.

Then on the view, fields_for loops through all the PlayerSkills we've racked up, because there is no way to eager load here, when I call =ps.object.skill.name to print out the skill's name, it does a Skill lookup, which brings in the second set of "n-queries."

My primary concern lies in the view layer, I cannot find any documentation (Rails API or otherwise) that states how you could eager load the associations if you're using fields_for to generate a nested form.

Thanks for any and all responses :) ~Robbie

+1  A: 

Can you try this and see if it will work?

You can keep your model the way it is.

Your controller can then look like this

def edit
  # Get all the skill objects once only
  skills = Skill.find(:all)

  # Used to extract Skill#name
  skills_hash = {}
  skills.map { |s| skills_hash[s.id] = s.name }

  # Create an array of the skill-ids
  skill_ids = skills.map { |s| s.id }

  @character = Character.find(params[:id])

  # Determine the character's missing skills
  skill_ids -= @character.player_skill_ids

  # Build all of the missing skills
  skill_ids.each do |id|
    @character.player_skills.build(:skill_id => id, :name => skills_hash[id])
  end
end
Coderama
This doesn't quite work because I still need to iterate over all possible skills (see stub_player_skills) and check if a PlayerSkill already exists.Imagine we have the Skill #1, #2, and #3. If I have Skill #1 and #3, then we need to stub out #2.I think the problem is I'm using ActiveRecord to do another "find", should I just iterate over the array in my business logic instead?
Robbie
I'm sorry, I totally missed that part. I can see what you are trying to do now... I'll revise my post.
Coderama
With slight modification I've gotten this working and the controller no longer requires n-queries to load the PlayerSkills. The view however still takes n-queries because I'm having it call ps.object.skill.name; It seems to me that this may be a limitation in how Rails handles fields_for though, which is rather unfortunate.
Robbie
Did you try adding an :include to your model? e.g. **has_many :player_skills, :dependent => :destroy, :include => :skill**
Coderama
Yes, the problem is it hits the DB for every call to "parent" from a "child.build" because they cannot be eagerly loaded (since they don't exist in the DB)I've kind of gotten around this, and negated the n-queries, by using another attribute "name" on PlayerSkill, which doesn't actually get saved.Related paste: http://pastie.org/1185550(Then the view just spits out @skills_arr, which just happens to be sorted properly)
Robbie
A: 

In case anyones interested in my "final" solution to this problem:

I've resorted to storing an array of the skill names, and referencing it in the view via a counter, as seen here:

  %p
    - index = 0
    -f.fields_for :player_skills do |ps|
      =@skill_arr[index]
      =ps.text_field :level
      =ps.hidden_field :skill_id
      -unless ps.object.new_record?
        =ps.check_box '_destroy'
        =ps.label '_destroy', 'Remove'
      - index += 1
      %br

In the controller, I've moved almost all the logic to the stub_player_skills method where it belongs, and taking a page out of Coderama's book, I've come up with this:

  private
    def stub_player_skills
      @skills = Skill.find(:all)
      @skills.each do |skill|
        skill_exists = @character.player_skills.select do |i|
          i.skill_id == skill.id
        end
        if skill_exists.empty?
          ps = @character.player_skills.build(:skill_id => skill.id, :name => skill.name)
        end
      end

      @skill_arr = @character.player_skills.map do |el|
        el.name.nil? ? el.skill.name : el.name
      end
    end

In the model layer, I just had to :include => :skill on the has_many :through relationship to get rid of a few more queries.

Robbie