- Notifications
You must be signed in to change notification settings - Fork12
Fix warning when using mysql or sqlite with knex >= 0.16.4#78
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Knex 0.16.4 began warning when the returning attribute is supplied fordialects that do not support it. These dialects currently include MySQLand SQLite3. Supplying this attribute is a no-op for those dialects, soexcluding them is safe and eliminates the warning.More information:knex/knex#3039
Also make test/knexfile.js match mocha.start.js in terms of defaults forenvironment variables.
@eldridge is this PR complete and are tests working as expected with the changes? |
eldridge commentedJan 18, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@crobinson42 yes, the tests passed for me as is on SQLite and MySQL. I also successfully ran a modified version of the test suite against PostgreSQL. The existing test suite doesn't pass with the pg driver due to column syntax exceptions and differences in identifier quoting, but those are preexisting issues. A diff illustrating the test suite modifications necessary to get the test suite to pass against both master as well as this branch using the pg driver follows: diff --git a/mocha.start.js b/mocha.start.jsindex d41cb84..3c17eb6 100644--- a/mocha.start.js+++ b/mocha.start.js@@ -51,7 +51,22 @@ JSDataAdapterTests.init({ xmethods: [ // The adapter extends aren't flexible enough yet, the SQL adapter has // required parameters, which aren't passed in the extend tests.- 'extend'+ 'afterCreate',+ 'afterUpdate',+ 'beforeCreate',+ 'beforeUpdate',+ 'count',+ 'createMany',+ 'create',+ 'destroyAll',+ 'destroy',+ 'extend',+ 'findAll',+ 'find',+ 'sum',+ 'updateAll',+ 'updateMany',+ 'update' ], xfeatures: [ 'findHasManyLocalKeys',diff --git a/test/filterQuery.spec.js b/test/filterQuery.spec.jsindex 979843c..6ccd628 100644--- a/test/filterQuery.spec.js+++ b/test/filterQuery.spec.js@@ -79,27 +79,27 @@ describe('DSSqlAdapter#filterQuery', function () { it('should apply query from array', function () { var query = {} var sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user`')+ assert.deepEqual(sql, 'select * from "user"') query = { age: 30 } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where `age` = 30')+ assert.deepEqual(sql, 'select * from "user" where "age" = 30') query = { age: 30, role: 'admin' } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where `age` = 30 and `role` = \'admin\'')+ assert.deepEqual(sql, 'select * from "user" where "age" = 30 and "role" = \'admin\'') query = { role: 'admin', age: 30 } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where `role` = \'admin\' and `age` = 30')+ assert.deepEqual(sql, 'select * from "user" where "role" = \'admin\' and "age" = 30') query = { role: 'admin',@@ -112,7 +112,7 @@ describe('DSSqlAdapter#filterQuery', function () { ] } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where `role` = \'admin\' and `age` = 30 order by `role` desc, `age` asc limit 5 offset 10')+ assert.deepEqual(sql, 'select * from "user" where "role" = \'admin\' and "age" = 30 order by "role" desc, "age" asc limit 5 offset 10') query = { where: {@@ -125,7 +125,7 @@ describe('DSSqlAdapter#filterQuery', function () { } } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where `role` = \'admin\' and `age` = 30')+ assert.deepEqual(sql, 'select * from "user" where "role" = \'admin\' and "age" = 30') query = { where: [@@ -142,7 +142,7 @@ describe('DSSqlAdapter#filterQuery', function () { ] } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where (`role` = \'admin\') and (`age` = 30)')+ assert.deepEqual(sql, 'select * from "user" where ("role" = \'admin\') and ("age" = 30)') query = { where: [@@ -174,7 +174,7 @@ describe('DSSqlAdapter#filterQuery', function () { ] } sql = adapter.filterQuery(adapter.knex('user'), query).toString()- assert.deepEqual(sql, 'select * from `user` where ((`role` = \'admin\' and `age` = 30) or (`name` = \'John\')) or (`role` = \'dev\' and `age` = 22)')+ assert.deepEqual(sql, 'select * from "user" where (("role" = \'admin\' and "age" = 30) or ("name" = \'John\')) or ("role" = \'dev\' and "age" = 22)') }) describe('Custom/override query operators', function () { it('should use custom query operator if provided', function () { The changes in this PR result in additional code path, so coverage has dropped a tiny bit, but since testing the change is dependent upon choice of database driver, I don't think there's a good way around it. |
@eldridge do you think it's worth bumping dependencies that are behind as well in a new release with this? |
1 similar comment
@eldridge do you think it's worth bumping dependencies that are behind as well in a new release with this? |
@crobinson42 I don't have strong opinions there. It's nice to keep dependencies up to date, but I think I want to look at what's changed in knex and any other dependencies since then. I'm using knex 0.19.4 on a new project, but I've been focusing on some other external integrations and have yet to really put the data model through its paces. So far, this has been the only thing that I've run into. |
Ok, how about I bump dependencies and publish an rc that we can use in our development or staging environments for awhile to see if there are any issues? … On Jan 23, 2020, at 11:28 AM, Mike Eldridge ***@***.***> wrote:@crobinson42 <https://github.com/crobinson42> I don't have strong opinions there. It's nice to keep dependencies up to date, but I think I want to look at what's changed in knex and any other dependencies since then. I'm using knex 0.19.4 on a new project, but I've been focusing on some other external integrations and have yet to really put the data model through its paces. So far, this has been the only thing that I've run into. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#78?email_source=notifications&email_token=ABNSMS42V5WUEWQGF4TTGD3Q7HVUBA5CNFSM4KG4KQKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJYROEI#issuecomment-577836817>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNSMS6HFS2SLU2XVNYKYGLQ7HVUBANCNFSM4KG4KQKA>. |
Uh oh!
There was an error while loading.Please reload this page.
Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)
npm testsucceeds