undefined method `<' for nil:NilClass ranker.rb:215:in `rebalance_ranks'

nbcraft commented 4 years ago

Fixed by https://github.com/mixonic/ranked-model/pull/160 Waiting for new release superior than v0.4.4

Hello,

I'm experiencing an issue when changing the scope and sending to the first position at the same time for 2 rows inside a transaction, and I'm suspecting it's because I'm around MIN_RANK_VALUE.

I don't know if I'm doing something wrong of if this is a legitimate bug, here's the information I've gathered so far:

My situation:

Table creation:

t.bigint :row_order, null: false

Model

include RankedModel
ranks :row_order, with_same: %i[scope_1 scope_2]

All rows with scope_1 = 114

id      row_order   scope_1 scope_2
12470   -2147483646 114     18
12464   -2147483644 114     18
12463   429496729   114     18
12454   1288490188  114     18
12462   -2147352576 114
12460   -1879048192 114
12459   -1610612736 114
12465   -1342177280 114
12458   -1073741824 114
12445   -536870912  114
14525   0               114

What I'm doing:

Inside a transaction:

Updating 12459 with scope_2 = 18 and row_order_position = :first
Updating 12462 with scope_2 = 18 and row_order_position = :first

My Code (renamed):

MyClass.transaction do
  my_instances # [MyClass<#12462>, MyClass<#12459>]
    .reverse_each do |my_instance|
      my_instance.lock!
      my_instance.update!(scope_2: Scope2<id: 18>, row_order_position: :first)
    end
end

The crash

The first update (for id: 12459) seems to go through, but the second one fails here: (And then everything gets rolled back)

[211, 219] in /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model/ranker.rb
   211:       def rebalance_ranks
   212:         if rank && instance.persisted?
   213:           origin = current_order.index { |item| item.instance.id == instance.id }
   214:           destination = current_order.index { |item| rank <= item.rank }
=> 215:           destination -= 1 if origin < destination
   216:
   217:           current_order.insert destination, current_order.delete_at(origin)
   218:         end
   219:
(byebug) current_order.map { |item| item.instance.id }
[12459, 12470, 12464, 12463, 12454]
(byebug) instance.id
12462
(byebug) origin
nil
(byebug) destination
3
(byebug) origin < destination
*** NoMethodError Exception: undefined method `<' for nil:NilClass

nil

StackTrace

Click to Reveal StackTrace

``` undefined method `<' for nil:NilClass /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model/ranker.rb:215:in `rebalance_ranks' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model/ranker.rb:169:in `rank_at_average' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model/ranker.rb:125:in `update_index_from_position' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model/ranker.rb:60:in `handle_ranking' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model.rb:33:in `block in handle_ranking' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model.rb:32:in `each' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model.rb:32:in `handle_ranking' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:429:in `block in make_lambda' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:201:in `block (2 levels) in halting' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:607:in `block (2 levels) in default_terminator' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:606:in `catch' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:606:in `block in default_terminator' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:202:in `block in halting' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:514:in `block in invoke_before' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:514:in `each' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:514:in `invoke_before' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:134:in `run_callbacks' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activesupport-6.0.2/lib/active_support/callbacks.rb:827:in `_run_save_callbacks' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/callbacks.rb:328:in `create_or_update' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/timestamp.rb:129:in `create_or_update' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/persistence.rb:503:in `save!' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/validations.rb:53:in `save!' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:319:in `block in save!' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/connection_adapters/abstract/database_statements.rb:279:in `transaction' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:212:in `transaction' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:366:in `with_transaction_returning_status' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:319:in `save!' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/suppressor.rb:48:in `save!' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/persistence.rb:635:in `block in update!' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/connection_adapters/abstract/database_statements.rb:279:in `transaction' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:212:in `transaction' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/transactions.rb:366:in `with_transaction_returning_status' /Users/nate/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/activerecord-6.0.2/lib/active_record/persistence.rb:633:in `update!' ```

Gut feeling:

I don't have a good grasp of what the gem is trying to accomplish in that method but, it seems like I'm only getting in that method because:

my first update (for id: 12459) must set row_order to be before element id: 12470 (which has row_order -2147483646), so it must end up being -2147483647 or -2147483648.
But now my second update (for id: 12462) must do the same thing, and would probably need row_order -2147483648 or below which is the MIN_RANK_VALUE, which probably triggers the call to rebalance_ranks
current_order does not have the current instance so origin is nil, and we get the crash at the comparison

=> I feel like the fix might be something along the lines of:

pre-adding the current instance to current_order when the scope is changing at the same time.
adding a check on origin.nil?
casting origin.to_i to get nil => 0, or maybe it should take a higher value than destination then? Not quite sure.

Anyways I hope this is clear enough and helpful, let me know if you'd like more information or clarification.

Thank you 🙏

brendon commented 4 years ago

Hi @nbcraft, thank you for the thorough bug report. We actually fixed this recently bu using your second suggested fix. Can you try with master and let me know if the issue is fixed for you?

nbcraft commented 4 years ago

Hi @brendon , thank you for that information, it does fix the issue! We'll be using the latest commit until a new version gets released.

Thanks a lot.

brendon commented 4 years ago

Thanks Nathan, I'll do a new release for you :)

brendon / ranked-model