Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validates length lets nil to be valid although nil is not allowed #7180

Closed
xHire opened this issue Jul 27, 2012 · 13 comments
Closed

validates length lets nil to be valid although nil is not allowed #7180

xHire opened this issue Jul 27, 2012 · 13 comments

Comments

@xHire
Copy link
Contributor

xHire commented Jul 27, 2012

When I setup length validation like this one:

validates :param, :length => { :minimum => 0, :allow_nil => false }

then even with nil this is valid. I created a simple demonstrative application: https://github.com/xHire/ror-zerolengthvalidation

At least in 3.0.3 this issue didn't exist, but in latest 3.1 and 3.2 it does.

@acapilleri
Copy link
Contributor

sorry, :allow_nil => false means that :mininum have to be =>1?

@xHire
Copy link
Contributor Author

xHire commented Jul 27, 2012

Well, empty string "" has zero length and it's not nil (try to call method size or length on nil ;-)). So empty string is valid here, but nil should not be. Especially when I explicitly state that nil is not allowed.

@acapilleri
Copy link
Contributor

you're right, I think that param is type_casted empty because nil.to_s == "", anyway I think that your validation isn't straightforward, you could use default => "" in your schema

acapilleri added a commit to acapilleri/rails that referenced this issue Aug 1, 2012
Given a class that include ActiveModel::Validations with a validation like the following:
validates_length_of(:title, :minimum => 0, :allow_nil => false).
This makes incosistent the validation because it not raise error when
title is *nil*.It should accepts only the empty string
With this patch title is *nil* is handled and the validation accepts only the empty string *""*
It should fix rails#7180
@josevalim
Copy link
Contributor

It is no secret that length is going to convert the value to a string if it isn't one. Writing validates_length_of(:title, :minimum => 0, :allow_nil => false) seems to be just a confusing way to write validates_presence_of(:title).

@xHire
Copy link
Contributor Author

xHire commented Aug 1, 2012

If you think that the behavior is correct, then the documentation should be appropriately updated, because it simply doesn't make any sense. nil is valid in this validation, however database doesn't save it into :null => false column (it knows that null is not the same as empty string ;-)).

What you say to be a confusing way to write something that doesn't fit to this situation is (without the redundant :allow_nil => false as it should be the default) a real use case in my application and in authlogic gem.

How would you write a validation with :presence that accepts empty string but rejects nil? There is no :allow_empty.

More than reading that my approach is bad I'd like to read why current approach is correct and why it was changed between 3.0 and 3.1? Isn't it just masking of the fact that refactoring of the validation method introduced a bug which you think is not necessary to fix so you say it's a feature?

By the way, if :minimum => 0 doesn't work (resp. it works _correctly_ only with :allow_nil => true), it's fair to at least raise an exception as acapilleri suggested. In the same way it behaves for negative numbers:

:minimum must be a nonnegative Integer (ArgumentError)

I'd like to reopen this issue because I'm confident that current state is not [correct](http://en.wikipedia.org/wiki/Correctness_(computer_science\)) and something should be done to fix this state (suggestions have already been given).

@acapilleri
Copy link
Contributor

@xHire there is a simple way to get your validation with something like that:
validates_presence_of :title , :unless => Proc.new { |a| a.title == "" }

I think it's helpful for you.

@xHire
Copy link
Contributor Author

xHire commented Aug 1, 2012

Well, maybe for me, but surely not for authlogic and others.

When someone competent solves this issue in a way that makes length validation with :minimum => 0 impossible (and answers those few questions in my previous comment), I'll create a ticket there. But currently this issue is still unresolved so I have to wait.

Simply because "hey, do it rather this way" doesn't fix broken correctness.

@acapilleri
Copy link
Contributor

I Agree, it's is inconsistent behavior and at least could be raise an exception if :minimum => 0 is not accepted.
But I don't think that rails should worry about authlogic and other but simply about his consistence

@rafaelfranca
Copy link
Member

@xHire if you want to fix why you don't give a try?

Also, I don't think that attacking the contributors will make someone fix your issue. Try to be more respectful next time.

I'm reopening but I'll not fix it now.

@rafaelfranca rafaelfranca reopened this Aug 2, 2012
@rafaelfranca
Copy link
Member

If the issue doesn't exist in 3.0.3 and exist 3.1 should be easy to see why it changed using github.

@acapilleri
Copy link
Contributor

@rafaelfranca it should be decided reopen this pull request (after some refactoring) or
handle the exception 💣 that :minimum must be greater or equal to 1

@xHire
Copy link
Contributor Author

xHire commented Aug 2, 2012

@rafaelfranca Thank you for reopening this. I didn't mean to attack anyone and I'm sorry if something seemed like that. Regarding my statement about making a feature from this bug -- that was what josevalim actually did by writing "It is no secret that length is going to convert the value to a string if it isn't one." and closing this issue without any discussion. So it seemed to me like what I wrote (I admin it could have been written in a bit different way).

I found that this commit introduces the bug: 157c37f

Right now I have a patch which handles nil correctly based on :allow_nil, :allow_blank and :maximum (:maximum should allow nil by default -- at least there is a test which explicitly requires this although it's not documented). I'll write a test suite for it and send a pull request for further discussion.

@steveklabnik
Copy link
Member

@xHire are you going to turn this into a PR anytime soon?

rafaelfranca added a commit that referenced this issue Nov 26, 2012
Length validation handles correctly nil. Fix #7180

Conflicts:
	activemodel/CHANGELOG.md
lukaszx0 pushed a commit to square/rails that referenced this issue Apr 9, 2014
When nil or empty string are not allowed, they are not valid.

Conflicts:
	activemodel/CHANGELOG.md
	activemodel/lib/active_model/validations/length.rb
	activemodel/test/cases/validations/length_validation_test.rb
lukaszx0 pushed a commit to square/rails that referenced this issue May 13, 2014
When nil or empty string are not allowed, they are not valid.

Conflicts:
	activemodel/CHANGELOG.md
	activemodel/lib/active_model/validations/length.rb
	activemodel/test/cases/validations/length_validation_test.rb

Conflicts:
	activemodel/lib/active_model/validations/length.rb
tamird pushed a commit to square/rails that referenced this issue Jun 22, 2014
When nil or empty string are not allowed, they are not valid.

Conflicts:
	activemodel/CHANGELOG.md
	activemodel/test/cases/validations/length_validation_test.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants