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 document should update with hash from as_indexed_json. #182

Closed
wants to merge 5 commits into from

Conversation

dwkoogt
Copy link
Contributor

@dwkoogt dwkoogt commented Jul 28, 2014

update document should update with hash from as_indexed_json which could also contain derived attributes.
resolves #178
this also should resolve issues #88 #166 and #195

@ksaveras
Copy link

+1

@karmi
Copy link
Contributor

karmi commented Aug 29, 2014

@dwkoogt @xawiers Please see my comments in #178 -- I think we can close this issue.

@dwkoogt
Copy link
Contributor Author

dwkoogt commented Aug 29, 2014

Well, #166 solves the problem of not using as_indexed_json to update the index. However, it is still limited to the attribute keys returned by changed_attriubtes which only watches changes in field attributes. If a custom key value (some derived value) is added within as_indexed_json, that key value change is not being watched. Hence if there's a change in that value, the change is not reflected in the index. Therefore my solution deletes the unchanged key-values instead of selecting the changed key-values. This will insure that any custom key value pair will be included in the update. This was my issue in #178.

@afn
Copy link

afn commented Dec 3, 2014

Can this be merged? The build failures appear to be spurious.

@afn
Copy link

afn commented Feb 24, 2015

Any update on this?

@Meekohi
Copy link

Meekohi commented Jul 17, 2017

Hate to just +1 an issue, but this is the same problem I'm having as well and a little lost. Are there any work-arounds in the meantime for "forcing" a derived field to get sent along with ES updates?

@Meekohi
Copy link

Meekohi commented Jul 17, 2017

Okay dug around in this more today and personally I like the @dwkoogt solution.

  1. It is exactly the same if you don't use as_indexed_json
  2. It is exactly the same if you use as_indexed_json but only with real attributes (i.e. that can be tracked in changed_attributes)
  3. If you use computed attributes or methods in as_indexed_json (i.e. anything that can not be tracked in changed_attributes) it conservatively sends those along during updates.

In my opinion this is a pretty significant and sneaky bug that should be fixed.

@karmi "It certainly cannot be merged as is... We can absolutely have talk about the problem there."

Could you let us know what needs to be improved here to have it merged? I'd be willing to spend some time on this.

@karmi
Copy link
Contributor

karmi commented Aug 25, 2017

Hi @dwkoogt, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@dwkoogt
Copy link
Contributor Author

dwkoogt commented Aug 25, 2017

Hi @karmi, just updated my profile with the email address. thx.

@@ -393,7 +393,9 @@ def delete_document(options={})
def update_document(options={})
if changed_attributes = self.instance_variable_get(:@__changed_attributes)
attributes = if respond_to?(:as_indexed_json)
self.as_indexed_json.select { |k,v| changed_attributes.keys.map(&:to_s).include? k.to_s }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change for performance? Original version seemed more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original change included with_indifferent_access method which is a rails extension which I removed here.

@dwkoogt
Copy link
Contributor Author

dwkoogt commented Aug 28, 2017

Btw, I rebased on master and Travis is failing on this:
$ curl -sS https://snapshots.elastic.co/downloads/elasticsearch/elasticsearch-6.0.0-alpha1-SNAPSHOT.tar.gz | tar xz -C /tmp

@Meekohi
Copy link

Meekohi commented Oct 5, 2017

@karmi any updates on what needs to happen here?

@stale
Copy link

stale bot commented Aug 31, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 31, 2020
@stale stale bot closed this Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update does not work for custom properties.
5 participants