Compatibility for rails 3.2 #116
Conversation
Rails to serialize model names to symbols instead of strings. Fixed in more recent versions of Rails.
| @@ -336,7 +336,7 @@ def delete_document(options={}) | |||
| 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 } | |||
| changed_attributes.select { |k,v| self.as_indexed_json.keys.map{|k| k.to_s}.include? k } | |||
There was a problem hiding this comment.
I wonder if this could be improved a little. The original code called #as_indexed_json for each changed attribute in the model. If there were 10 changed attributes, there would be 10 calls to serialize the model. The code should probably only call #as_indexed_json once in order to get the list of keys.
The new code adds a map operation that converts all keys to strings, so if there were 10 keys to convert, and 10 attribute changes there would be 100 #to_s calls.
Symbol.#to_s creates a new String instance, while String.to_sym will always return the same symbol given the same String.
How about something like this?
indexed_symbols = self.as_indexed_json.keys.map(&:to_sym)
changed_attributes.select { |k,v| indexed_symbols.include? k.to_sym }There was a problem hiding this comment.
Your absolut right. I improved the code with commit b8aedf9
|
Yeah, thanks @AaronRustad, the main issue with repeatedly calling @beatjoerg Thanks for the patch!, can you please try to figure out how you can add unit tests for your change? I wonder, by the way, what is going on in here, because the gem is tested against ActiveRecord 3.2, see the gemfile. I'd like to also ask you to consult the commit log, to see how the commits are worded and written -- I can always retouch them before merging, but it's also always nice when I have less to do :) |
…because as_json returns symbols instead of strings in rails 3.2 See Rails 3.2 (missing .to_s) https://github.com/rails/rails/blob/3-2-stable/activemodel/lib/active_model/serialization.rb#L85 vs Rails 4.1 https://github.com/rails/rails/blob/4-1-stable/activemodel/lib/active_model/serialization.rb#L110 Related: elastic#116
|
I have added a unit test as requested. I hope commit message confirms the rules. returns in rails 3.2: |
|
@beatjoerg Great, thanks, I'll look at it next week.
That's troubling, can you maybe open a new ticket with any errors etc. That should be all very easy -- the tests assume a testing cluster/node is running on |
…e miss because as_json returns symbols instead of strings in rails 3.2 Related: elastic#116
|
Just found my mistake and commited the integration test |
|
@beatjoerg I don't think this particular issue warrants a separate, special integration test. Thanks for the patch!, I'll try processing it soon. |
|
@beatjoerg I'm terribly sorry, I can see from git blame that I've already added the |
Rails to serialize model names to symbols instead of strings. Fixed in more recent versions of Rails.