Serialization problem in Setting model (Bug #935)

Hello Robert,

Can you provide a test case or a least an example of the failure?

Felix Schäfer wrote:

Hello Robert,

Can you provide a test case or a least an example of the failure?

Hi,

Here is example with plugin chiliproject_backlogs:
https://github.com/finnlabs/chiliproject_backlogs/issues/9

But the best example is if You will add some new gem, which has some default setting options (for example chiliproject_backlogs)
then in console just type:
Setting.plugin_backlogs
and instead of normal hash You will get string, and this is the problem because if there is no already added settings to database chiliproject use find_or_default method where is used new method from ActiveRecord which do not know that value should be serialized.

From other point I do not see any reason why in setting do not use serialize method from ActiveRecord instead of those home made methods for that. ;)

best regards

Felix Schäfer wrote:

The proposed fix won't work though as defaults are read from a yml file as well as from the DB.

Are sure about that, maybe a little example what happen in find_or_default method which is called to get setting if does not exist it get default from yml:

>> Setting.new(:name => "foo", :value => {:bar => "foo"})
=> #<Setting id: nil, name: "foo", value: "barfoo", updated_on: nil>

so value is String not hash (because of missing serialize for AR model)

After my fix (remove value and value= methods):

>> Setting.new(:name => "foo", :value => {:bar => "foo"})
=> #<Setting id: nil, name: "foo", value: {:bar=>"foo"}, updated_on: nil>

I tried to figure out why it happens like that and I found a cause.
The problem is with value=(v) method.

Simple example:

This will work:

setting = Setting.new
setting.name = "plugin_foo" 
setting.value = {:bar => 2}

but this not:

setting = Setting.new
setting.value = {:bar => 2}
setting.name = "plugin_foo" 

because to set up value we need first have name, without that we can't get @@available_settings[name], it is nil.
Because of that sometimes (depend of that what AR will first create name or value) he write string instead of hash to database because in value=(v) is:

v = v.to_yaml if v && @@available_settings[name]
write_attribute(:value, v.to_s)

so in that case v will be not converted to yaml and it will convert hash to string and store in database.

This is why remove value=(v) and value method fix the problem. I think this is the best solution to replace them by serialize from AR, it make code cleaner and do the same things.

Links:
pull request: https://github.com/chiliproject/chiliproject/pull/167

Robert Mitwicki wrote:

Felix Schäfer wrote:

The proposed fix won't work though as defaults are read from a yml file as well as from the DB.

Are sure about that, maybe a little example what happen in find_or_default method which is called to get setting if does not exist it get default from yml:

You are right, the value and value= don't read values from the settings.yml file, they do read formats from it though. Sorry about that.

Back to the problem, I don't think it was ever intended for those 2 methods to be used directly, access to the settings should all go through the Setting class with just Setting.some_setting and Setting.some_setting=, so I'm unsure about the best way to handle this.

IIRC Holger was the last one to work on the settings and had some ideas for future updates to it, so I'd like him to have a look too.

Just wanted to add my 2 Cents here.

I was running into this while working at Finnlabs, we fixed it in the finnlabs/chiliproject branches and now I'm running into it again. It seems to be related to the fact, that Hash ordering in 1.8.7 is not specified.

When initializing a new Setting instance via .new(attributes) ActiveRecord uses the defined getters and setters to set the attributes. When setting name and value in one call, it is not defined, if the name or the value is set first. If the value is set before the name, then the checks within value= if the attribute should be serialized or not, do not work.

I do not know under which circumstances the one or the other hash order occurs. All I know is, that sometimes it just goes wrong.

I will add a pull request which simply changes the way, the Setting model is initialized, but I agree with the OP, that we should eventually use ActiveRecord's built-ins.

You may find my Pull Request at https://github.com/chiliproject/chiliproject/pull/222

Gregor