Fix changed attributes matching

[FUT]

If I define as_indexed_json function to return symbol-keyed hash then the changes will not be detected because if I changed name:

def as_indexed_json
  {name: name} 
end

as_indexed_json.keys.include? 'name' #=> false

Thus I recommend to compare string variants.

[karmi]

Hey, I think this might depend on the Ruby version? Also, I have trouble understanding the issue from your description, and there's sadly no test to help me there .)

[karmi]

(Or, instead of test, a better code example of what you're doing.)

[FUT]

Sorry for bad description :). Here is a better one I hope. First of all my searchable module definition:

module Searchable
  module User
    extend ActiveSupport::Concern

    included do
      include Elasticsearch::Model
      include Elasticsearch::Model::Callbacks

      settings index: { number_of_shards: 1 } do
        mapping do
          indexes :name, index: :not_analyzed
        end
      end

      def as_indexed_json(options={})
        { name: name }
      end
    end
  end
end

The main point here is as_indexed_json function. It has symbol keys. Then we find a user and want to update it's name.

user = User.find 1
user.update_attributes name: 'Yury'

I expect corresponding index item to be also updated but it will not because of update_document method implementation.

        # lib/elasticsearch/model/indexing.rb:336

        def update_document(options={})
          if changed_attributes = self.instance_variable_get(:@__changed_attributes)
            attributes = if respond_to?(:as_indexed_json)
              changed_attributes.select { |k,v| self.as_indexed_json.keys.include? k } #<= LOOK HERE
            else
              changed_attributes
            end

            client.update(
              { index: index_name,
                type:  document_type,
                id:    self.id,
                body:  { doc: attributes } }.merge(options)
            )
          else
            index_document(options)
          end
        end

Here are the values for LOOK HERE line:

changed_attributes    #=> {'name' => 'Yury'}
self.as_indexed_json #=> {name: 'Yury'}
self.as_indexed_json.keys #=> [:name]

changed_attributes.select { |k,v| self.as_indexed_json.keys.include? k } #=> {}

Thus we will see an update request to elasticsearch even with changed attributes:

09.06.2014 17:51: POST http://localhost:9200/users-development/user/1/_update [status:200, request:0.006s, query:n/a]
09.06.2014 17:51: > {"doc":{}}
09.06.2014 17:51: < {"_index":"users-development","_type":"user","_id":"1","_version":24}

Please let me know if you need more information. I am ashamed to say that but I failed to write test in 10 minutes... Thus detailed description is here :)

[karmi]

Right, got you. So, the problem is that when I run the example from examples/activerecord_article.rb, I got a correct behaviour:

a = Article.first
  Article Load (0.3ms)  SELECT  "articles".* FROM "articles"   ORDER BY "articles"."id" ASC LIMIT 1
=> #<Article id: 1, title: "Foo", published_at: nil, created_at: "2014-06-09 15:07:22", updated_at: "2014-06-09 15:07:22">

> a.title = "Bar"
=> "Bar"

> a.save
   (0.1ms)  begin transaction
  SQL (0.2ms)  UPDATE "articles" SET "title" = ?, "updated_at" = ? WHERE "articles"."id" = 1  [["title", "Bar"], ["updated_at", "2014-06-09 15:07:41.656140"]]
   (0.1ms)  commit transaction

2014-06-09 17:07:41 +0200: POST http://localhost:9200/articles/article/1/_update [status:200, request:0.014s, query:n/a]
2014-06-09 17:07:41 +0200: > {"doc":{"title":"Bar"}}
2014-06-09 17:07:41 +0200: < {"_index":"articles","_type":"article","_id":"1","_version":2}
=> true

I haven't got time right now to peek how changed_attributes are implemented in Rails, and whether there has been some change or not, but it would surprise me if it would fail so spectacularly as you describe.

Notice also this integration test: https://github.com/elasticsearch/elasticsearch-rails/blob/master/elasticsearch-model/test/integration/active_record_basic_test.rb#L117-L129

Can you play a bit with these examples?

Finally, I'm not opposed to being so defensive here, in this code, but I'd like to first understand what's going on, and then, of course, we need to make sure we have tests for this. And when no tests are failing, we first need to come up with a failing test....

[karmi]

@FUT Any news here?

[paulkoegel]

Just ran into the same problem. Very annoying and hard to debug.

[paulkoegel]

Btw, the Rails API demo code for changed_attributes shows strings as keys:
http://api.rubyonrails.org/classes/ActiveModel/Dirty.html#method-i-changed_attributes

[paulkoegel]

Are the specs on Travis for this PR only red b/c of a Travis issue? maybe try restarting them.

[karmi]

@paulwittmann You shouldn't use the automatic callbacks when you define your own as_indexed_json anyway. Although I understand your frustration, I don't know which "same problem" you exactly run into.

@FUT I'll try to write a test for this and verify it...

[paulkoegel]

The problem is if we have this:

class User < ActiveRecord::Base

  include Elasticsearch::Model
  include Elasticsearch::Model::Callbacks

  settings do
    mappings dynamic: 'false' do
      indexes :description, type: 'string'
    end
  end

  def as_indexed_json(options = {})
    {
      description: self.description
    }
  end

end

Then the following won't write any changes to ElasticSearch:

user.update_attributes(description: 'new description')

my current workaround:

  def as_indexed_json(options = {})
    {
      'description' => self.description
    }
  end

Why are the built-in callbacks not meant to be used when I have a custom as_indexed_json and where do the docs warn against combining the two?

[karmi]

@FUT I've updated some tests to catch the behaviour you and @paulwittmann are describing, and added the defensive to_s conversion.

@paulwittmann Your issue should be solved by the closing patch. Thanks for adding the example.

[paulkoegel]

thanks so much for the quick fix!
would still be interested if it's alright to mix include Elasticsearch::Model::Callbacks with a custom as_indexed_json (as my example showed).

[karmi]

would still be interested if it's alright to mix (...)

Should be working all right now for the case you have. But there are other problems, like eg. #166.

In general, I would recommend to take indexing into your own hands, except for trivial cases, where the automatic callbacks work. They're more a convenience, so people don't have to write silly code just to update Elasticsearch when they save a model. But in complex situations, indexing should be custom, asynchronous, etc.

[FUT]

Thank you very much for the fix guys! Sorry that I did not helped a lot..

[karmi]

@FUT On the contrary, thanks for reporting it and posting the example, unfortunately it clicked for me only after @paulwittmann added his :)