Add support for MySQL #47
Conversation
ricardogama
force-pushed
the
feature/mysql-support
branch
2 times, most recently
from
1b04b92
to
b7a29d7
Compare
src/index.js
Outdated
| @@ -49,19 +49,22 @@ function parse(model, response, options = {}) { | |||
| export default Bookshelf => { | |||
| const Model = Bookshelf.Model.prototype; | |||
| const client = Bookshelf.knex.client.config.client; | |||
| const parseOnFetch = client === 'sqlite' || client === 'sqlite3'; | |||
| const handleOnSaving = client !== 'mysql'; | |||
There was a problem hiding this comment.
Disabling serialization on save for mysql breaks in my project, so I'm inclined to think that you should not do this. It seems like it is necessary to allow the plugin to do its thing both on save and fetch for mysql. Disabling on save when trying to save a model with an object in place of the expected JSON string results in the following error: Error: ER_WRONG_VALUE_COUNT_ON_ROW: Column count doesn't match value count at row 1. So the json column / object still needs to be serialized/stringified when mysql is the client.
There was a problem hiding this comment.
That's weird, if you see the test suite I added for MySQL, all tests passed on saving without registering a JSON column, which does not happen for other clients, and that's why we didn't tested against MySQL until now.
Can you provide a snippet with real data showing the plugin breaking? Turning on debug on the knexfile would help.
There was a problem hiding this comment.
Sure thing. Disabling the stringify on save results in the following:
{ method: 'insert',
options: {},
timeout: false,
cancelOnTimeout: false,
bindings:
[ { street: '1234 Road Ave',
city: 'Town',
zip: 32309,
someOtherField: 'data' },
Thu Jan 19 2017 09:39:17 GMT-0500 (EST),
'test@example.com',
'72261b8c-752f-4196-96b7-6b115d921f21',
'Test User',
'test-realm',
Thu Jan 19 2017 09:39:17 GMT-0500 (EST) ],
__knexQueryUid: '9ed6cc46-ec48-465a-9df2-0d7b1299df9c',
sql: 'insert into `users` (`address`, `created_at`, `email`, `id`, `name`, `phoneNumber`, `realm`, `updated_at`) values (?, ?, ?, ?, ?, DEFAULT, ?, ?)' }
Error: ER_WRONG_VALUE_COUNT_ON_ROW: Column count doesn't match value count at row 1
address is the field that should be stringified.
Enabling stringify on save works great:
{ method: 'insert',
options: {},
timeout: false,
cancelOnTimeout: false,
bindings:
[ '{"street":"1234 Road Ave","city":"Town","zip":32309,"someOtherField":"data"}',
Thu Jan 19 2017 09:45:29 GMT-0500 (EST),
'test@example.com',
'dc608afb-3731-4d31-9a07-73cb97cee673',
'Test User',
'test-realm',
Thu Jan 19 2017 09:45:29 GMT-0500 (EST) ],
__knexQueryUid: '4b5508eb-c3a0-4755-b8f0-6b053d98ec56',
sql: 'insert into `users` (`address`, `created_at`, `email`, `id`, `name`, `phoneNumber`, `realm`, `updated_at`) values (?, ?, ?, ?, ?, DEFAULT, ?, ?)' }
The address gets stringified and appropriately inserted into the TEXT field in the mysql db.
There was a problem hiding this comment.
I'm struggling to reproduce your query, what's the default value for phoneNumber? Can you provide the full table schema? Also, if you set a phoneNumber on the model do you get the same error?
There was a problem hiding this comment.
Please also provide your knex and MySQL versions as well.
There was a problem hiding this comment.
Sure here's the whole schema:
knex.schema.createTableIfNotExists('users', (table) => {
table.uuid('id').primary().notNullable()
table.timestamps(false, true)
table.string('name').notNullable()
table.string('realm').notNullable()
table.string('email').notNullable()
table.string('phoneNumber')
table.json('address')
table.unique(['realm', 'email'])
})Using knex ^0.12.6 and mysql ^2.12.0
There was a problem hiding this comment.
For what it's worth, my suggestion would be to simply remove handleOnSaving and the associated logic for it. In my experiences so far, that gets everything working perfectly.
There was a problem hiding this comment.
The thing is that I would like to reproduce the error, adding a test where a query fails without using the plugin and another fixing it with the plugin as it's our convention.
Anyway if I can't do that I'll remove that constraint and turn on the parsing on saving across all clients as you suggested, although tests on MySQL will get weird.
I currently don't have time to investigate further, can you please use the hotfix/mysql-support branch in the meantime?
There was a problem hiding this comment.
Yeah absolutely. Thanks for setting up the hotfix branch. :)
.travis.yml
Outdated
| - cp test/postgres/knexfile.js.dist test/postgres/knexfile.js | ||
| - cp test/sqlite/knexfile.js.dist test/sqlite/knexfile.js | ||
| - psql -U postgres -c 'create database "bookshelf-json-columns";' | ||
| - mysql -e "create database \`bookshelf-json-columns\`;" -uroot |
README.md
Outdated
| @@ -75,12 +75,12 @@ bookshelf.Model.extend({ | |||
|
|
|||
| Contributions are welcome and greatly appreciated, so feel free to fork this repository and submit pull requests. | |||
|
|
|||
| **bookshelf-json-columns** supports PostgreSQL and SQLite3. You can find test suites for each of these database engines in the *test/postgres* and *test/sqlite* folders. | |||
| **bookshelf-json-columns** supports PostgreSQL, SQLite3 and MySQL. You can find test suites for each of these database engines in the *test/postgres*, *test/sqlite* and *test/mysql* folders. | |||
|
|
|||
There was a problem hiding this comment.
What about ... You can find test suites for each of these database engines in the *test* folder.
I think it's enough.
README.md
Outdated
|
|
||
| ### Setting up | ||
|
|
||
| - Fork and clone the **bookshelf-json-columns** repository. | ||
| - Duplicate *test/postgres/knexfile.js.dist* and *test/sqlite/knexfile.js.dist* files and update them to your needs. | ||
| - Duplicate *test/postgres/knexfile.js.dist*, *test/sqlite/knexfile.js.dist* and *test/mysql/knexfile.js.dist* files and update them to your needs. |
There was a problem hiding this comment.
Maybe, in relation to the comment above: Duplicate each *.dist* database engine test file and update them to your needs.
There was a problem hiding this comment.
Done, please review.
ricardogama
force-pushed
the
feature/mysql-support
branch
from
4e2dba3
to
95777de
Compare
This PR adds support for MySQL which does not parse JSON columns on fetch.
Closes #46