Example for attr_accessible?

steve · October 16, 2007, 4:37am

Is anyone out there writing specs to check attr_accessible fields? I had
originally written my spec to check for allowing the desired fields, and
then none of the other regular db fields. Unfortunately this isn’t
satisfactory, because attr_protected could have been used instead, which
of course wouldn’t prevent mass assignment to any whatever=(val) method.

I’m thinking of maybe some internal Rails variable that holds an array
of
accessible fields maybe? Does anyone know if such a thing exists? Just
to
make it clear, I don’t want to test Rails, I want to check that the
accessible fields have been set properly. Would it be possible to mock
or
some such to check for a call to attr_accessible?

Thanks,
Steve

steve · October 16, 2007, 6:25pm

On 10/15/07, Steve [email protected] wrote:

some such to check for a call to attr_accessible?

Thanks,
Steve

rspec-users mailing list
[email protected]
http://rubyforge.org/mailman/listinfo/rspec-users

You could expect a call to attr_accessible and then load your model
file.

Or, loop through all the column names in your model, and make sure
that only the ones that are supposed to be accessible get set.

Pat

steve · October 16, 2007, 8:30pm

You could expect a call to attr_accessible and then load your model file.

Or, loop through all the column names in your model, and make sure
that only the ones that are supposed to be accessible get set.

Pat

Expecting the call is what I had originally thought of, but couldn’t
figure out how to write that test. Isn’t attr_accessible called when you
create a new instance, so there wouldn’t be time to setup an expectation
for the call?

steve · October 16, 2007, 8:44pm

On 10/16/07, Pat M. [email protected] wrote:

describe Chicken do
it “should make only :name and :age attr_accessible” do
Chicken.should_receive(:attr_accessible).with(:name, :age)
load “#{RAILS_ROOT}/app/models/chicken.rb”
end
end

I first saw this technique described by David C. at
http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html

Which I first saw described by Jay Fields at

steve · October 16, 2007, 8:41pm

On 10/16/07, Steve [email protected] wrote:

create a new instance, so there wouldn’t be time to setup an expectation
for the call?

describe Chicken do
it “should make only :name and :age attr_accessible” do
Chicken.should_receive(:attr_accessible).with(:name, :age)
load “#{RAILS_ROOT}/app/models/chicken.rb”
end
end

I first saw this technique described by David C. at
http://rubyforge.org/pipermail/rspec-users/2007-September/002965.html

Pat

steve · October 16, 2007, 8:57pm

On Tue, 16 Oct 2007 13:43:43 -0500, David C. wrote:

Which I first saw described by Jay Fields at
Jay Fields' Thoughts: Rails: Unit Testing ActiveRecord Validations

Sweet, thanks guys. That’s exactly what I was looking for.

steve · October 17, 2007, 3:43am

El 16/10/2007, a las 6:50, Steve [email protected]
escribió:

I’m thinking of maybe some internal Rails variable that holds an
array of
accessible fields maybe? Does anyone know if such a thing exists?
Just to
make it clear, I don’t want to test Rails, I want to check that the
accessible fields have been set properly. Would it be possible to
mock or
some such to check for a call to attr_accessible?

Yes, I spec this kind of thing.

describe User, ‘accessible attributes’ do
it ‘should allow mass-assignment to the login name’ do
lambda { new_user.update_attributes(:login_name =>
String.random) }.should_not raise_error
end
# many other examples here, one for each accessible attribute
that I care about
end

describe User, ‘protected attributes’ do
it ‘should deny mass-assignment to the passphrase hash’ do
lambda { new_user.update_attributes(:passphrase_hash =>
String.random) }.should raise_error
end
# many other examples here, one for each protected attribute
that I care about
end

I do it this way because:

I am most interested in the behaviour; does this let me mass-
assign to this attribute (or not)?
I don’t want my specs to depend on internal knowledge of the Rails
attribute protection/access implementation.

The style of spec shown above seems the best way to me of both
documenting the desired behaviour and confirming that it exists; I
don’t view these as tests of Rails’ internals, but as tests that I’ve
used attr_accessible and attr_protected correctly. I tried other ways
in the past, but this is the one I like best.

Another thing to note that helps the readability of the specs: the
new_user and String.random methods are provided by FixtureReplacement
(http://replacefixtures.rubyforge.org/). Note that by default
FixtureReplacement used mass-assignment under the covers, so it can’t
be used with protected attributes, but I’ve sent a patch in to the
maintainer which changes that:

Index: lib/fixture_replacement/fixture_replacement.rb

— lib/fixture_replacement/fixture_replacement.rb (revision 31)
+++ lib/fixture_replacement/fixture_replacement.rb (working copy)
@@ -70,7 +70,9 @@
hash_given = args[0] || Hash.new
merged_hash = self.send(attributes_method).merge(hash_given)
evaluated_hash = Generator.merge_unevaluated_method
(self, :create, merged_hash)

     obj = class_name.create!(evaluated_hash)

```
     obj = class_name.new
```

     evaluated_hash.each { |k, v| obj.send("#{k}=", v) }

```
     obj.save!
      obj
    end
  end
```

@@ -86,7 +88,9 @@
hash_given = args[0] || Hash.new
merged_hash = self.send(attributes_method).merge(hash_given)
evaluated_hash = Generator.merge_unevaluated_method
(self, :create, merged_hash)

```
     class_name.new(evaluated_hash)
```

```
     obj = class_name.new
```

     evaluated_hash.each { |k, v| obj.send("#{k}=", v) }

```
     obj
    end
  end
end
```

Hopefully this will be included soon.

One more thing to note: the documentation for FixtureReplacement is
pretty thin at this stage, so I threw together this cheatsheet:

http://wincent.com/knowledge-base/FixtureReplacement_cheatsheet

This is a bit easier than consulting the (excellent) screencast
provided by the author:

http://http://replacefixtures.rubyforge.org/

Cheers,
Wincent

steve · October 17, 2007, 5:59am

On 10/16/07, Steve [email protected] wrote:

I first saw this technique described by David C. at
validators, by running through all of the various checks manually.

I guess maybe it depends on if you view the testing as mostly testing a
black box, or if you assume some knowledge of the code internals? Thoughts?

Well, you’ll find differing opinions on this. Every time I’ve done
validations I’ve written a spec that actually exercises the behavior.
For example

class Person < ActiveRecord::Base
validates_presence_of :name
end

describe Person do
it “should not be valid without a name” do
p = Person.new
p.should_not be_valid
p.errors.on(:name).should == “can’t be blank”
end
end

As far as I’m concerned, that’s specifying the behavior. A person
without a name is not valid, and it results in an error message on
name. Doing it the way we’ve discussed in this thread is ugly and
stupid, imo.

However, I had never used attr_accessible up to this point, and in
this case I thought it made sense. It’s just a lot easier to expect
the call itself than to try to test every single column that’s not
accessible. You end up looping through the columns, writing helper
methods to provide default values…it all gets ugly. So, I thought
this works.

An interesting thing is that since I decided it’s okay for
attr_accessible, I’m starting to consider whether it may be okay for
other validations. afaik David C doesn’t have a firm opinion on this,
and I have to admit that mine is softening up a bit.

Personally, if I wanted to extract this pattern, I would use a custom
expectation matcher.

Person.should require_field(:name) =>
p = Person.new
p.should_not be_valid
p.errors.on(:name).should == “can’t be blank”

or to make it mirror AR a bit more
Person.should validate_presence_of(:name)

I would be happy with a nice little library of expectation matchers
that handle all the validations like that. It may seem silly since
the name is essentially the same as the method call itself, but I
prefer it because you’re using the object’s actual behavior.
Generally I want to exercise the behavior as much as possible and mock
out as little as I can. The exceptions are when I haven’t implemented
an interaction yet, so I mock it out, or when I’m dealing with code
from a lower layer in which case I keep the mocks for speed/dependency
purposes.

Pat

steve · October 17, 2007, 5:18am

On Tue, 16 Oct 2007 13:43:43 -0500, David C. wrote:

Which I first saw described by Jay Fields at
Jay Fields' Thoughts: Rails: Unit Testing ActiveRecord Validations

I’ve been thinking about this throughout the day, and is this a pattern
that should maybe be implemented in a more widespread pattern? For
example, check that the model makes the requisite calls to the various
validation functions? It would seem that unless you have some very
custom
validation methods, that you would be testing rails implementation of
its
validators, by running through all of the various checks manually.

I guess maybe it depends on if you view the testing as mostly testing a
black box, or if you assume some knowledge of the code internals?
Thoughts?

Steve

steve · October 17, 2007, 6:30am

On 10/16/07, Pat M. [email protected] wrote:

Well, you’ll find differing opinions on this.

Yes, you will. This gets very gray. Message expectations (mocks)
implicitly know about internals, but they also allow you to spec more
accurately WHERE behaviour gets implemented.

The argument in favor of loading up the file and expecting an AR
declaration (like attr_accessible) is that the behaivour is actually
implemented in AR, and it’s really not any different from a Message
Expectation except it’s happening on file load. It also means you
don’t have to hit the database, which means it runs faster.

The argument against is that the declaration is an implementation
detail and therefore brittle.

p = Person.new
p.should_not be_valid
p.errors.on(:name).should == "can't be blank"

end
end

I’ve got one project where I do this - it comes from somebody’s post
on this list (though I’m at a loss for who right now):

person.should reject(nil).for(:name).with(“can’t be blank”)
person.should reject(“”).for(:name).with(“can’t be blank”)
person.should accept(“a name”).for(:name)

An interesting thing is that since I decided it’s okay for
attr_accessible, I’m starting to consider whether it may be okay for
other validations. afaik David C doesn’t have a firm opinion on this,
and I have to admit that mine is softening up a bit.

Personally, if I wanted to extract this pattern, I would use a custom
expectation matcher.

… which would probably be implemented like this under the hood:

def matches?(target)
target.should_receive(:validates_presence_of).with(@expected)
load “#{RAILS_ROOT}/app/models/#{target.humanize}.rb”
end

No?

steve · October 17, 2007, 11:15am

On Tue, 16 Oct 2007 10:39:32 +0200, Wincent C. wrote:

in the past, but this is the one I like best.

Another thing to note that helps the readability of the specs: the
new_user and String.random methods are provided by FixtureReplacement
(http://replacefixtures.rubyforge.org/). Note that by default
FixtureReplacement used mass-assignment under the covers, so it can’t
be used with protected attributes, but I’ve sent a patch in to the
maintainer which changes that:

I’m all for avoiding rails internal behavior, but attr_accessible deals
on a share least model. If you can verify that only
attrs x, y, z are accessible, you know that any other field
that rails can come up with, or you add later is covered.

I just saw FixtureReplacement the other day, and plan on giving it a go.
Looks like it does a bunch of what I already do manually.

steve · October 17, 2007, 6:57am

On 10/16/07, David C. [email protected] wrote:

No?

No. I actually showed it in my post, though I suppose I did it in a
slightly confusing format. I would implement it like so:

def matches?(target)
instance = Target.new
instance.should_not be_valid
instance.error_messages.on(@expected).should == “can’t be blank”
end

A couple key points:

AR defines validates_presence_of, but your subclass is responsible
for doing the actual validation
This particular code is lightweight enough that it’s okay to use
the real implementation. You don’t get a big performance hit like you
do when you’re creating/updating objects in the db and their
associations
When you call MyModel.new when spec’ing business logic, it hits the
db to get the column info. You already have a coupling to the DB
simply by using AR, so I don’t think that’s a good reason to prefer
expecting the validates_* method call. If perf does become an issue,
you can stub out the columns and remove the db dependency all
together. Jay Fields mentions that in a blog, and I think he’s insane
(at least the way he showed), but it should be easy enough to preload
all the column info from schema.rb. In fact I think I heard of that
technique from someone on this list.

Pat

steve · October 18, 2007, 4:44am

El 17/10/2007, a las 7:26, “Pat M.” [email protected]
escribió:

p = Person.new
p.should_not be_valid
p.errors.on(:name).should == "can't be blank"
end
end

As far as I’m concerned, that’s specifying the behavior. A person
without a name is not valid, and it results in an error message on
name. Doing it the way we’ve discussed in this thread is ugly and
stupid, imo.

I agree that you’ll find many opinions on this. My personal take is
that you should be testing the behaviour, not testing that the
“validates…” method was called.

Why? Because while this is fine for a simple validation like
validates_presence_of, it can fall down for complex validations
with :on, :if, and other clauses. The problem is that it is fairly
easy to write a complex validation that doesn’t actually do what you
think it does. Testing that you set it up gives you very little in
that case; you should be testing that it does what you think it
does. You are not testing ActiveRecord in this case (it’s well
tested); you are testing that your use of ActiveRecord gives you the
desired behaviour. And as has already been pointed out, you can write
a custom matcher to make such oft-used specs easy to write. My own
take on this is here:

Compare this with attr_accesible. That’s much simpler; it takes a
list of symbols. It is much harder to make a mistake. In that case I
think it’s fine to mock the call and just check that your code does
the attr_accessible call. But having said that, seeing as testing the
behaviour directly is so simple (just try mass assigning and see if
it worked or not) I just test the behaviour anyway and forget about
the mock+load trick.

Cheers,
Wincent