Models with Associations #10

Closed
russ opened this Issue May 3, 2011 · 8 comments

Comments

Projects
None yet
3 participants

russ commented May 3, 2011

I am trying to create an index with associations and running into errors.

For example, I have a Video model that has many categories. I want to be able to search on the video information as well as the category name.

def to_indexed_json
    { :title => title,
      :description => description,
      :categories => categories.collect { |c| c.name },
      :released_at => released_at,
    }.to_json
end

Though when the object gets reloaded I get an unknown attribute error from Rails being caused by this line in Results::Collection

object = Configuration.wrapper.new(document)

I see what the problem is, so my question is, are there plans on changing on how this is implemented in the future or am I just going about this the wrong way.

Owner

karmi commented May 4, 2011

Hi, there's no elegant solution for this problem with associations, in the current version.

The wrapper is set to the model, and obviously, the model inicialization fails with NoMethodError (or other) error when being passed the hash, which was returned from ElasticSearch.

First, I have anticipated the issue. As you can see in commit 5d89dd1, Tire was initializing the collection from the records retrieved from the database, originally (via MyModel.find [list of IDs]). This, however, had many issues. The performance was unnecesarilly dumped by roundtrips to the database. The sort order was not preserved (a huge bug). So I decided against it.

Second, the problem can be immediately solved. Let's simplify your situation (because that's really a M to N relation, with videos and categories).

The Article model has many Comment associations:

class Article < ActiveRecord::Base
  include Tire::Model::Search
  include Tire::Model::Callbacks

  index_name 'articles-with-comments'

  has_many :comments
end

The Comment is simple:

class Comment < ActiveRecord::Base
  belongs_to :article
end

Now, obviously, when you have the Article indexed as:

def to_indexed_json
  { :id      => id,
    :title   => title,
    :content => content,
    :comments => comments.map { |c| { :id => id, :author => c.author, :content => c.content }  },
  }.to_json
end

it would break on Article#comments= being given wrong type of objects on initialization.

You can temporarily, and in a quite ugly way, solve it like this:

class Article < ActiveRecord::Base
  # ...
  alias :original_comments= :comments=
  def comments=(comments)
    # Are the objects returned from ElasticSearch?
    if comments.all? { |c| c.is_a? Hash }
      comment_ids = comments.map { |c| c['id'] }
    else
      send :original_comments=, comments
    end
  end
end

So, when we are initializing the comments from ElasticSearch, set the ID (an retrieve the records from database), otherwise, perform the original method.

Of course, this could be isolated into completely different model, like this:

class SearchedArticle < Article
  index_name 'articles-with-comments'

  def comments=(comments)
    comment_ids = comments.map { |c| c['id'] }
  end
end

You'll then search via this fake model: SearchedArticle.search 'love'. Of course, this is an ugly, and temporary way of dealing with the issue. The underlying issue is, of course, blocking for advanced production usage.

When I have discussed this with another Tire users, the real, and elegant, solution seems to me like this:

  • Do not wrap the results in the real model class, but wrap them in Tire::Results::Item, as usual
  • Add a proxy object to every result, let's say object, which would point, via the ID, to the underlying record (and model instance)

In this way, we would keep the awesome performance of ElasticSearch, for most cases. Where you'd need or like to get the original model instance, you'd just write result.object.comments.first.my_complicated_method, instead of result.comments.my_complicated_method.

Thank you for the report, and I'll definitely have a look at this issue.

russ commented May 4, 2011

Cool. I'll give that a try. Thanks for the detailed response.

russ closed this May 4, 2011

karmi reopened this May 5, 2011

karmi was assigned May 5, 2011

Owner

karmi commented May 13, 2011

@russ, any luck with the proposed solution?

russ commented May 16, 2011

The solution did work. But for the time being I went back to sunspot. I will definitely give Tire another try in the future.

Owner

karmi commented May 17, 2011

Understood. Thanks for the report!

karmi closed this in 84f015d Jul 14, 2011

Owner

karmi commented Jul 14, 2011

Hi @russ, I've tried to solve this issue with ActiveRecord associations -- see the closing commit.

Could you test it against your use-case, if you still have the code handy? The only thing you need is to define Git as endpoint in the gemfile:

gem "tire", :git => "git://github.com/karmi/tire.git", :branch => "activerecord"

This looks very cool :D

Owner

karmi commented Jul 14, 2011

@aaronchi: It looks, but let's see how it works in real world :)

@tklee tklee pushed a commit to tklee/tire_shiphawk that referenced this issue Oct 26, 2014

@karmi karmi [ACTIVEMODEL] [¡BREAKING!] Wrap search results in Item class, not the…
… actual model class [Closes #11]

Due to the problem with indexing and retrieving ActiveRecord models with associations (such as Article has_many :comments),
do not wrap the results in the model class, but in the Tire::Configuration.wrapper class.

See the karmi/retire#10 issue for explanation.

This commit breaks the old behaviour and usage in Rails, since Results::Item is not (yet) ActiveModel compatible.
2295135
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment