Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

when some field is defined as not null on the database, the unique and format matchers raise Errors #194

Closed
shotty01 opened this Issue · 14 comments

8 participants

@shotty01

Rails 3.1
Ruby 1.9.2
Shoulda-matchers 1.4.1

I have a model Daemon with the following validations:

class Daemon < ActiveRecord::Base
validates :name, :presence => true, :format => /\A[\w -.]+\z/
validates :binary, :presence => true
etc.

In the migration, it is defined as:

class CreateDaemons < ActiveRecord::Migration
def change
create_table :daemons do |t|
t.string :name, :null => false
t.string :binary, :null => false
t.string :version, :null => false, :default => 'unknown'
etc.

when i construct the test:
describe Daemon do
it { should validate_presence_of(:name) }
it { should validate_uniqueness_of(:name) }
it { should validate_format_of(:name).with('socr-ates on Test DMZ Munich') }
it { should validate_format_of(:name).not_with('socr#ates') }

i get the following errors:

1) Daemon
Failure/Error: it { should validate_uniqueness_of(:name) }
ActiveRecord::StatementInvalid:
Mysql::Error: Column 'binary' cannot be null: INSERT INTO daemons (binary, created_at, installation_id, log_file, name, opt_file, port, report_file, updated_at, version) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
# ./spec/models/daemon_spec.rb:7:in `block (2 levels) in '

2) Daemon
Failure/Error: it { should validate_format_of(:name).not_with('socr#ates') }
Expected errors to include "is invalid" when name is set to "socr#ates", got errors: ["binary can't be blank (nil)"]
# ./spec/models/daemon_spec.rb:9:in `block (2 levels) in '

Now, either the shoulda-matchers only work if i don't have any not null constraints in the database (then you should add that in the README and in the documentation) or I'm doing something wrong - am I missing something? I also tried to add a
before(:each) { FactoryGirl.create(:daemon) }
in my spec file, but this also does get me the binary can't be blank(nil) error in both cases.
Thanx in advance for any help/comments

@shotty01

thanks to the changed documentation and some trial and error, got it to work!

before(:each) was necessary and you have to pay attention if your model is dependent on another model.
for me, i had to create the 'parent' object along with the daemon

@shotty01 shotty01 closed this
@gabebw
Owner

Glad you got it working!

@bdwain

None of the other validations require that and it goes against common expectations about how the method works. Wouldn't it be better to make the method work as expected rather than have everyone just use a workaround?

@mcmire
Owner

@bdwain All validation matchers expect the record you are testing to be valid (except for the column that you're checking), as #valid? will be called on the record in question. Here, in the case of validates_uniqueness_of, we try to create a new Daemon by saying Daemon.new; ... some stuff ...; daemon.save, and that fails because the binary column wasn't filled in. In the case of validates_format_of, it's a similar error except this time it's the validates_presence_of that's failing.

The correct solution here is to 1) use a factory in conjunction with validation matchers instead of just Class.new and 2) make sure that required columns are filled in inside of your factory.

@bdwain

I've never needed to create an object before the example when checking other things like validates_presence_of or validates_numericality_of. Uniqueness is the only one that had the problem for me (I haven't used format though).

My point was just that it was not obvious to me as a user of shoulda-matchers that I would need to create an instance of the object before testing it because I didn't need to for any other validation that I did, and the documentation didn't display that very prominently so it was easy to miss.

I don't think many people using the library would expect to need an object to be created in the database before calling should_validate_foo_of, and since most validations don't, these 2 exceptions should be noted much more prominently in the docs or they should not be exceptions.

@hanchennz

Thanks for the help guys. Here's the updated documentation for future reference!

http://rubydoc.info/github/thoughtbot/shoulda-matchers/master/frames

Found it: Ensures that the model is invalid if the given attribute is not unique. It uses the first existing record or creates a new one if no record exists in the database. It simply uses :validate => false to get around validations, so it will probably fail if there are NOT NULL constraints. In that case, you must create a record before calling validate_uniqueness_of.

@hanchennz

oops, thanks~!

@mcmire
Owner

Yeah, as you found, it is in the docs. However, if you ever get an error from validates_uniqueness_of it should be more friendly and should teach you what to do to resolve the error. We have a plan for this that we'll implement in the future.

@jmuheim

This issue is really a bit of a pain, needed some time to find this page and to create a workaround. I've done it like this:

describe Member do
  it { should validate_presence_of(:username).with_message "can't be blank" }
  it { create(:member); should validate_uniqueness_of(:username).case_insensitive }
end

This should be a better way to solve this. A config option to let Shoulda know what factory to use (for example) would be highly appreciated.

@vikhyat vikhyat referenced this issue in discourse/discourse
Merged

Initial badge system implementation #2115

@tyre

:+1:

This violates the principle of least astonishment, making assumptions about the other column.

In many cases, the uniqueness is scoped to a foreign key. Whenever possible, it is best practice to have not null constraints on foreign keys (or any column that is expected to never be null.)

I think the default should be changed to put a value in the field and have an option, or chained method, to allow_nil. Similar to e349381

@mcmire
Owner

Sorry but we don't ever plan on automatically filling in attributes. We tried to do this a while back and found that we couldn't do this 100% of the time. Either way a "surprising" exception may get raised, and you have to handle it somehow.

I think something that checks the record in question to ensure that columns marked as NOT NULL are filled in and raising a specific error message would probably be the best solution here. I have a note to add this at some point though I'd gladly take a pull request.

@schbetsy schbetsy referenced this issue from a commit in bendyworks/payroll
@schbetsy schbetsy Ensure non-null values in SQL
- Requires a workaround in the uniqueness test, as in
thoughtbot/shoulda-matchers#194
e92b83a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.