Losing sort order when decorate records with draper

[zoer]

response = Article.search(q:"test").page(1).limit(20)
decorated = ArticleDecorator.decorate_collection(response.records)

decorated.map(&:id) == response.records.to_a.map(&:id) # => false

Seems this happened because map method is not delegated to the records:

class Records
  include Enumerable

  delegate :each, :empty?, :size, :slice, :[], :to_a, :to_ary, to: :records
  ...

https://github.com/elasticsearch/elasticsearch-rails/blob/v0.1.4/elasticsearch-model/lib/elasticsearch/model/response/records.rb#L13

But draper use map for decorate collections:

module Draper
  class CollectionDecorator
     ...

     # @return [Array] the decorated items.
    def decorated_collection
      @decorated_collection ||= object.map{|item| decorate_item(item)}
    end

https://github.com/drapergem/draper/blob/v1.3.1/lib/draper/collection_decorator.rb#L39

I just fixed this by override decorated_collection

# config/initializers/draper.rb
require "draper"
class Draper::CollectionDecorator
  def decorated_collection
    @decorated_collection ||= object.to_a.map{|item| decorate_item(item)}
  end
end

This is not an issue, but I think you should know about this.

[karmi]

Hey, so let's stick with the first problem -- the records collection should definitely have the map method.

response.records.map
=> #<Enumerator: ...>

I'm all for Draper support!

[zoer]

Sorry, forgot that I'm using kaminari and page method. I changed example and implemented the test:

context "Mongoid integration with kaminari" do
  class ::MongoidArticleKaminari < ::MongoidArticle
    include ::Kaminari::ConfigurationMethods
  end

  setup do
    Elasticsearch::Model::Adapter.register \
      Elasticsearch::Model::Adapter::Mongoid,
      lambda { |klass| !!defined?(::Mongoid::Document) && klass.ancestors.include?(::Mongoid::Document) }

    MongoidArticleKaminari.__elasticsearch__.create_index! force: true

    MongoidArticleKaminari.delete_all

    10.times do
      MongoidArticleKaminari.create! title: "test "*rand(1..5)
    end

    MongoidArticleKaminari.__elasticsearch__.refresh_index!
    MongoidArticleKaminari.__elasticsearch__.client.cluster.health wait_for_status: 'yellow'
  end

  should "not lose sort order on map" do
    records = MongoidArticleKaminari.search(query: {match: {title: {query: 'test'}}}).page(1).limit(100).records

    assert_equal records.map(&:id) , records.to_a.map(&:id)
  end
end

Output:

  FAIL (0:00:41.941) test_: Mongoid integration with kaminari should not lose sort order on map.
    <[BSON::ObjectId('53c53a0d7661673091000000'),
     BSON::ObjectId('53c53a0d7661673091010000'),
     BSON::ObjectId('53c53a0d7661673091020000'),
     BSON::ObjectId('53c53a0d7661673091030000'),
     BSON::ObjectId('53c53a0d7661673091040000'),
     BSON::ObjectId('53c53a0d7661673091050000'),
     BSON::ObjectId('53c53a0e7661673091060000'),
     BSON::ObjectId('53c53a0e7661673091070000'),
     BSON::ObjectId('53c53a0e7661673091080000'),
     BSON::ObjectId('53c53a0e7661673091090000')]> expected but was
    <[BSON::ObjectId('53c53a0d7661673091050000'),
     BSON::ObjectId('53c53a0d7661673091000000'),
     BSON::ObjectId('53c53a0e7661673091070000'),
     BSON::ObjectId('53c53a0e7661673091060000'),
     BSON::ObjectId('53c53a0d7661673091030000'),
     BSON::ObjectId('53c53a0d7661673091040000'),
     BSON::ObjectId('53c53a0e7661673091090000'),
     BSON::ObjectId('53c53a0e7661673091080000'),
     BSON::ObjectId('53c53a0d7661673091010000'),
     BSON::ObjectId('53c53a0d7661673091020000')]>.

[karmi]

This is weird, do you have any idea why records.map(&:id) and records.to_a.map(&:id) would be different in Mongoid?

[zoer]

In my opinion that happening because map method on the records object, not delegated to to_a method which you creating on the mongoid criteria.

I could be wrong, but on debugging he just do not go in.

[zoer]

And I remind, this problem occurs only when kaminari page and limit methods are used.

[fabiob]

I'm facing the same issue, on the same setup (Mongoid + Kaminari). Anyone though of any workaround yet?

Edit: found a workaround: I'm calling #to_a myself on the view (instead of just #each).

[karmi]

In my opinion that happening because map method on the records object (...)

Still don't get it -- map is implemented via each, which is delegated to records, and records have the to_a method implemented in the part you posted.

[dwkoogt]

Hi. I ran into this problem while I was working on to_a issue with mongoid (#99) a while back.
The issue is that records method will retrieve from mongo with array of IDs (where clause) and the result does not honor the order in the IDs array. That is why to_a method is resetting the order by calling sort with hit index on the db result. This is all in mongoid adapter.

Here's what you have to do:

class ObjectsDecorator < PaginationDecorator
  def decorated_collection
    @decorated_collection ||= object.to_a.map{|item| decorate_item(item)}
  end
end

#And PaginationDecorator is something you should be familiar with:

class PaginationDecorator < Draper::CollectionDecorator
  delegate :current_page, :prev_page, :next_page, :total_pages, :limit_value
end

[dwkoogt]

I dug in a little more and I don't think map is implemented via each (could you point me out where?).
The method each is delegated to records, yes. And the records includes an adapter class which in this case is Elasticsearch::Model::Adapter::Mongoid. But method each is not defined in none of these. It eventually is delegated to what the records method returns and that is Mongoid::Contextual::Mongo through Mongoid::Criteria. Within that class, both each and map methods are implemented. And you can see that both iterate over db result. Therefore you have to call to_a before calling each or map when you get records with elasticsearch result (see each_with_hit and map_with_hit). You can do some experiments by checking query results and their scores.

[karmi]

@zoer So, I was digging into this... The problem appears to be that map indeed has to call to_a first, so our "resorting" logic kicks in.

As soon as I add this to the Mongoid adapter:

def map(&block)
  records.to_a.map(&block)
end

your tests pass. Of course, that is the same as adding this:

def each(&block)
  records.to_a.each(&block)
end

So, this is probably what we have to do...

---

@dwkoogt In Ruby, map is automatically added for an Enumerable class which implements each, which the Records/Results class do... Example:

class MyCollection
  include Enumerable

  def each(&block)
    [1, 2, 3].each { |i| yield i }
  end
end

p MyCollection.new.map { |i| i * 100 }
# => [100, 200, 300]

[karmi]

@zoer Any new info, feedback here?

[zoer]

Yes, I think it's something that should be. I already use this workaround in my code.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.