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
Merged

Fix issues around specifying default values for columns #3318

merged 3 commits into from Jun 30, 2019

Conversation

lorefnon
Copy link
Collaborator

@lorefnon lorefnon 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).

@kibertoad
Copy link
Collaborator

What makes this WIP?

@lorefnon
Copy link
Collaborator Author

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
Copy link
Collaborator Author

lorefnon 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
Copy link
Collaborator

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

@kibertoad
Copy link
Collaborator

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.

@elhigu
Copy link
Member

elhigu 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
Copy link
Member

elhigu commented Jun 30, 2019

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

@lorefnon
Copy link
Collaborator Author

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 changed the title WIP: Update defaultTo to stringify jsonb columns. Fixes #3167 Update defaultTo to stringify jsonb columns. Fixes #3167 Jun 30, 2019
@lorefnon
Copy link
Collaborator Author

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
Copy link
Collaborator

kibertoad 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
Copy link
Collaborator

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

@lorefnon
Copy link
Collaborator Author

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
Copy link
Collaborator

@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.

…igned for text & blob columns, both of which work fine
@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
Copy link
Collaborator Author

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

This is fixed now.

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.

None yet

3 participants