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

Can Active Record stop treating blank? as nil in #9999

Closed
SamSaffron opened this issue Mar 29, 2013 · 3 comments
Closed

Can Active Record stop treating blank? as nil in #9999

SamSaffron opened this issue Mar 29, 2013 · 3 comments

Comments

@SamSaffron
Copy link
Contributor

I have been looking at some very minor perf issues that have been bugging me in Active Record.

In particular this revert by @carlosantoniodasilva :

111611b#commitcomment-2894962

At Discourse I notices blank? on string is being called a lot:

image

This is mostly originating from the reverted change in AR, once you replace apply the changes in the reverted commit the cost drops way down:

Let's pretend String#blank? was empty?
image


The revert bothers me cause its pointlessly slower and dictates a rather odd semantics:

Any unicode space is always treated as nil for boolean columns.

[2] pry(main)> u.moderator = "  \r\n\v\f\r\s\u00A0\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000"
=> "  \r\n\v\f\r                "
[3] pry(main)> u.save
   (0.4ms)  BEGIN
  User Exists (3.7ms)  SELECT 1 AS one FROM "users" WHERE ("users"."email" = 'bla@gmail.com' AND "users"."id" != 3650) LIMIT 1
   (0.7ms)  UPDATE "users" SET "moderator" = NULL, "updated_at" = '2013-03-29 20:57:34.968383' WHERE "users"."id" = 3650
   (0.6ms)  COMMIT

A single char that is not a unicode space is treated as false

 u.moderator = "a"
=> "a"
[16] pry(main)> u.save
   (0.3ms)  BEGIN
  User Exists (0.5ms)  SELECT 1 AS one FROM "users" WHERE ("users"."email" = 'bla@gmail.com' AND "users"."id" != 3650) LIMIT 1
   (0.3ms)  UPDATE "users" SET "moderator" = 'f', "updated_at" = '2013-03-29 21:02:18.428719' WHERE "users"."id" = 3650
   (0.5ms)  COMMIT
=> true

@tenderlove agrees that treating non empty strings as null is odd and a db adapter issue as opposed to something that should be handled in core.

I can go ahead and put a PR in place that reverts this, and nukes some of the odd tests for this behavior like.

7f160b0

introduced with the resolution of:

#6045


For Discourse I am working around this by implementing blank? natively, https://github.com/SamSaffron/fast_blank ... and staying consistent with .strip.length == 0 ... treating odd unicode spaces as text.

However, this odd AR behaviour kind of bothers me, and I am not sure why:

"\r\n\v\f\r\s\u00A0\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u202F\u205F\u3000" really means NULL and "\u1999" means not NULL

Can we revert connection_adapters/column.rb to do empty? checks instead, reverting all the problem tests?

@tenderlove
Copy link
Member

Can we revert connection_adapters/column.rb to do empty? checks instead, reverting all the problem tests?

👍 send a PR please.

@SamSaffron
Copy link
Contributor Author

Cool! will do, first thing next week.

carlosantoniodasilva referenced this issue Mar 30, 2013
…o_empty_on_string"

This reverts commit 9c4c05f, reversing
changes made to 4620bdc.

Reason: They're not completely interchangeable, since blank? will also
check for strings containing spaces.
SamSaffron added a commit to SamSaffron/rails that referenced this issue Apr 3, 2013
…for empty? as opposed to blank?

This is both faster and more correct, added tests to make sure this is not reverted again.
@SamSaffron
Copy link
Contributor Author

@tenderlove @carlosantoniodasilva , sent through a PR, also corrected the test suite so this is not reverted again.

Only breaking change is on booleans, time and data are functionally equivalent.

spastorino added a commit that referenced this issue Apr 3, 2013
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

3 participants