Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

update document should update with hash from as_indexed_json. #182

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

Closed
dwkoogt wants to merge 5 commits into elastic:master from dwkoogt:update_changed_index

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

Copy link

+1

Copy link
Contributor

karmi commented Aug 29, 2014

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

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.

Meekohi and MichaelHoste reacted with thumbs up emoji

Copy link

afn commented Dec 3, 2014

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

Copy link

afn commented Feb 24, 2015

Any update on this?

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?

Copy link

Meekohi commented Jul 17, 2017
edited
Loading

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.

MichaelHoste and xxswingxx reacted with thumbs up emoji

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?

Copy link
Contributor Author

dwkoogt commented Aug 25, 2017

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

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.

Meekohi reacted with thumbs up emoji
Copy link
Contributor Author

dwkoogt commented Aug 28, 2017
edited
Loading

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

Copy link

Meekohi commented Oct 5, 2017

@karmi any updates on what needs to happen here?

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.

bockets reacted with thumbs down emoji

@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

Reviewers

1 more reviewer

@Meekohi Meekohi Meekohi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update does not work for custom properties.

AltStyle によって変換されたページ (->オリジナル) /