Skip-reranking when current_first/current_last is nil

[bf4]

A take on 'first do no harm' when seeing the occasional

NoMethodError: undefined method `rank' for nil:NilClass (Most recent call first)


File /app/vendor/bundle/ruby/2.6.0/gems/ranked-model-0.4.4/lib/ranked-model/ranker.rb line 188 in rearrange_ranks

There are various possible strategies for handling a missing 'current_x'

• reset cache and retry
• make a best effort with what we know
• calculate 'freshness' of the current instance and do something when not fresh
  • have a pluggable strategy when no longer fresh
• try to lock the re-reranking

Related

• undefined method `rank' for nil:NilClass #124
• Make rearrange_ranks resilient to race conditions #117

Work done:

• just the failing test Failing test raises NoMethodError #149
• two commits: 1. failing test, 2. make it pass
  • strategy 1: skip cannot rearrange (this PR).
  • strategy 2: abort save Abort save when cannot re-arrange #151
• unrelated misc Misc touchups #150

[brendon]

My preference would be locking, perhaps with an advisory lock if you wanted to investigate that. Does this problem still occur with the latest version of ranked-model?

[brendon]

Sorry, obviously it does since that's mentioned in your error :)

How do we end up with no current_first? It would imply that finder.first returns nil which would be an empty list?

[bf4]

@brendon Here's our ranked model

class JobProductionPlanSegment < ApplicationRecord
  include RankedModel
  ranks :sequence, with_same: :job_production_plan_id

The migration for it specified

 t.integer :sequence, null: false

The database is pg 11

Here are some logs from a recent occurrence

all requests below are POST to /v1/job-production-plan-segments with the same job_production_plan_id

timestamp	server	pid	thread	post response status	post body sequence-position	notes
2019-07-21T13:37:26.112Z	web.1	451	70057487585260	201	0
2019-07-21T13:37:26.149Z	web.1	836	70057503954640	500	1	failed
2019-07-21T13:37:26.234Z	web.1	451	70057487580040	201	3
2019-07-21T13:37:26.352Z	web.1	836	70057504046960	201	2
2019-07-21T13:37:26.456Z	web.1	451	70057487574620	201	4
2019-07-21T13:37:35.848Z	web.1	451	70057487561080	201	1	appears to be a successful retry of the failed request

so maybe sequence-position 0 hadn't been persisted yet?

[bf4]

returning nil as implemented will skip-reranking

alternatively, could have a strategy

fail RankedModel::Unrankable, message, instance

Or, since we're in a before_save we can 'throw :abort' in the later rails versions, like

  errors.add ranker.column, "Could not re-rank columns: #{message}"
  throw :abort

[bf4]

see #151

[bf4]

@brendon on reflection I decide it made sense to make failing the 'before-save' hook to be a first class thing, and then used that as the default strategy for handling current-first or current-last being missing.

If you want to point me to where the appropriate test would go and give a few pointers, I'd be happy to add it.

[bf4]

OMG, I wrote a failing test and made it pass. But I don't think my code style is right for the repo.

[bf4]

This is otherwise nil. It's need to be truthy for rearrange_ranks to be called

[bf4]

return value never seems to need to be anything but truthy since it was introduced 8280511

[brendon]

That's true. Perhaps it was intended for future use. For it to return a boolean you'd probably want to implement a current_at_rank? method.

[bf4]

@brendon So, the reason I'm looking at current_at_rank at all is because I wanted to reproduce the problem I experienced: creating a record, specifying a position, rearrange ranks failing due to current_first being nil

In order to do that, I discovered that in order for rearrange_ranks to be called in my situation, current_at_rank had to be truthy. I looked at the method and determined, for mocking purposes, I didn't need to care about what it was doing or returning

I was a little curious about current_at_rank since I didn't see it used anywhere else or directly tested. So, I looked through the version history and found as noted above that it was introduced to replace the condition (current_order.find do |rankable| rankable.rank.nil? || rankable.rank == rank end) which confirmed that its usage in assure_unique_position is only to return something truthy.

Interestingly, it's truthiness was replaced by finder.except( :order ).where( ranker.column => rank ).first where 'finder' is more or less equivalent to 'current_order'

now, why would current_at_rank(rank) be truthy when current_first is nil, when current_first is defined as finder.first. I'm not sure, but it could be a race condition or a query cache issue or related to the except call (though I doubt it)

For the purpose of reproducing the failure and testing fixes, I think my test and and prs are sufficient

[bf4]

I'm not sure exactly what the intended meaning of current_at_rank is, or if it changed during the refactor. I think it is that another record exists?

[brendon]

Let's continue the discussion here: #149

[brendon]

Hi @bf4, this is going to take me a while to look through. At first glance it seems like a mildly ungraceful solution to the problem that seems to deal with the symptoms rather than trying to figure out the cause. Nonetheless I'll star this to look at once I've cleared my other backlog of work :D

[bf4]

@brendon In that case, allow me to break this up into smaller prs

• just the failing test Failing test raises NoMethodError #149
• two commits: 1. failing test, 2. make it pass
  • strategy 1: skip cannot rearrange (this PR).
  • strategy 2: abort save Abort save when cannot re-arrange #151
• unrelated misc Misc touchups #150

[bf4]

Closing as stale