retire
retire copied to clipboard
karmi
Result Items pretend to be another class
This is one of the most frustrating things in any library. If you want it to pretend to be something else then use SimpleDelegator. This caused a lot of pain until realizing that I was not really the class that the object claimed to be.
Hi, thanks for the feedback. Now:
a) What problem exactly do you experience by Tire::Results::Item masquerading as a different class, within Rails. It does so for a purpose, and it is clearly laid out in the Readme.
b) Could you be more specific as to how would SimpleDelegator (or any other delegation) solve the problem that Tire::Results::Item instance must generate proper dom_id strings or work in article_path(@article) helpers?
@marioaquino and @coffeencoke: No point in +1-ing anything here, really. Do you have any concrete suggestions/ideas or a concrete problematic behaviour to demonstrate?
Karmi and I spent some time talking on IRC. I just want to get some of that information propagated back to the ticket.
I wasn't upset with the code. Tire works great. I love TIRE!
Overriding class is a usability issue for many developers. When debugging they interrogate the class and based on the information they get back there are some assumptions made about the capabilities of the object. Those assumptions here are really wrong.
On further inspection of the object respond_to? returns false for most methods because it overrides method_missing to give those capabilities. This maybe another ticket, but it would be great if respond_to? would be overridden in the same way. Although with the current implementation the object really responds to any and all method names you can throw at it.
In this case I have an assumption that my object is my model. So I try the following:
result.my_name #=> 'amos' result.my_name = 'Amos' #=> No error result.my_name #=> 'amos'
That is a surprising result.
Hi, thanks for the update, Adam.
The thing is, this whole story with overriding class is much more complicated then everybody thinks — or I am indefinitely stupid :)
First, I don't know how delegation as a concept would solve this issue. We really don't want to delegate to anything, because we don't have anything. We don't have the “real” model loaded. We have only some JSON returned from elasticsearch.
(Notice how we use delegation in Tire to expose the methods such as mapping, settings, index, search either directly in the class or in the tire instance and class proxy methods: https://github.com/karmi/tire/blob/master/lib/tire/model/search.rb#L225-241.)
Please correct me if you feel I'm wrong on this.
Second, the whole purpose of this is to make Tire::Results::Item instances work within Rails, without loading the “real” models from the database. As you may be aware, that means eg. dom_id helpers, and, prominently URL helpers.
You may try it out very simply: just comment out the def class definition, and you'll see the errors.
You'll see how Rails' https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/routing/polymorphic_routes.rb fails to construct the correct route, and when you do the digging, you'll discover how https://github.com/rails/rails/blob/master/activemodel/lib/active_model/naming.rb#L162-164 calls the class method on the passed model (if it's not a class).
Now, here's your turn, @adkron, @marioaquino, @coffeencoke. How should we handle this? Is there a way to fake it for ActiveModel/ActionPack without over-loading class? I'd be very glad if there was.
@adkron, @marioaquino, @coffeencoke, would love to hear your thoughts on the last comment... Closing the issue for now.
I have to think back to what the issue was in the first place to come up with some suggestions. Providing an object that acted and appeared like a specific model, but wasn't the model, caused quite a lot of issues and pain when we were using Tire.
If I have a moment I'll try to check it out. I do not think the current implementation is the only implementation, and I think that's why this issue was created in the first place. It was created to notify the makers of tire that the design for this particular class caused a lot of pain for us.
If your decision is to keep the current implementation, I think that's absolutely no problem.
Providing an object that acted and appeared like a specific model, but wasn't the model, caused quite a lot of issues and pain when we were using Tire.
Interesting -- would love to hear them, because we didn't hit any issues with that. Once you understand that the result object just looks like your model instance (and it does not hide it, see eg. https://github.com/karmi/tire/blob/master/lib/tire/results/item.rb#L68), you're OK, I'd say.
I understood the issue as being submitted by @adkron as an academic concern. Moreover, delegation really can't solve anything here (unless I'm seriously mistaken), which is why a bit of sarcasm may have creeped into my answers.
Until ActiveModel works differently in Naming.model_name_from_record_or_class, I don't think we have much choice, if we do want to use the Item instances in Rails helpers, views, etc. And I, for one, seriously do want and need to do that :)
Well, what caused me some extra time today was exactly this. I have an existing (read-only) Elasticsearch store with a couple of indices that map to plain Ruby objects that implement the ActiveModel API to support routing, naming etc. in Rails. In these Ruby objects we define certain methods to do stuff with the data from Elasticsearch, think of things like combining names or formatting stuff. What isn't very nice about how Tire::Result::Item is currently implemented is that it only delegates #class to the object. If you've defined extra methods in your classes they can't be accessed from the Item. So if I have a class definition like this (where first_name and last_name come from ES):
class Author
include MyActiveModelImplementation
include Tire::Model::Search
def name
[first_name, last_name].join(' ')
end
end
If you search ES for authors you will get Items that pretend to be Authors, but aren't. So you can't use any custom defined methods. Best example of how Tire not fully implements the required methods to be a "pretend" class, try this from a console with a item from a result:
# item.class
=> Author
# item.is_a?(Author)
=> false
# item.name
=> nil
As you can see it claims to be an Author, but when you query that using is_a? you get false. You also get nil for any undefined attributes.
We circumvent this by creating a proxy object that inherits from SimpleDelegator and pass that as the wrapper object to Tire and have it decide at runtime what it should map to and how it should delegate. The only thing Tire currently does is delegate #class to super and return nil for every unknown attribute. The proxy object simply assigns attributes like Tire does, but also acts as a proxy for the "real" object so you can have all your class/instance methods and variables at your disposal.
The ActiveModel bit is actually extra here, the same issue would still arise if you just use plain Ruby objects. You lose the ability to use any extra stuff you have defined on your classes. I would say that if it is going to pretend to be an object it should mimick the behaviour of said object by delegating any missing attributes/methods (and potentially even allow for overriding them).
A very simple proxy that would instantiate an object based on _type would be something like:
class ProxyObject < SimpleDelegator
delegate :class, :is_a?, :to_hash, :to => :_proxied_object
def initialize(attrs={})
klass = attrs['_type'].camelize.classify.constantize
@_proxied_object = klass.new(attrs)
super(_proxied_object)
define_methods attrs # this call creates all relevant accessors for the attrs and methods (method_missing)
end
private
def _proxied_object
@_proxied_object
end
end
It's a bit of a tricky thing to explain, but like I said, if it's going to delegate #class I think it should play along nicely and be a proper delegator/proxy and I think that's the point the original poster tried to convey.
+1 to what @tieleman proposes. I'm not entirely sure what the architecture of such a solution should look like, but being able to somehow access instance variables from tire Item's would be a very good feature.
@tieleman Thanks for the suggestion.
Nevertheless, your implementation only adds support for is_a? -- is my understanding correct here?
Do notice that you cannot delegate in the proper sense here, because you simply don't have the object to delegate to.
My implementation “masquerades” as the real object (by over-riding class, making it possible to use Item in Rails view helpers), your implementation “delegates” to a bogus instance of the model, created with MyModel.new(attributes). (For a fun piece of archeology, the behaviour has changes couple of times here, to accommodate for most cases, see eg. https://github.com/karmi/tire/commit/2295135.)
Now, we can argue what's a better/cleaner way, but I would generally stay out of calling new on ActiveRecord classes as far as I can. Second, the default Item is by no means the only available wrapper for results -- you are free and encouraged to create your own. In your case, you probably could make your Article class a nice wrapper.
Third, I agree that Item#is_a? should behave properly -- that is, in my opinion, orthogonal to all this “delegation” debate?
Yes, seems fair enough. Note that in my examples I wasn't actually using ActiveRecord models, but plain Ruby objects that implement ActiveModel. But, your point still stands: we are creating objects (using new) that aren't really there, but that's what ES is all about, you are retrieving the data from somewhere else, so some magic is bound to occur.
Another suggestion (which I haven't worked out) would be to have the resulting Item inherit from the "real" class, so in the example above Item would inherit from Author. That would carry over certain instance methods etc. for use in the subclass. But that would just create more headaches down the road I think (protected and private methods come to mind).
Your suggestion about creating your own wrappers is actually precisely what we're doing at the moment in our setup. We retrieve the data from ES, then construct our faux models ourselves using a different wrapper than Item. We have no underlying SQL database. That's why we're not using ActiveRecord, but merely are implementing ActiveModel to get all the fancy routing/templating stuff from Rails.
I was just contributing to the discussion here (clarifying if you will), because I do think that it's something a lot of people might be interested in. Ofcourse they are free to implement their own wrappers based on what I've suggested in my earlier comment. Might be nice to have a Wiki page about this subject? I'd be willing to write a how-to on how to (har har) do this and provide sample code etc.
And agreed about the Item#is_a?, that would make it more complete.
@tieleman Thanks for your thoughts!
Note that in my examples I wasn't actually using ActiveRecord models, but plain Ruby objects that implement ActiveModel.
Yeah, I noticed. But the problem is that many people work with “underlying” models provided by some crazy ORM/ODM with unknown semantics...
But, your point still stands: we are creating objects (using new) that aren't really there (...)
Yes!, and that's why all discussion about “using SimpleDelegator” is more or less armchair warfare. Of course, we can debate the merits of different implementations of the behaviour.
We retrieve the data from ES, then construct our faux models ourselves using a different wrapper than Item.
That's very cool to hear -- I always envisioned the wrapper feature of Tire as a nice hook point. Also, do notice the Tire::Model::Persistence layer, which is what I have been using for these purposes.
(...) have the resulting Item inherit from the "real" class (...)
I would be even more worried about inheritance then delegating to bogus models :)
Might be nice to have a Wiki page about this subject?
Absolutely -- create one, we can add it here, tweet about it, etc. But after some pressing stuff is done, Tire obviously needs more structured documentation.
I've started work on the Wiki page, it is not complete yet by any means, but hopefully it will serve as a summary and a bit of general information about wrapping results.
https://github.com/karmi/tire/wiki/Data-storage-in-Tire-and-wrapping-the-results
I've finished the write-up for now. I'd appreciate it if anyone would be able to give it a read and let me know if it's understandable. I'll add it to the Wiki homepage.
@tieleman I think you're making it a tad bit complicated in the "Using a custom wrapper" part -- normally folks using ActiveRecord/Mongoid/etc can just use :load and be done with it.
Custom wrapper is really useful if you want to somehow wrap, decorate, etc the JSON results from ES -- as was your original situation with models having MyActiveModelImplementation...
Fine, it was a bit hard to distill from the above discussion what use cases people were trying to achieve. I'll strip the AR bit and just make it plain objects.
Yes, please mention the :load option and that you can use a custom proxy object to wrap it like you did...
Am I correct in assuming that if you use the :load option to retrieve objects from your ActiveRecord/Mongoid store you will no longer have access to the ES meta-information (including _score and stuff like highlighting)?
Update -- seems like the unfortunate call to class is still there in Rails 4 (https://github.com/rails/rails/blob/master/actionpack/lib/action_controller/model_naming.rb#L9).
Thinking about how to solve this more elegantly in the new Elasticsearch gem, but I'm worried that we need to "masquerade" if we want to support the out-of-the box compatibility of results with Rails' helpers.
I'll definitely make it opt-in, at least.