Confusing API for updating attributes

[brandonweiss]

I wrote some code to update a Virtus object, something like:

@form.attributes.merge(user_params)

I was surprised when my tests failed. The attributes hash was being updated correctly but the reader methods for the attributes weren't returning the right data. I checked the docs and googled a bit to try and figure out how to mass-assign attributes, and found #86. Apparently I'm supposed to do this:

@form.attributes = user_params

Now my tests pass, which is great. But I can already tell that this is going to be confusing for future-me or other people. You would never know that it's only updating attributes present in the hash from looking at that if you didn't already know that Virtus specifically designed the attributes setter to behave like that. That's very confusing. Even worse, the API that someone would logically expect to work, doesn't.

[elskwid]

@brandonweiss - I find attributes= to be a confusing interface as well. I wouldn't want to use .merge since that seems tied to the internal implementation. What about something like .merge_attributes which means what it says but we don't have to care about what's going on in there.

[brandonweiss]

Yeah, to support .merge you'd have to move the concept of attributes to an object that behaved like enumerable. Which might not be bad. But creating a method would be far easier. .merge_attributes makes sense. Or even .update_attributes.

[solnic]

I'm not sure if the current API is confusing, it's a typical mass-assignment API that other libs have. Frankly I've never seen any complaints about it until now. Virtus stores values in ivars and #attributes returns hash with attribute values so I'm not sure why anybody would expect attributes.merge to work.

We can add #update - no need for the _attributes suffix. This would make virtus objects quack more like a hash.

[solnic]

btw in ROM we rely on #update when you want to use immutable objects in session so adding it to virtus would be nice.

[solnic]

btw 2 - I hate mass-assignment. it's been extracted into a separate module in virtus 1.0 and I'm considering turning it off by default in 2.0.

[mbj]

btw 2 - I hate mass-assignment. it's been extracted into a separate module in virtus 1.0 and I'm considering turning it off by default in 2.0.

+1

[elskwid]

In the interest of full disclosure, I hadn't tried to use the .attributes= or looked at it closely until this issue was filed. When I see that method and it's name I expect it to replace all the attributes - as if I am setting the attributes to the values being passed in. Looking at it now it seems silly that I thought this method would reset all the attributes and populate the object with new values.

I use the mass assignment at instantiation all the time with Virtus but I never use it afterwards. This is especially true when using Virtus in Rails projects. There's such a huge proliferation of hash-like interfaces in Rails. Controller params, attributes off of AR models, etc. Virtus shines in these places because it gives structure where it is missing. (Esp in controllers!). @solnic, do you have an example of another library that uses a the .attributes= for what amounts to a partial update of an object's attributes? (I'm curious about it.)

[mbj]

I aggree #attributes = is a SRP and DHMB (Dont hurt my brain) violation. Lets nuke it.

[solnic]

@elskwid not sure if you're trolling me right now or what :smile:

Here are two libs that have #attributes= which works like in virtus:

• ActiveRecord
• DataMapper

Maybe you've heard about those ;)

I started Virtus by doing copy'n'paste from dm-core so some things we no longer like are still here.

[elskwid]

@solnic - Not trolling at all. I have never used that method in either library. WHAT A NEWB!

[dkubb]

I think we're all agreed that #attributes= is a crappy interface. The only reason it exists in virtus is to provide some compatibility to the AR and DM interfaces. I agree with @solnic that it should be removed for virtus 2.0. In the meantime, for virtus 1.0, I think we're better off sticking with the current semantics.