german / redis_orm

ORM for Redis
MIT License
89 stars 16 forks source link

Unable to build ActiveRecord associations ("Couldn't find [Model name] without an ID" error) #2

Closed gleuch closed 11 years ago

gleuch commented 11 years ago

When building a has_one or belongs_to association that maps to an ActiveRecord model, a lookup is done in each of the association types for an old association that will later be cleared. However, when the lookup happens when there is no prior association, it fails. As such:

1. @comment = Comment.new(:message => 'Awesome Comment')
2. @comment.user = User.first # User is ActiveRecord
3. @comment.save

... would fail on line 2 because of the way the lookup is done for a has_one and belongs_to lookup with:

Couldn't find User without an ID

The lookup is made in lib/redis_orm/associations/belongs_to.rb, on lines 43-44,

# we need to store this to clear old association later
old_assoc = self.send(foreign_model_name)

... that calls foreign_model_name method that finds the record using the id value it gets from Redis, which returns nil if never previously associated. When nil is passed to an ActiveRecord model's find method, ActiveRecord raises an error because find_by_ids checks to see if there are 0, 1, or many ids being passed, and raising an error if the length is 0. (See: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/finder_methods.rb#L244)

When old_assoc is later checked to see if the old associated records (if !old_assoc.nil? / if old_assoc), the best fix, IMO, would be to add rescue nil when sending foreign_model_name, or adding a rescue clause in foreign_model_name, such as:

old_assoc = self.send(foreign_model_name) rescue nil

Another option is to adding some check during foreign_model_name to see if an association exist before finding the associated record, return nil before attempting to find the associated record.

Holla which method you like and I can help patch.

german commented 11 years ago

Hello @gleuch, thanks for the feedback.

Sorry but I didn't quite get it. Do you use RedisOrm along with ActiveRecord? Or you pointing out the discrepancy in APIs between them?

Also I couldn't understand why the error "Couldn't find User without an ID" would be shown after invoking User.first command? Usually it'll return just nil if there are no Users at all (I just rerun the code you provided and it works fine (if user assoc isn't necessary for Comment model)).

Patch would be ok, but providing the failing test is even better. Thanks!

gleuch commented 11 years ago

The goal was to make a has_many associated with an ActiveRecord model, which I was unable to do. After trying to debug this issue, I realized that ActiveRecord too would also need patching to get a belongs_to association from it to work with a Redis_Orm model. Defeats my purpose for mixed-model associations.

This trouble was w/ Rails 3.2.8.

The issue is not User.first. The issue is when saving a new Redis_Orm record that had an association with an ActiveRecord model, it would fail. Looking at the code, it seems that a check to find the associated record happens whether or not there is a previously associated id, in which saving a new record should not need to do this lookup. The old_assoc lookup should return nil if it is a new record or has not been assigned before, rather than blindly trying it. (The issue arises when doing a find on ActiveRecord model where the id is nil, as ActiveRecord raises error if id is nil.)

On Wed, Sep 12, 2012 at 6:13 PM, Dmitrii Samoilov notifications@github.comwrote:

Hello @gleuch https://github.com/gleuch, thanks for the feedback.

Sorry but I don't quite get it. Do you use RedisOrm along with ActiveRecord? Or pointing out the discrepancy in APIs between them?

Also I couldn't understand why the error "Couldn't find User without an ID" would be shown after invoking User.first command? Usually it'll return just nil if there are no Users at all (I just rerun the code you provided and it works fine (if user assoc isn't necessary for Comment model)).

Patch would be ok, but providing the failing test is even better. Thanks!

— Reply to this email directly or view it on GitHubhttps://github.com/german/redis_orm/issues/2#issuecomment-8512023.

german commented 11 years ago

The goal was to make a has_many associated with an ActiveRecord model, which I was unable to do.

Yes, there's no possibility to do that in current version of RedisOrm. There are certain constrains like backlinks (like you've mentioned earlier), so I don't think that's possible. However you could make your own fork and experiment with it.

Anyway why should one mix and match RedisOrm and ActiveRecord? I mean either you do the whole project on Redis or you do it on some relational database and use Redis as a storage for sessions or cached views.