Properly process booleans in MySQL dialect to fix #607#608
Properly process booleans in MySQL dialect to fix #607#608sdepold merged 5 commits intosequelize:masterfrom terraflubb:mysql_bools_save_as_tinyints
Conversation
In the MySQL dialect, values used to generate insert, bulk insert, and update queries are now checked for boolean-ness and will be turned into an int. There was already duplicated logic applied to outgoing values to check if the value was a Date or not. I factored out value processing to a single function and added a check for typeof boolean. The new function also escapes the return value since that was also being done everywhere. Fixes #607
|
Looks good, you have my vote :+1 :D |
|
Good catch, and the tests look very exhaustive. Good work! I know that @sdepold is working on refactoring the query generators right now, but I think this should still be merged in - the tests at least are crucial, since there is obviously a part that is not covered right now, |
|
Great! Thanks. Does that mean we should stick this in as a bandaid in the meantime and it will be blown away when the @sdepold refactoring drops? Or would you prefer I revert my code changes and make this a "failing-test only" PR (hopefully not to be merged until the code is fixed elsewhere 😉). |
|
coolio :) |
Properly process booleans in MySQL dialect to fix #607
|
@terraflubb I merged your stuff into a special branch features/mariadb: https://github.com/sequelize/sequelize/tree/features/mariadb Tests are pretty broken. It would be cool if you could take a closer look at it. |
|
I'd love to! I'll install MariaDB tonight and see what I can do. |
|
@janmeier Hah, I think so. I think it was also directed at @reedog117 since I don't see any of my stuff in the mariadb branch. I thought he meant that my MySQL test fudged things up over there, since MariaDB is "drop in replacement" maybe the MySQL tests were being aped. |
|
oh. got confused obsiously :D sorry |
4 participants
Fixes #607
The problem was that queries generated by with the MySQL dialect (where
Sequelize.BOOLEANis aTINYINT(1)) contained booleans in the formtrueorfalseinstead of1or0. This was causing all my queries involving adding or updating booleans to fail to the default. A whole lot of true values where I wasn't expecting them!The solution was to make sure that when values were being passed into a query, we check if they're boolean, and if we do then turn them into integers.
When I was doing that, I noticed that there was duplicated logic for processing values (where it was checked if the value was a
Date) all over the place, but not everywhere. In particular, the values used to buildWHEREclauses.So I factored all the logic for processing values (checking the type then escaping) into a function and called it from every where the logic was duplicated, and added it to the
hashToWhereConditionstoo.I included failing tests and I ran the entire test suite locally before committing.
Also to make sure I was doing it right, I ran JSHint like the README suggested. I saw a handful of minor lints I picked off. If this isn't cool, let me know and I'll revert that commit.
(I was also careful not to use any semicolons ;) )