Confusion about Ohm#update_attributes raising NoMehodError

[confiks]

There are now several issues (mainly #199 and #167) in which people voice their confusion about update_attributes raising NoMethodError upon accessing a model with an attribute that is present in Redis but on that model.

I imagine this is an issue that occurs frequently with new users, as they iterate the model designs while developing, and inevitably delete a model attribute at some point. At the very least this should be documented in README.md. It would also be good to raise a more specific error when an attribute found in Redis is not found in the model. Something along the lines of (method_missing is probably too naive a solution; you'll know better):

class AttributeNotFound < Error; end

def update_attributes(atts)
  atts.each do |att, val| 
    if self.respond_to?(:"#{att}=")
      send(:"#{att}=", val)
    else
      raise AttributeNotFound, "Attribute #{att} not found in model #{self.class}, while it is present in Redis."
    end
  end
end

Although you have talked at length about it in previous issues, I want to redundantly say that I think that the choice of requiring to sync all attributes present in Redis to the model layer is a wrong one. It runs contrary to common practice in other key-value store access library, it disallows other processes than Ohm adding attributes to an object, and it prevents quickly iterating a design.

That being said, apart from this issue I find Ohm to be a really nice tool to prototype a product with. For something new I really wanted to take a break from all the intricacies of relational DBs and ActiveRecord.

Might you consider adding a configuration option that modifies this behavior, with the default set to current Ohm practice? For now I've simply monkeypatched Ohm's update_attributes method:

def update_attributes(atts)
  atts.each { |att, val| send(:"#{att}=", val) if self.respond_to?(:"#{att}=") }
end

[soveran]

@confiks What do you think about letting the object load the attributes and then prevent it from saving any changes if the model and the database are out of sync?

I'm thinking about the best way to handle this issue, and I have basically two ideas:

1. Allow the user to load the attributes and print a message to stderr
2. Same as 1, but prevent the changes to be saved in order to preserve the data integrity.

The reason for preventing the changes has to do with this idea: when an error happens, there's no way for Ohm to determine if the model is authoritative or not. The developer surely knows, but for Ohm, the model could be outdated and a new definition was in charge of adding the missing fields.

I agree with you that the current behavior is probably too strict, so let's find a way to make it better. Any change will be coupled with a way to remove the extra fields from the database.

Thanks a lot for your help, I'll be waiting for your thoughts on this.

[djanowski]

I agree that current behavior is too strict, but simply ignoring unknown attributes is not useful for figuring out that you need to clean your database.

I agree that allowing to load the instance is desirable. If I deploy without running the migration first, at least my reads still work. I agree these instances should be read-only and one shouldn't be able to save them.

I don't think we should build the mechanism to remove an attribute in the instance itself (this is probably an old idea but it's mentioned in one of the linked issues). This would make the code much more complex with the only gain of some eye candy. Instead of being able to assign an old attribute to nil with the intention of deleting it on save, we could add a module with the responsibility of migrating data. Ohm::Migration.remove_attribute(:Post, :title), Ohm::Migration.remove_model(:Post) could be some examples.

This module should be part of Ohm core because it has to know exactly the key structure in order to move/delete data.

Some migrations that I had to run in the past:

• Rename attribute
• Delete attribute
• Rename model
• Delete model
• Change key structure from Ohm version X to version Y

Whenever possible, migrations should use *SCAN commands to iterate over models, keys, etc.

I'm also thinking of a simple way to let the user report progress on the migration, either with a simple puts or by connecting to something like https://github.com/djanowski/batch.

[soveran]

@djanowski I think that's the way to go.

[scalp42]

@djanowski any chance you could share examples of the different types of migration you mentioned?

I think it would be very useful if it was added as some kind of doc @soveran, what do you think ?

"To rename attribute foo to attribute bar, here is what your migration would look like" etc etc

[soveran]

@scalp42 Agreed! I'll work on it.

[scalp42]

@soveran just to clarify, I'd totally love to help but I just don't have the knowledge (or not at a point where I had to run migrations yet, trying to do everything backward-compatible as much as possible) to do so.

@djanowski list of migrations he had to run in the past sound like it's covering a lot of cases and would be awesome to see them.