Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update callbacks do not use as_indexed_json #37

Closed
fcheung opened this issue Mar 11, 2014 · 7 comments
Closed

Update callbacks do not use as_indexed_json #37

fcheung opened this issue Mar 11, 2014 · 7 comments

Comments

@fcheung
Copy link

fcheung commented Mar 11, 2014

The update callbacks mine changed_attributes directly

def update_document(options={})
   if changed_attributes = self.instance_variable_get(:@__changed_attributes)
     client.update(
        { index: index_name,
          type:  document_type,
          id:    self.id,
          body:  { doc: changed_attributes } }.merge(options)
     )
  else
    index_document(options)
  end
end

This has 2 consequences for people with a custom as_indexed_json

  • any indexed attributes that are not active model attributes are not indexed
  • if as_indexed_json omits some attributes then they will be included when a record is updated in this way

Perhaps the documentation should highlight this?

@palanglung
Copy link

+1
the changed_attributes should filtered as like as_indexed_json cause maybe not all fields are public and used for indexing.

@fcheung
Copy link
Author

fcheung commented Mar 11, 2014

If we did want to make this work beyond trivial cases perhaps ES::Model should be asking the instance for updates rather than taking them out of changed attributes. There could always be a default implementation that just returned the changed attributes.

On the otherhand perhaps the pragmatic thing is to just write your own update_document if you have that need.

@samqiu
Copy link

samqiu commented Mar 11, 2014

Please see #5 .

@fcheung
Copy link
Author

fcheung commented Mar 11, 2014

D'oh - I'm a dummy. Given that the conclusion has been reached that the library shouldn't be trying to do anything smart in this case, perhaps the documentation should reflect this?

@spodlecki
Copy link

+1 to document it's behavior in README. I had custom callbacks, and it would not update the serialized json field. The solution was to just use es.index_document on the models that have serialized fields versus update_document. Its a bit of a performance killer most likely, but it works and is only updated on a cron job every 20 mins.

@karmi
Copy link
Contributor

karmi commented Mar 13, 2014

@fcheung I'm still thinking whether we can somehow intercept the changes better, but I don't think so. It would be nice to check whether the model itself (and not the proxy) implements the as_indexed_method...

karmi added a commit that referenced this issue Apr 15, 2014
…serialized during update

Previously, the `update_document` method simply intercepted the changes to the model, via the
`@__changed_attributes` variable, and used these directly.

This caused models with a custom serialization method to be incorrectly serialized, namely
unwanted attributes were added.

This patch looks for `as_indexed_json` defined on the model, and when it finds it,
filters the changed attributes through the keys.

Closes #75

Related:

* #59
* #57
* #52
* #40
* #37
* #5
@karmi
Copy link
Contributor

karmi commented Apr 15, 2014

@fcheung Actually, the simple case was quite simple to solve, unless I'm missing something very obvious -- please check the linked commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants