0

I have a simple association like

class Slot < ActiveRecord::Base
  has_many :media_items, dependent: :destroy
end

class MediaItem < ActiveRecord::Base
  belongs_to :slot
end

The MediaItems are ordered per Slot and have a field called ordering. And want to avoid n+1 querying but nothing I tried works. I had read several blogposts, railscasts etc but hmm.. they never operate on a single model and so on...

What I do is:

def update
  @slot = Slot.find(params.require(:id))

  media_items = @slot.media_items
  par = params[:ordering_media]
  # TODO: IMP remove n+1 query
  par.each do |item|
    item_id = item[:media_item_id]
    item_order = item[:ordering]
    media_items.find(item_id).update(ordering: item_order)
  end
  @slot.save
end

params[:ordering_media] is a json array with media_item_id and an integer for ordering I tried things like

@slot = Slot.includes(:media_items).find(params.require(:id)) # still n+1
@slot = Slot.find(params.require(:id)).includes(:media_items) # not working at all b/c is a Slot already
media_items = @slot.media_items.to_a # looks good but then in the array of MediaItems it is difficult to retrieve the right instance in my loop

This seems like a common thing to do, so I think there is a simple approach to solve this. Would be great to learn about it.

einSelbst
  • 2,099
  • 22
  • 24
  • What approach do you prefer: fill an order field for each media item at the slot form, or drag and drop media items on slot show? – Alejandro Babio Oct 23 '14 at 11:50

1 Answers1

1

First at all, at this line media_items.find(item_id).update(ordering: item_order) you don't have an n + 1 issue, you have a 2 * n issue. Because for each media_item you make 2 queries: one for find, one for update. To fix you can do this:

params[:ordering_media].each do |item|
  MediaItem.update_all({ordering: item[:ordering]}, {id: item[:media_item_id]})
end

Here you have n queries. That is the best we can do, there's no way to update a column on n records with n distinct values, with less than n queries.

Now you can remove the lines @slot = Slot.find(params.require(:id)) and @slot.save, because @slot was not modified or used at the update action.

With this refactor, we see a problem: the action SlotsController#update don't update slot at all. A better place for this code could be MediaItemsController#sort or SortMediaItemsController#update (more RESTful).

At the last @slot = Slot.includes(:media_items).find(params.require(:id)) this is not n + 1 query, this is 2 SQL statements query, because you retrieve n media_items and 1 slot with only 2 db calls. Also it's the best option.

I hope it helps.

Alejandro Babio
  • 5,189
  • 17
  • 28
  • Very helpful and great explanation. I had to use the following syntax for *update_all* ```MediaItem.where(id: item[:media_item_id]).update_all(ordering: item[:ordering])``` and I can see in the console that the *Load* statements are missing. What you say about refactoring makes sense, but right now the only way to interact with mediaItems is via slot. It's just an API, no rendered pages/forms. – einSelbst Oct 27 '14 at 09:46