soveran / ohm

Object-Hash Mapping for Redis
http://ohm.keyvalue.org
MIT License
1.4k stars 167 forks source link

Can `MissingID` be removed? #206

Closed mikeknep closed 8 years ago

mikeknep commented 8 years ago

Is there a reason calling id on a not-yet-persisted model throws an error instead of simply returning nil? I am using Ohm in a Rails app and some of Rails' form helpers expect to be able to call id on the object being created/updated, which of course it assumes is an Active Record model, which will simply return nil if it has not yet been saved to the database.

Currently I'm monkey-patching Ohm to get around this like so:

module Ohm
  class Model
    alias_method :original_id, :id
    def id
      original_i
    rescue MissingID
    end

    alias_method :original_equality, :==
    def ==(other)
      id.nil? ? false : original_equality(other)
    end
  end
end

I don't understand why id should behave differently from other attributes on an Ohm model, but perhaps I'm missing something?

soveran commented 8 years ago

The reason why raising that error is convenient is because it allows Ohm to fail as early as possible when you try to use an object that was not saved. The error can arise from comparisons (as you noticed) and from direct calls to id, but also from any call to methods that need object.key to return a proper value, as key in turn calls the id method. By raising that error we avoid internal checks for nil.

I think that patch may get you in troubles if you don't also modify the key method. Is there a way to modify the form helper you are using so that it checks object.new? instead? If that's not possible, I'm thinking an alternative could be to use a validation object instead of the real model. I use Scrivener for that purpose, maybe it works better with the form helper you are using. Another idea would be to find a form helper that's not that tightly coupled with ActiveRecord. If there isn't one, maybe we can write it.

Let me know what you find, and if you need help patching the key method let me know and I'll help you.

mikeknep commented 8 years ago

Thanks for sharing your rationale and these resources/suggestions. Scrivener looks pretty neat—I will keep it in mind as we continue to decouple this codebase from ActiveRecord. I'm pretty confident this issue of mine is limited to the form helper, to which we're unfortunately pretty tightly coupled currently, but thanks for pointing out #key—I will double check to see if I need to modify that.

soveran commented 8 years ago

Maybe a patch to key should be enough for your use case: it should raise MissingID if the new id returns nil. You may still encounter other errors that we are not considering right now, but it looks like it may work. If it doesn't, let me know and I'll help you.

soveran commented 8 years ago

Maybe this should work:

def id
  @id
end

def key
  raise MissingID if not defined?(@id)
  model.key[id]
end                                   

It tried it and it looks like it will work fine for your use case, and it won't break any other functionality. In fact, it's a change we may consider for a future release.

mikeknep commented 8 years ago

Yeah, that does look like a safer approach for the patch. Thanks!

soveran commented 8 years ago

I'm considering the change in https://github.com/soveran/ohm/commit/02239403dbcb0a53990f4e39df0fd852643b4e08, and thereby I summon @djanowski and @cyx :-)

soveran commented 8 years ago

Hey @mikeknep, just a heads up to let you know that this change made it to Ohm 3.0.0. You can read more about that release at https://github.com/soveran/ohm/releases/tag/3.0.0