ActiveRecord extension to find next / previous record(s) in one query

Question

I just wrote this gem order_query to find next or previous records relative to the record assuming a (possibly complex) ORDER BY over the records. For example, this is needed to show a link to the next result from the current search result page.

The gem is used like so:

class Issue < ActiveRecord::Base
  include OrderQuery
  order_query :order_display, [
    [:priority, %w(high medium low)],
    [:valid_votes_count, :desc, sql: '(votes - suspicious_votes)'],
    [:updated_at, :desc],
    [:id, :desc]
  ]
  def valid_votes_count
    votes - suspicious_votes
  end
end

Issue.order_display         #=> ActiveRecord::Relation<...>
Issue.reverse_order_display #=> ActiveRecord::Relation<...>

p = Issue.find(31).order_display(scope) # scope default: Issue.all
p.items_before  #=> ActiveRecord::Relation<...>
p.prev_item     #=> Issue<...>
p.position      #=> 5
p.next_item     #=> Issue<...>
p.items_after   #=> ActiveRecord::Relation<...>

Read more

The code gets a complex method warning from CodeClimate, and I agree: it does not look easy to comprehend.

Can this be done better?

This is the code that constructs the query:

# @param [:before or :after] mode
def build_query(mode)
  # The next element will be the first one among elements with lesser order
  build_query_factor(
      order.map { |o| where_relative(o, mode) },
      order.map { |o| where_eq(o) }
  )
end

# @param [Array] x query conditions
# @param [Array] y query conditions
# @return [query, query_args] The resulting query is as follows:
#   x0 | y0 &
#      (x1 | y1 &
#        (x2 | y2 &
#          (x3 | y3 & ... )))
#
# Explanation:
#
#   To narrow the result to only the records that come before / after the current one, build_query passes
#   the values of x and y so that:
#
#   x matches order criteria with values that come after the current record.
#   y matches order criteria with values equal to the current record's value, for resolving ties.
#
def build_query_factor(x, y, i = 0, n = x.length)
  q = []

  x_cond = [x[i][0].presence, x[i][1]]
  q << x_cond if x_cond[0]

  if i >= 1
    q << ['AND'] << y[i - 1]
  end

  if i < n - 1
    q << ['OR'] if x_cond[0]
    nested = build_query_factor(x, y, i + 1)
    q << ["(#{nested[0]})", nested[1]]
  end

  [q.map { |e| e[0] }.join(' '),
   q.map { |e| e[1] }.compact.reduce(:+) || []]
end

EMPTY_FILTER = ['', []]

def where_eq(spec)
  ["#{spec.col_name_sql} = ?", [values[spec.name]]]
end

# @param [:before or :after] mode
def where_relative(spec, mode)
  ord   = spec.order
  value = values[spec.name]
  if ord.is_a?(Array)
    # ord is an array of values, ordered first to last, e.g.
    # all up to current
    pos    = ord.index(value)
    values = mode == :after ? ord.from(pos + 1) : ord.first(pos) if pos
    # if current not in result set, do not apply filter
    return EMPTY_FILTER unless values.present?
    ["#{spec.col_name_sql} IN (?)", [values]]
  else
    # ord is :asc or :desc
    op = {before: {asc: '<', desc: '>'}, after: {asc: '>', desc: '<'}}[mode][ord || :asc]
    ["#{spec.col_name_sql} #{op} ?", [value]]
  end
end

Answer 1

I will stick to reviewing the code you've posted, although it seems tied to other code.

First of all: Use descriptive variable names. x, y, q, and so forth aren't great for legibility. Use query, position and similar full words. Especially since you're dealing with complex nested arrays.

There also seems to be some logical issues/dangerous assumptions. I don't know if you check/sanitize your input elsewhere, but if you don't things will get weird. E.g.:

# This will fail if x[i] is nil
x_cond = [x[i][0].presence, x[i][1]]

q << x_cond if x_cond[0]

if i >= 1
  # 1. This *may* append ["AND"] etc. directly to an empty array,
  #    if x_cond[0] was nil above, resulting in a nonsense query.
  # 2. This will fail if y.count < i - 1
  q << ['AND'] << y[i - 1]
end

In other words, if x_cond[0] isn't present, you don't append anything to the array. Yet the following if block assumes that something definitely was appended. And there's no check for the length of y so y[i - 1] may fail. And x[i][0].presence will fail if x[i] is nil to begin with.

I also see some (dangerous) redundancy, like the n = x.length method parameter. There's no reason for this to be a method parameter. In fact, it's detrimental, because the n value is never checked, so you can pass any value like n = -42 which wouldn't make sense.

Moreover, the code doesn't actually match its description: Your comment for build_query_factory states that the result should be something like:

x0 OR y0 AND (x1 OR y1 AND (x3 OR y3))

but the method returns:

x1 OR (x2 AND y1 OR (x3 AND y2))

Notice that a) the structure is very different, and b) y3 doesn't even appear.

---

Now, assuming that you want the result you give as an example in the comment (and not the result the method actually returns), I would do something like this:

def build_nested_query(x, y)
  # zip the two arrays (x, y, x, y, x, y, ...)
  ordered = x.zip(y)

  # split into conditions and parameters
  ordered = ordered.map do |x, y|
    [ [x.first, y.first], [x.second, y.second] ] # still confusing, to be honest
  end

  conditions = ordered.map(&:first)
  parameters = ordered.map(&:second).flatten # thanks to ActiveSupport for Array#second, by the way

  [ nest_conditions(conditions) , parameters ]
end

def nest_conditions(conditions)
  # create "x OR y" string
  branch = conditions.first
  branch = branch.join(" OR ")

  # nest the remaining recursively, appending them with " AND "
  remaining = conditions[1..-1]
  if remaining.any?
    nested = nest_conditions(remaining)
    branch += " AND (#{nested})"
  end

  branch
end

That'll do something like this:

x = [["x1", [:x1]], ["x2", [:x2]], ["x3", [:x3]]]
y = [["y1", [:y1]], ["y2", [:y2]], ["y3", [:y3]]]

query = build_nested_query(x, y)
query[0] #=> "x1 OR y1 AND (x2 OR y2 AND (x3 OR y3))"
query[1] #=> [:x1, :y1, :x2, :y2, :x3, :y3]

Which I believe is what you want. You'll want to do some sanity-checking/sanitizing of x and y beforehand, to make sure they make sense.

Of course, if you find a better way to supply the method parameters in the first place, the code can be cleaned up even more.

I would also consider encapsulating conditions and parameters somehow to avoid the non-descriptive [0] and [1] (or first/second) stuff in favor of something more descriptive (Rails no doubt already has an internal class for this structure. Otherwise, a simple Struct-based class should do.)

---

I haven't looked closely at where_relative, but again I see some strange logic:

def where_relative(spec, mode)
  ord = spec.order

  # I assume `values` is an instance variable?
  # Anyway, why assign `value` here? It's only used in
  # the else-block, and so should be assigned there
  value = values[spec.name]

  if ord.is_a?(Array)

    # ...

    # Wait, now you're assigning something to `values`?
    values = mode == :after ? ord.from(pos + 1) : ord.first(pos) if pos

    # and apparently, that something can be nil, since you're
    # checking for that here - but you didn't check for that
    # before you called `values[spec.name]` above
    return EMPTY_FILTER unless values.present?

    # ...

  else

    # ...

    # And here you use `value` (i.e. values[spec.name])
    ["#{spec.col_name_sql} #{op} ?", [value]]

  end
end

Now, I admit I don't know the exact sequence of events, but the above looks/smells like dangerous assumptions and nasty side effects. If the method is called several times per object instantiation, results get unpredictable. First time it's called, I assume values is set to something. But it might get set to nil by the method. This will cause the next call to the method to crash on values[spec.name]. If the method is so completely dependent on values, it should be a parameter.

Lastly, this line should really be reworked:

values = mode == :after ? ord.from(pos + 1) : ord.first(pos) if pos

Don't use a ternary and postfixed if on the same line - it's very confusing.