[MODEL] Fixed a problem where Hashie::Mash#min and #max returned unexpected values

While people can define names for aggregations such as `min` and `max`, these
methods are defined in `Enumerable#min` and `Enumerable#max`, and return
an unexpected value:

    result.aggregations
    # => {"price"=>{"doc_count"=>9428, "min"=>{"value"=>1.0}, "max"=>{"value"=>260000.0}}}
    result.aggregations.price
    # => {"doc_count"=>9428, "min"=>{"value"=>1.0}, "max"=>{"value"=>260000.0}}
    result.aggregations.price.min
    # => ["doc_count", 9428]

Therefore, the whole Hash with aggregations is being traversed, in a wrapper class,
and a `min` and `max` method is defined for Hashie::Mash instances,
because these names are too common.

The wrapper class can have its uses further down the line, so a dedicated
unit test file has been added as well.

Closes elastic#568

Related: elastic/elasticsearch-ruby#306

Author: karmi

elasticsearch-model/lib/elasticsearch/model.rb

@@ -31,6 +31,7 @@
 require 'elasticsearch/model/response/results'
 require 'elasticsearch/model/response/records'
 require 'elasticsearch/model/response/pagination'
+require 'elasticsearch/model/response/aggregations'
 require 'elasticsearch/model/response/suggestions'
 
 require 'elasticsearch/model/ext/active_record'

elasticsearch-model/lib/elasticsearch/model/response.rb

@@ -69,7 +69,7 @@ def shards
         # Returns a Hashie::Mash of the aggregations
         #
         def aggregations
-          response['aggregations'] ? Hashie::Mash.new(response['aggregations']) : nil
+          Aggregations.new(response['aggregations'])
         end
 
         # Returns a Hashie::Mash of the suggestions

elasticsearch-model/lib/elasticsearch/model/response/aggregations.rb

@@ -0,0 +1,36 @@
+module Elasticsearch
+  module Model
+    module Response
+
+      class Aggregations < Hashie::Mash
+        def initialize(attributes={})
+          __redefine_enumerable_methods super(attributes)
+        end
+
+        # Fix the problem of Hashie::Mash returning unexpected values for `min` and `max` methods
+        #
+        # People can define names for aggregations such as `min` and `max`, but these
+        # methods are defined in `Enumerable#min` and `Enumerable#max`
+        #
+        #     { foo: 'bar' }.min
+        #     # => [:foo, "bar"]
+        #
+        # Therefore, any Hashie::Mash instance value has the `min` and `max`
+        # methods redefined to return the real value
+        #
+        def __redefine_enumerable_methods(h)
+          if h.respond_to?(:each_pair)
+            h.each_pair { |k, v| v = __redefine_enumerable_methods(v) }
+          end
+          if h.is_a?(Hashie::Mash)
+            class << h
+              define_method(:min) { self[:min] }
+              define_method(:max) { self[:max] }
+            end
+          end
+        end
+      end
+
+    end
+  end
+end

elasticsearch-model/test/integration/active_record_basic_test.rb

@@ -12,6 +12,7 @@ class ActiveRecordBasicIntegrationTest < Elasticsearch::Test::IntegrationTestCas
             create_table :articles do |t|
               t.string   :title
               t.string   :body
+              t.integer  :clicks, :default => 0
               t.datetime :created_at, :default => 'NOW()'
             end
           end
@@ -24,24 +25,25 @@ class ::Article < ActiveRecord::Base
               mapping do
                 indexes :title,         type: 'string', analyzer: 'snowball'
                 indexes :body,          type: 'string'
+                indexes :clicks,        type: 'integer'
                 indexes :created_at,    type: 'date'
               end
             end
 
             def as_indexed_json(options = {})
               attributes
                 .symbolize_keys
-                .slice(:title, :body, :created_at)
+                .slice(:title, :body, :clicks, :created_at)
                 .merge(suggest_title: title)
             end
           end
 
           Article.delete_all
           Article.__elasticsearch__.create_index! force: true
 
-          ::Article.create! title: 'Test',           body: ''
-          ::Article.create! title: 'Testing Coding', body: ''
-          ::Article.create! title: 'Coding',         body: ''
+          ::Article.create! title: 'Test',           body: '', clicks: 1
+          ::Article.create! title: 'Testing Coding', body: '', clicks: 2
+          ::Article.create! title: 'Coding',         body: '', clicks: 3
 
           Article.__elasticsearch__.refresh_index!
         end
@@ -217,11 +219,17 @@ def as_indexed_json(options = {})
 
         should "allow dot access to response" do
           response = Article.search query: { match: { title: { query: 'test' } } },
-                                    aggregations: { dates: { date_histogram: { field: 'created_at', interval: 'hour' } } },
+                                    aggregations: {
+                                      dates: { date_histogram: { field: 'created_at', interval: 'hour' } },
+                                      clicks: { global: {}, aggregations: { min: { min: { field: 'clicks' } } } }
+                                    },
                                     suggest: { text: 'tezt', title: { term: { field: 'title', suggest_mode: 'always' } } }
 
           response.response.respond_to?(:aggregations)
-          assert_equal 2, response.aggregations.dates.buckets.first.doc_count
+          assert_equal 2,   response.aggregations.dates.buckets.first.doc_count
+          assert_equal 3,   response.aggregations.clicks.doc_count
+          assert_equal 1.0, response.aggregations.clicks.min.value
+          assert_nil        response.aggregations.clicks.max
 
           response.response.respond_to?(:suggest)
           assert_equal 1, response.suggestions.title.first.options.size

elasticsearch-model/test/unit/response_aggregations_test.rb

@@ -0,0 +1,46 @@
+require 'test_helper'
+
+class Elasticsearch::Model::ResponseAggregationsTest < Test::Unit::TestCase
+  context "Response aggregations" do
+    class OriginClass
+      def self.index_name;    'foo'; end
+      def self.document_type; 'bar'; end
+    end
+
+    RESPONSE = {
+      'aggregations' => {
+        'foo' => {'bar' => 10 },
+        'price' => { 'doc_count' => 123,
+                     'min' => { 'value' => 1.0},
+                     'max' => { 'value' => 99 }
+                   }
+      }
+    }
+
+    setup do
+      @search  = Elasticsearch::Model::Searching::SearchRequest.new OriginClass, '*'
+      @search.stubs(:execute!).returns(RESPONSE)
+    end
+
+    should "access the aggregations" do
+      @search.expects(:execute!).returns(RESPONSE)
+
+      response = Elasticsearch::Model::Response::Response.new OriginClass, @search
+      assert_respond_to response, :aggregations
+      assert_kind_of Elasticsearch::Model::Response::Aggregations, response.aggregations
+      assert_kind_of Hashie::Mash, response.aggregations.foo
+      assert_equal 10, response.aggregations.foo.bar
+    end
+
+    should "properly return min and max values" do
+      @search.expects(:execute!).returns(RESPONSE)
+
+      response = Elasticsearch::Model::Response::Response.new OriginClass, @search
+
+      assert_equal 123, response.aggregations.price.doc_count
+      assert_equal 1,   response.aggregations.price.min.value
+      assert_equal 99,  response.aggregations.price.max.value
+    end
+
+  end
+end