Failing test raises NoMethodError

[bf4]

No description provided.

[brendon]

I still think we need to figure out exactly why rearrange_ranks is being called on an empty list. That seems to me to be the only reason current_first would return nil.

This is the only code that would rearrange_ranks:

    def assure_unique_position
        if ( new_record? || rank_changed? )
          if (rank > RankedModel::MAX_RANK_VALUE) || current_at_rank(rank)
            rearrange_ranks
          end
        end
      end

Let me know if I don't make sense :)

[bf4]

I wrote up some thoughts in #148 (comment)

Would you prefer to discuss here?

[brendon]

current_at_rank(rank) is definitely for determining if there is an existing item at the current rank in this instance. Its name works well for finding the current item at that rank but as you say, the method is unused. I'm about to do a commit that renames this method to rank_taken?. It was also superfluous to pass in the rank anymore because it's accessible on the instance anyway.

def rank_taken?
  finder.except(:order).where(ranker.column => rank).exists?
end

Are you able to examine any SQL logs you get from requests where your error occurs? It'd be interesting to see how we get down that code path with an empty list. I do have one hunch:

1. Assume an empty scope
2. update_index_from_position is called. position is :first.
3. current_first evaluates and is cached to nil.
4. position_at :middle is therefore called.
5. update_index_from_position is called. position is :middle.
6. rank_at_average sets the rank to 1 or 0, I forget how the math plays out.
7. assure_unique_position is called and new_record? is true.
8. ...meanwhile another request, somehow faster than this one has executed and added the first record to the scope (thus with a position the same as the one we're about to set).
9. Our simplified rank_taken? method (above), since it modifies the arel query, now executes new SQL that evaluates to true. This triggers a rearrange.
10. rearrange_ranks is called. Early on current_first is called. Since this was cached earlier on to be nil it returns this now and we get our error trying to call .rank on it.

The only flaw in this logic is that the caching for current_first is @current_first ||= begin. If the instance variable is nil the block will be run again and should return the actual first item already inserted.

I guess what I'm trying to say is that I'd like to figure out why we're getting this error through concrete reproducible example in the real world. I've had a look at your test, but I can't get my head around how it's managing to manifest the error. Can you outline the code flow for me that would result in the error (at bit like I've done above?).

I'm not trying to be a pain :) I'd just like to see if there's a more elegant solution to the problem.

[bf4]

@brendon we have it happen maybe once a month. I have put f095445 into prod in our app so that maybe I'll pull in something useful in the logs. In the original issue I put in the timing information from the logs. I don't know arel enough to know if it's possibly a query cache issue or something else going on that makes the two queries that look so similar return different truth

[brendon]

Thanks @bf4, hopefully that will yield some better information. I think at a minimum we probably need to re-look at memoization in these methods as that in some ways doesn't make the best of sense :)