This repository has been archived by the owner. It is now read-only.

allow removal from index on update_index callback #115

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@jarosan

jarosan commented Sep 29, 2011

I'm working on a project where we have a much larger set of data than I would like to have in the index. For this reason I am using a should_be_indexed? method and if false then submit an empty json in to_indexed_json method.

When an object gets changed and should_be_indexed? conditions change and return false then I want to remove object from the index.

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Sep 29, 2011

Owner

Hi, I feel sorry for such a fine patch. You can, in fact, achieve what you're after if you do not include Tire::Model::Callbacks and implement the indexing hooks manually, a la:

class Article

  after_save do
    self.update_index if self.state == 'published'
  end

  # ...

end

See this comment for issue karmi/retire#114.

Owner

karmi commented Sep 29, 2011

Hi, I feel sorry for such a fine patch. You can, in fact, achieve what you're after if you do not include Tire::Model::Callbacks and implement the indexing hooks manually, a la:

class Article

  after_save do
    self.update_index if self.state == 'published'
  end

  # ...

end

See this comment for issue karmi/retire#114.

karmi added a commit that referenced this pull request Sep 30, 2011

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Oct 1, 2011

Owner

Ping! Any news on this? Were you able to achieve the custom indexing with example I posted?

Owner

karmi commented Oct 1, 2011

Ping! Any news on this? Were you able to achieve the custom indexing with example I posted?

@ralph

This comment has been minimized.

Show comment
Hide comment
@ralph

ralph Oct 1, 2011

Contributor

While this is certainly possible to achieve (at least with the new version 0.3.4), I like the should_be_indexed? variant better, because you don't have to take care of the destroy/removal from the index. It's an interface everybody instantly understands, like the to_indexed_json method. Include the default modules for the default behavior, overwrite a method if you want custom behavior, simple as that.

But anyway, thanks for making this possible with version 0.3.4.

Contributor

ralph commented Oct 1, 2011

While this is certainly possible to achieve (at least with the new version 0.3.4), I like the should_be_indexed? variant better, because you don't have to take care of the destroy/removal from the index. It's an interface everybody instantly understands, like the to_indexed_json method. Include the default modules for the default behavior, overwrite a method if you want custom behavior, simple as that.

But anyway, thanks for making this possible with version 0.3.4.

@leehambley

This comment has been minimized.

Show comment
Hide comment
@leehambley

leehambley Oct 1, 2011

Contributor

I agree with @ralph's position here, but only in the face of Tire being effectively a "Rails" interface to full text search. In Rails apps, as with most "web applications" I think it's quite normal that there's a bunch of things which shouldn't be searchable... and it's natural to expect something like should_be_indexed? – I expected at least that it would exist. (but we haven't had the need for me to explore that more thoroughly yet)

Contributor

leehambley commented Oct 1, 2011

I agree with @ralph's position here, but only in the face of Tire being effectively a "Rails" interface to full text search. In Rails apps, as with most "web applications" I think it's quite normal that there's a bunch of things which shouldn't be searchable... and it's natural to expect something like should_be_indexed? – I expected at least that it would exist. (but we haven't had the need for me to explore that more thoroughly yet)

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Oct 1, 2011

Owner

@ralph, @leehambley: I understand your point. I don't think adding a top level thing like should_be_indexed? is particularly useful to general audience, though.

First, you have to properly document and explain, how that works, and you'll still be fighting edge-cases. It's a slippery slope of massive "optional" behaviours written for singular cases. We should be more concerned about adding timeout and search_type, folks! :)

Second, and more importantly, Tire is just Ruby -- why not use it? I mean, Tire::Model::Search::InstanceMethods::Searchupdate_index is just couple of lines, doing nothing particularly advanced. If one has an edge case, where the indexing logic is somehow more complicated, she should definitely write her own indexing hooks. Possibly doing stuff in the background, via queue or messaging infrastructure, etc.

All that said, I'll happily pull that into karmi/tire-contrib!

@jarosan: Any luck using what I suggested?

Owner

karmi commented Oct 1, 2011

@ralph, @leehambley: I understand your point. I don't think adding a top level thing like should_be_indexed? is particularly useful to general audience, though.

First, you have to properly document and explain, how that works, and you'll still be fighting edge-cases. It's a slippery slope of massive "optional" behaviours written for singular cases. We should be more concerned about adding timeout and search_type, folks! :)

Second, and more importantly, Tire is just Ruby -- why not use it? I mean, Tire::Model::Search::InstanceMethods::Searchupdate_index is just couple of lines, doing nothing particularly advanced. If one has an edge case, where the indexing logic is somehow more complicated, she should definitely write her own indexing hooks. Possibly doing stuff in the background, via queue or messaging infrastructure, etc.

All that said, I'll happily pull that into karmi/tire-contrib!

@jarosan: Any luck using what I suggested?

@jarosan

This comment has been minimized.

Show comment
Hide comment
@jarosan

jarosan Oct 1, 2011

@karmi: The reason I wanted this to be done with should_be_indexed? method is because i wanted to keep the existing destroy/save callbacks without worrying about them.

It is possible to achieve that with this:

def remove_from_index
Place.tire.index.remove self if self.published_at.blank?
end

but this still felt like a hack to get it working.

I think the reason my suggestion makes less sense to you because I didn't include the logic of indexing/not indexing the record in to_indexed_json method. @karmi, what do you think. Is it possible that you change your mind, if i submit a patch, where this logic (should_be_indexed?) would also affect whether an object gets indexed or not in the first place. That way this would be a general and recommended way to decide what objects get indexed and what not.

The syntax for doing this could be different. Either a should_be_indexed? method. Or maybe use named_scope-like syntax to pass a proc exclude_from_index lambda { |r| r.published_at.blank? }

Either way I really think that this is something Tire would benefit from :)

jarosan commented Oct 1, 2011

@karmi: The reason I wanted this to be done with should_be_indexed? method is because i wanted to keep the existing destroy/save callbacks without worrying about them.

It is possible to achieve that with this:

def remove_from_index
Place.tire.index.remove self if self.published_at.blank?
end

but this still felt like a hack to get it working.

I think the reason my suggestion makes less sense to you because I didn't include the logic of indexing/not indexing the record in to_indexed_json method. @karmi, what do you think. Is it possible that you change your mind, if i submit a patch, where this logic (should_be_indexed?) would also affect whether an object gets indexed or not in the first place. That way this would be a general and recommended way to decide what objects get indexed and what not.

The syntax for doing this could be different. Either a should_be_indexed? method. Or maybe use named_scope-like syntax to pass a proc exclude_from_index lambda { |r| r.published_at.blank? }

Either way I really think that this is something Tire would benefit from :)

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Oct 2, 2011

Owner

Hi @jarosan,

I am still not convinced conditional indexing like this is something which belongs to core Tire. I still think you'd be better off if you took care of the indexing yourself, in this case. I imagine the tests, the code readability, everything would benefit from that.

That said, why not extract that, and put it into tire-contrib? Then the only thing you have to do is:

gem 'tire-contrib'
require 'tire/conditional_indexing'

I will happily pull that into karmi/tire-contrib.

Owner

karmi commented Oct 2, 2011

Hi @jarosan,

I am still not convinced conditional indexing like this is something which belongs to core Tire. I still think you'd be better off if you took care of the indexing yourself, in this case. I imagine the tests, the code readability, everything would benefit from that.

That said, why not extract that, and put it into tire-contrib? Then the only thing you have to do is:

gem 'tire-contrib'
require 'tire/conditional_indexing'

I will happily pull that into karmi/tire-contrib.

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Oct 4, 2011

Owner

@jarosan: Ping -- need any help with porting it over to tire-contrib?

Owner

karmi commented Oct 4, 2011

@jarosan: Ping -- need any help with porting it over to tire-contrib?

@jarosan

This comment has been minimized.

Show comment
Hide comment
@jarosan

jarosan Oct 4, 2011

@karmi: no worries. wouldn't want to overcomplicate my code with using tire-contrib. will use the before_update_elasticsearch_index hook to get it working.

jarosan commented Oct 4, 2011

@karmi: no worries. wouldn't want to overcomplicate my code with using tire-contrib. will use the before_update_elasticsearch_index hook to get it working.

@jarosan jarosan closed this Oct 4, 2011

@mahemoff

This comment has been minimized.

Show comment
Hide comment
@mahemoff

mahemoff Jan 22, 2013

For anyone searching, I explained how to support private resources with manual callbacks. http://softwareas.com/private-resources-with-elasticsearch-and-tire

For anyone searching, I explained how to support private resources with manual callbacks. http://softwareas.com/private-resources-with-elasticsearch-and-tire

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Jan 22, 2013

Owner

Thanks for the article, Michael, will have a look.

Owner

karmi commented Jan 22, 2013

Thanks for the article, Michael, will have a look.

@brupm

This comment has been minimized.

Show comment
Hide comment
@brupm

brupm Jan 23, 2013

Contributor

@karmi and @mahemoff

Really confused about this:

# This is copied straight from the source, which references half-baked implentations.
# Don't know in what situations it would be necessary, but including it anyway
before_destroy { @destroyed = true }
def destroyed?; !!@destroyed; end 
Contributor

brupm commented Jan 23, 2013

@karmi and @mahemoff

Really confused about this:

# This is copied straight from the source, which references half-baked implentations.
# Don't know in what situations it would be necessary, but including it anyway
before_destroy { @destroyed = true }
def destroyed?; !!@destroyed; end 
@mahemoff

This comment has been minimized.

Show comment
Hide comment
@mahemoff

mahemoff Jan 23, 2013

@brupm it's code in the original callback [1] that implements the bult-in destroyed? method, but only @karmi can explain in what circumstances that method mightn't work.

  1. https://github.com/karmi/tire/blob/master/lib/tire/model/callbacks.rb#L28

@brupm it's code in the original callback [1] that implements the bult-in destroyed? method, but only @karmi can explain in what circumstances that method mightn't work.

  1. https://github.com/karmi/tire/blob/master/lib/tire/model/callbacks.rb#L28
@mahemoff

This comment has been minimized.

Show comment
Hide comment
@mahemoff

mahemoff Aug 6, 2013

@karmi I've just been looking at this code and it seems the before_destroy (or destroyed?) override is preventing destruction for some reason. Do you know if it's still necessary in Rails 3.2+?

(I wasn't much deleting this record type before, so it may have always been an issue.)

mahemoff commented Aug 6, 2013

@karmi I've just been looking at this code and it seems the before_destroy (or destroyed?) override is preventing destruction for some reason. Do you know if it's still necessary in Rails 3.2+?

(I wasn't much deleting this record type before, so it may have always been an issue.)

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Aug 6, 2013

Owner

@mahemoff As far as I remember, "some half-baked ActiveModel implementations" were not providing destroyed?, which would break the "update or delete" semantics in update_index. Is the issue you're experiencing on ActiveRecord, some ActiveModel, or Tire::Persistence?

Owner

karmi commented Aug 6, 2013

@mahemoff As far as I remember, "some half-baked ActiveModel implementations" were not providing destroyed?, which would break the "update or delete" semantics in update_index. Is the issue you're experiencing on ActiveRecord, some ActiveModel, or Tire::Persistence?

@mahemoff

This comment has been minimized.

Show comment
Hide comment
@mahemoff

mahemoff Aug 6, 2013

@karmi This is with an ActiveRecord where I'm manually implementing Tire::Model::Callbacks because some records are private. I noticed the records weren't being deleted until I removed those lines (Rails 3.2.13).

Based on your explanation, it's probably okay for me to remove those lines as I'm handling the callbacks manually anyway.

mahemoff commented Aug 6, 2013

@karmi This is with an ActiveRecord where I'm manually implementing Tire::Model::Callbacks because some records are private. I noticed the records weren't being deleted until I removed those lines (Rails 3.2.13).

Based on your explanation, it's probably okay for me to remove those lines as I'm handling the callbacks manually anyway.

@karmi

This comment has been minimized.

Show comment
Hide comment
@karmi

karmi Aug 6, 2013

Owner

@mahemoff This sounds like a bug -- would it be feasible to create an isolated example or even an integration test? No worries if not.

Owner

karmi commented Aug 6, 2013

@mahemoff This sounds like a bug -- would it be feasible to create an isolated example or even an integration test? No worries if not.

@mahemoff

This comment has been minimized.

Show comment
Hide comment
@mahemoff

mahemoff Aug 6, 2013

I'll try to find time in the next few days.

I don't really think it's a Tire bug though.

mahemoff commented Aug 6, 2013

I'll try to find time in the next few days.

I don't really think it's a Tire bug though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.