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

Unify boolean quoting for Mysql- and Mysql2Adapter. #12425

Merged
merged 2 commits into from
Nov 11, 2013

Conversation

senny
Copy link
Member

@senny senny commented Oct 2, 2013

This issue was raised with #11119. Fixed with #11120 and later reverted to prevent possible backward incompatible changes.

Since then I've written a bunch of test-cases to illustrate the behavior of Mysql2Adapters quoting and to reason about consequences of the patch.

@senny
Copy link
Member Author

senny commented Oct 2, 2013

/cc @awilliams @pixeltrix @jeremy

@iwiznia
Copy link
Contributor

iwiznia commented Oct 23, 2013

Why is this not merged already??? Please, merge it so it can be added to v4.0.1... it's a broken functionality!

@senny
Copy link
Member Author

senny commented Oct 23, 2013

Even if we merge this one right now, it won't be part of 4.0.1. As this changes the functionality we probably won't even backport it to 4-0-stable and we'll have to wait for 4.1 until this gets released.

@iwiznia
Copy link
Contributor

iwiznia commented Oct 23, 2013

Bummer!
This bug (among other things) causes that if you define a uniqueness validator on a boolean column, it fails...

@senny
Copy link
Member Author

senny commented Oct 23, 2013

@iwiznia we are aware of that, that's where the issue originated from. (If you want to read back on the discussion, see the issues I linked at the top).

@pixeltrix
Copy link
Contributor

@iwiznia I've never understood why people are putting validates_uniqueness_of on boolean columns - what's your use case?

@iwiznia
Copy link
Contributor

iwiznia commented Oct 23, 2013

Well it's only useful when using it with the scope option too.
A use case might look like: You have a user and a user has many payment methdos, but only one payment method can be active at any given time, so the payment method has an 'active' field which is boolean, and you want that value to be unique when it's set to true.

@pixeltrix
Copy link
Contributor

You have a user and a user has many payment methdos, but only one payment method can be active at any given time, so the payment method has an 'active' field which is boolean, and you want that value to be unique when it's set to true.

Surely the better way to model this would be a active_payment_method_id column on the user model? Either way I'd see getting a validation error saying that I already had an active payment method as an annoyance - why not just change the active one to the one I'd just entered or selected if that's what I've indicated?

@iwiznia
Copy link
Contributor

iwiznia commented Oct 23, 2013

Well, it depends on the use case, the case I described is a small example...
Besides, why would you add a new column to the user model if you can get the information anyway without it? I mean, you can have an association on the user that retrieves the active payment method without adding that column. Like so:

 has_one :active_payment_method, -> { where(active: true) }, class_name: 'PaymentMethod'

@pixeltrix
Copy link
Contributor

The reason to use an attribute on the user model is one of atomicity - with the validation on the active attribute you have to first set the currently active payment method to false before you can set a new one to true. Also in the strictest send validates_uniqueness_of would only allow two payment methods - one with active: true and one with active: false, whereas what you really want is to ensure that only one payment method is active at any time.

@iwiznia
Copy link
Contributor

iwiznia commented Oct 24, 2013

Yes, you need to update the previous active to false first, but that's no problem... In the other solution, you also have 2 make 2 operations, first create the record, then associate it in the parent model, so it's basically the same.
And in my case I have an 'if' option to only run the validation when it's active.

@nanaya
Copy link
Contributor

nanaya commented Oct 29, 2013

@pixeltrix: by setting active: nil in a callback, it's possible to have one active: true and many active: nil. It is also a valid db unique index in some databases.

I'm also hitting this problem and I can't add another column because in my case, active (in my case, main) is scoped to another columns:

Tag:
- id

TagName:
- id
- name
- locale
- main
- tag_id

a tag can have many names, with each locale only have one main TagName.

validates :main, uniqueness: { scope: [:locale, :tag_id],
                               allow_nil: true }

senny added a commit that referenced this pull request Nov 11, 2013
Unify boolean quoting for Mysql- and Mysql2Adapter.
@senny senny merged commit 0bb6b43 into rails:master Nov 11, 2013
@senny senny deleted the mysql_booleans branch November 11, 2013 15:23
@arthurnn
Copy link
Member

arthurnn commented Mar 3, 2014

Can we backport this to 4-0-stable? @carlosantoniodasilva @senny

@senny
Copy link
Member Author

senny commented Mar 3, 2014

@arthurnn better not. This actually changes behavior. I hope no-one is actively relying on the old one but better save than sorry.

@akaspick
Copy link
Contributor

I'm finally getting around to upgrading a rails 3.2 app to rails 4 and use the uniqueness validation on a boolean field with a scope and this issue breaks it as already mentioned. What I don't see is a suggested fix for this regression. What are people currently doing to get this working again with rails 4?

@byroot
Copy link
Member

byroot commented Mar 19, 2014

@akaspick we use this monkey patch: https://gist.github.com/byroot/9645116

@akaspick
Copy link
Contributor

@byroot Thanks. Works perfectly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants