brendon / ranked-model

An acts_as_sortable/acts_as_list replacement built for Rails 4+
https://github.com/mixonic/ranked-model
MIT License
1.09k stars 134 forks source link

N+1 requests when outputting rank values #186

Closed mrcasals closed 2 years ago

mrcasals commented 2 years ago

Hi!

Imagine we have a list of ducks with row_order rank (example taken from the changelog. Then we run this code:

ducks = Ducks.all
ducks.map do |duck|
  {
    id: duck.id,
    position: duck.row_order_rank
  }
end

The duck.row_order_rank generates an N+1, because it fetches the position from the DB for every single element. I tried adding Duck.rank(:row_order).all, but it doesn't precalculate the ranks.

Is there any way to avoid this N+1? I checked the readme and the code and couldn't find anything, but I might have missed something.

Thanks! ❤️

brendon commented 2 years ago

Hi @mrcasals, you're right there. The call boils down to this code:

      def relative_rank
        escaped_column = instance_class.connection.quote_column_name ranker.column

        finder.where("#{escaped_column} < #{rank}").count(:all)
      end

If you're fetching all the ducks in a scope (ordered) then you'd know their relative positions implicitly by their order in the returned enumerable. Would this be enough?

In your example you're ignoring any scopes that might exist and also ordering, so you'd get some interesting output :)

mrcasals commented 2 years ago

I know I could sort them with Duck.rank(:row_order), but using that I'd expect the row_order_rank attribute to be set. I could get the order from the collection, but then we get to this case:

ducks = Duck.rank(:row_order).find(305)
{
  id: duck.id,
  position: duck.row_order_rank
}

This still calls the DB twice.

Is there any way to define a scope (or reuse rank(:row_order)) that automatically fills the row_order_rank attribute?

brendon commented 2 years ago

Unfortunately as it stands there is no way to know a duck's rank without counting all the ducks before it. The rank provides no clue as it's sparse. Perhaps there's a way to generate a pseudo-column that is the order of that row in the results, but then you'd have to be very sure that you always selected all the records.

mrcasals commented 2 years ago

Mhhhh I see. Do you think it's worth adding this to the Readme file? It might be something to consider in some use cases 😊

brendon commented 2 years ago

Sounds good to me :) Would you be keen to put together a PR with that change?

mrcasals commented 2 years ago

@brendon done! See #187 😄