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

Fix issues around specifying default values for columns #3318

Merged
merged 3 commits into from Jun 30, 2019

Conversation

Projects
None yet
3 participants
@lorefnon
Copy link
Contributor

commented Jun 30, 2019

This PR addresses following issues:

  1. SQL Server allows default values for BLOB/nvarchar but knex ignores defaults for these types. This PR removes that behavior.

  2. Setting default value of JSON(B) fields don't work and results in invalid syntax for pretty much all dialects (unless knex.raw is explicitly used).

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch 4 times, most recently from 194e412 to 8fd4736 Jun 30, 2019

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

What makes this WIP?

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Some mysql & mssql issue. I added an integration test and it seems like default value for json were failing for them. Checking on it.

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch 4 times, most recently from 69ed2c4 to 8158eb3 Jun 30, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Is the mysql image specified in docker-compose.yml actually used in the tests ? It appears that even though the latest version is specified, the tests are run against some older mysql version.

The Build System Information section in Travis logs, show the following:

mysql version
mysql Ver 14.14 Distrib 5.7.25, for Linux (x86_64) using EditLine wrapper

(which is outdated)


Do we want to support older versions of mysql (which didn't support default values for json columns) ?

From MySQL docs:

Prior to MySQL 8.0.13, a JSON column cannot have a non-NULL default value.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

@lorefnon Hmmm, good question, I wonder where else it could come from. travis.yml doesn't specify any alternative versions.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

I would say that since we made a switch to run tests on MySQL 8, it would make sense to run them on latest one. As long as older versions are still supposed to work when not using the new functionality, we should be good.

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch from 3a8a6d9 to 8602c03 Jun 30, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

Im pretty sure that docker version is used. The one coming from travis is the old one. Docker version is running in different port.

@elhigu

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

I think using latest version is better than fixing version to 8

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch from 8602c03 to 41c5d04 Jun 30, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Yes, I have verified that now. Something else is wrong. I'll remove the second commit before removing WIP. I fixed the version just to be sure.

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch from 41c5d04 to 39dbc98 Jun 30, 2019

@lorefnon lorefnon changed the title WIP: Update defaultTo to stringify jsonb columns. Fixes #3167 Update defaultTo to stringify jsonb columns. Fixes #3167 Jun 30, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Sorry about the spam about incorrect version.

The problem was that mysql allows default values for json, only when they are expressions. So json string literals need to be wrapped in parenthesis.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

Would be cool to also have an integration test which would verify that defaults actually kick in when inserting new rows.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

new test seems to be failing in pg, mysql and mssql.

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch from c1d796f to 2b498fd Jun 30, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Is it expected that in dialects other than postgres, json columns are returned as string ? I initially thought something was wrong with how default value was getting populated but it seems unrelated:

λ node --experimental-repl-await
> knex = require('knex')({client: 'sqlite3', connection: {filename: ':memory:'}})

> await knex.schema.createTable('users', (t) => { t.increments(); t.json('metadata'); })
[]

> await knex('users').insert({id: 1, metadata: {foo: 'bar'}})
[ 1 ]

> await knex('users').where('id', 1).first()
{ id: 1, metadata: null } 
// The metadata is gone

> knex('users').insert({id: 1, metadata: {foo: 'bar'}}).toSQL()
{ method: 'insert',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ 1, { foo: 'bar' } ],
  __knexQueryUid: '29507f8d-f2ae-47a6-b2cc-8c2f544bd582',
  sql: 'insert into `users` (`id`, `metadata`) values (?, ?)' }

> knex('users').insert({id: 2, metadata: JSON.stringify({foo: 'bar'})}).toSQL()
{ method: 'insert',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ 2, '{"foo":"bar"}' ],
  __knexQueryUid: 'cb2455a1-64a1-499e-b8ee-19980000074a',
  sql: 'insert into `users` (`id`, `metadata`) values (?, ?)' }

> await knex('users').insert({id: 2, metadata: JSON.stringify({foo: 'bar'})})
[ 2 ]

> await knex('users').where('id', 2).first()
{ id: 2, metadata: '{"foo":"bar"}' }
// metadata is stringified json

> await knex('sqlite_master').where({name: 'users'})
[ { type: 'table',
    name: 'users',
    tbl_name: 'users',
    rootpage: 2,
    sql:
     'CREATE TABLE `users` (`id` integer not null primary key autoincrement, `metadata` json)' } ]
// (Verified that this uses json column and not text)

In postgres though querying json column returns javascript objects, which imho is the ideal thing to do.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jun 30, 2019

@lorefnon I think this is leftover from the times where those databases didn't support proper JSON columns so we were storing them as strings. I agree that javascript objects make more sense, but that would be a breaking change; probaably we can do it in 0.19.0 after 0.18 branch quiets down a bit.

Remove condition preventing SQL server from having default values ass…
…igned for text & blob columns, both of which work fine

@lorefnon lorefnon force-pushed the lorefnon:#3167 branch from 819ee29 to 9c6c99d Jun 30, 2019

@lorefnon lorefnon changed the title Update defaultTo to stringify jsonb columns. Fixes #3167 Fix issues around specifying default values for columns Jun 30, 2019

@lorefnon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

new test seems to be failing in pg, mysql and mssql.

This is fixed now.

@kibertoad kibertoad merged commit 56c3af8 into tgriesser:master Jun 30, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.005%) to 88.062%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.