From fbcec890a7fa4e88aa24aa5815477d2c89f94b46 Mon Sep 17 00:00:00 2001
From: Kevin Miller <kevin.miller@flexport.com>
Date: Thu, 25 Apr 2019 19:30:57 -0700
Subject: [PATCH] Do not fetch attribute values for excluded models

Upon upgrading from 1.20 => 1.22 we noticed errors due to changes around
handling excluded records. This PR implements a fix by only fetching
attribute names when determining if an index operation is necessary and
adds a test to ensure that our use case will not break. Example model

```rb
class Example < ActiveRecord::Base
  include AlgoliaSearch

  algoliasearch :unless => :nil_name do
    attribute :name_length do
      # will crash if name is nil
      name.length
    end
  end

  def nil_name
    name.nil?
  end
end
```
---
 lib/algoliasearch-rails.rb | 28 +++++++++++++++++-----------
 spec/integration_spec.rb   | 25 ++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/lib/algoliasearch-rails.rb b/lib/algoliasearch-rails.rb
index 818b48cf..4a182311 100644
--- a/lib/algoliasearch-rails.rb
+++ b/lib/algoliasearch-rails.rb
@@ -146,18 +146,18 @@ def get_default_attributes(object)
     end
 
     def get_attribute_names(object)
-      get_attributes(object).keys
+      get_unresolved_attributes(object).keys
     end
 
     def attributes_to_hash(attributes, object)
       if attributes
-        Hash[attributes.map { |name, value| [name.to_s, value.call(object) ] }]
+        Hash[attributes.map { |name, value| [name.to_s, value.is_a?(Proc) ? value.call(object) : value] }]
       else
         {}
       end
     end
 
-    def get_attributes(object)
+    def get_unresolved_attributes(object)
       # If a serializer is set, we ignore attributes
       # everything should be done via the serializer
       if not @serializer.nil?
@@ -168,17 +168,23 @@ def get_attributes(object)
           attributes = get_default_attributes(object)
         else
           # at least 1 `attribute ...` has been configured, therefore use ONLY the one configured
-          if is_active_record?(object)
-            object.class.unscoped do
-              attributes = attributes_to_hash(@attributes, object)
-            end
-          else
-            attributes = attributes_to_hash(@attributes, object)
-          end
+          attributes = @attributes
         end
       end
 
-      attributes.merge!(attributes_to_hash(@additional_attributes, object)) if @additional_attributes
+      attributes.merge!(@additional_attributes) if @additional_attributes
+      attributes
+    end
+
+    def get_attributes(object)
+      attributes = get_unresolved_attributes(object)
+      if is_active_record?(object)
+        object.class.unscoped do
+          attributes = attributes_to_hash(attributes, object)
+        end
+      else
+        attributes = attributes_to_hash(attributes, object)
+      end
 
       if @options[:sanitize]
         sanitizer = begin
diff --git a/spec/integration_spec.rb b/spec/integration_spec.rb
index 915d6232..b737561e 100644
--- a/spec/integration_spec.rb
+++ b/spec/integration_spec.rb
@@ -98,6 +98,9 @@
   create_table :disabled_symbols do |t|
     t.string :name
   end
+  create_table :disabled_conditionals do |t|
+    t.string :name
+  end
   create_table :encoded_strings do |t|
   end
   create_table :forward_to_replicas do |t|
@@ -208,6 +211,21 @@ def self.truth
   end
 end
 
+class DisabledConditional < ActiveRecord::Base
+  include AlgoliaSearch
+
+  algoliasearch :synchronous => true, :index_name => safe_index_name("DisabledConditional"), :unless => :nil_name do
+    attribute :name_length do
+      # will crash if name is nil
+      name.length
+    end
+  end
+
+  def nil_name
+    name.nil?
+  end
+end
+
 module Namespaced
   def self.table_name_prefix
     'namespaced_'
@@ -506,7 +524,7 @@ class SerializedObject < ActiveRecord::Base
     it "should push the name but not the other attribute" do
       o = SerializedObject.new :name => 'test', :skip => 'skip me'
       attributes = SerializedObject.algoliasearch_settings.get_attributes(o)
-      expect(attributes).to eq({:name => 'test', "_tags" => ['tag1', 'tag2']})
+      expect(attributes).to eq({"name" => 'test', "_tags" => ['tag1', 'tag2']})
     end
   end
 end
@@ -1342,6 +1360,11 @@ class ReplicaThenSlave
     DisabledSymbol.create :name => 'foo'
     expect(DisabledSymbol.search('').size).to eq(0)
   end
+
+  it "should disable the indexing using a conditional and not read attributes" do
+    DisabledConditional.create :name => nil
+    expect(DisabledConditional.search('').size).to eq(0)
+  end
 end
 
 describe 'NullableId' do