Add json support to the query builder for mysql #1902

baronfel · 2017-02-07T21:35:32Z

This is a minimal amount of work to make the json(COLUMN_NAME) method on the query builder do the right thing for versions of MySQL greater than or equal to 5.7.8, when the json data type first came into being.

It follows most of the patterns put forth for the postgres json support, including retrieving the version from the target server and using that to determine support levels.

I've included tests patterned after the postgres tests which pass on my local mysql instance.

Anything else I should do?

Fixes #1036 for mysql only.

…ort it

baronfel · 2017-02-08T18:04:02Z

I don't know why the tests can't connect to mariadb now. Anyone have any pointers?

elhigu · 2017-02-08T19:51:48Z

Looks like travis has too old mysql server, you need to change .travis.yml file somehow to get newer version installed

mysql --version
mysql  Ver 14.14 Distrib 5.5.53, for debian-linux-gnu (x86_64) using readline 6.2

elhigu · 2017-02-08T19:57:25Z

src/dialects/maria/schema/columncompiler.js

+    // mariadb doesn't support json as of yet. 
+    // see https://jira.mariadb.org/browse/MDEV-9056 for issue status.
+  json () {
+    return 'text';


I dont see any value of using text column instead of json, since you won'r be able to run any json queries for those columns anyways. I don't know what has been original reasoning for using json as text columns

this is here because maria doesn't have the json data type, and so needs to override the inherited definition of json with text so that table creation statements don't blow up.

Shouldn't it blow up if database is asked to create json column, but it doesn't support it?

that would be a breaking change with what already exists, but I wouldn't be opposed to it.

@wubzz @rhys-vdw @tgriesser any second opinions? This feature is already breaking change for people who has been using mysql >= 5.7 and .json() column type (probably not many or none).

@elhigu So for an updated PR, should current version be kept or all changes to MariaDB should be dropped?

baronfel · 2017-02-08T20:00:36Z

re: travis configuration, how did the mysql connection ever work, then? I didn't touch anything around the configuration at all, and yet what I see is a DB connection issue. Most mysterious.

elhigu · 2017-02-08T20:01:09Z

src/dialects/mysql/schema/columncompiler.js

+function jsonColumn(client) {
+  if(!client.version) {
+    return 'json'; // this is here jsut for tests
+  } else if(semver.valid(client.version) && semver.gte(client.version, MIN_JSON_VERSION)) {


I suppose this same check would work also for mariadb removing need for changes in its column compiler...

elhigu · 2017-02-08T20:02:51Z

src/dialects/mysql/schema/columncompiler.js

+  } else if(semver.valid(client.version) && semver.gte(client.version, MIN_JSON_VERSION)) {
+    return 'json';
+  } else {
+    return 'text';


I would just go with using type 'json' always if json column is built and leave it up to DB to throw error if it doesn't support it (IIRC it is how it is done in postgresql dialect).

it breaks backwards compatibility though and should be mentioned in CHANGELOG

That's not the case, see this snippet from postgres's columns compiler:

function jsonColumn(client, jsonb) { if (!client.version || parseFloat(client.version) >= 9.2) return jsonb ? 'jsonb' : 'json'; return 'text'; }

… mysql json handling

elhigu

I think knex shouldn't try to be too smart about deciding by DB version if json or text column is used. When queries expects column to be json, they will fail anyways.

Travis configuration needs to be updated to be able to test this.

CHANGELOG should mention about backwards compatibility change

baronfel · 2017-02-08T20:26:52Z

@elhigu thanks for the review! I'll wait a while for more comments to com in from those that you tagged and then work to incorporate the changes.

elhigu · 2017-02-16T22:50:08Z

Closing and opening to make travis to trigger build again with fresh .travis.yml config... there is still not mysql 5.7, but I updated it to trusty with 5.6... maybe later this year travis updates to provide easier installing for 5.7 too.

johannessjoberg · 2017-03-07T15:47:27Z

Any updates on this PR?

baronfel · 2017-03-07T15:51:09Z

Let me summarize and see if I've got it all straight:

We don't want to do the version-checking logic to determine which storage json should choose, instead we'll just always use json column type and let the database response blow up if the database doesn't support it
Therefore, the mariadb changes don't need to happen and can be rolled back\
Likewise, the version-checking changes in the core mysql driver can be rolled back
Finally, a CHANGELOG entry should be added to note that mysql will use json columns when a column is defined using the json builder method, which is breaking for people currently using it and expecting TEXT

sourcec0de · 2017-08-16T18:28:58Z

Hey everyone, thanks for your awesome work on knex.
I was hoping to get an update on the support for this feature in MySQL.
The new JSON features are great and it would be cool to be able to set a column as JSON in a migration.

UPDATE

🤕 just realized I can just use the table.specificType(column, value) in migrations.

elhigu · 2017-10-31T22:26:45Z

Just to set reminder for me here to be easily found from end of thread. This PR is left open, because it is waiting for travis to support easier installation of mysql 5.7 (or us to write docker based service setup there).

igor-savin-ht · 2018-05-23T00:48:15Z

@elhigu I presume travis-ci/travis-ci#5122 takes care of the mysql version issue? If I create an updated PR against latest master, is this change still good to go?

elhigu · 2018-05-23T10:18:16Z

@igor-savin-ht yes, this would be good to go if you are able to setup travis with mysql 5.7 👍

* Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version

* Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations

kibertoad · 2018-06-01T21:32:25Z

@elhigu This is redundant now and can be closed.

* Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations * Remove WebSQL dialect

* Kill queries after timeout for PostgreSQL * Fix cancellation connection acquiring and test. * Fix releasing connection in case query cancellation fails * Add support for native enums on Postgres (#2632) Reference https://www.postgresql.org/docs/current/static/sql-createtype.html Closes #394 Signed-off-by: Will Soto <will.soto9@gmail.com> * Removal of 'skim' (#2520) * Allow overwriting log functions (#2625) * Example build of custom log functions * Handle logger object better for transactions * Adjust test to ignore sqlite warning message * Fixed onIn with empty values array (#2513) * Drop support for strong-oracle (#2487) * Remove babel-plugin-lodash (#2634) While in theory, this may reduce the bundle size, in practice it adds a ton of overhead during startup due to the number of additional requires. Bundle size also shouldn't matter for server side modules. * Add json support to the query builder for mysql (#2635) * Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * fix wrapIdentifier not being called in postgres alter column (#2612) * fix wrapIdentifier not being called in postgres alter column * add test for wrapIdentifier call in postgres alter column * add comment regarding issue * add issue & PR #'s in comment * Remove readable-stream and safe-buffer (#2640) * Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations * fixes 2630 (#2642) * Timeout errors shouldn't silently ignore the passed errors, but rather reject with original error. Fixes #2582 (#2626) * chore: add Node.js 10 (#2594) * chore: add Node.js 10 * chore: trigger new build * chore: update lockfile * chore: trigger new build * fix: use npm i instead of npm ci * Fix mssql driver crashing (#2637) * Run SQL Server tests locally running SQL server in docker * WIP mssql test stuff * Patched MSSQL driver to not crash knex anymore * Removed semicolon from rollback stmt for oracle (#2564) * Remove WebSQL dialect (#2647) * Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations * Remove WebSQL dialect * add homepage field to package.json (#2650) * Make the stream catch errors in the query (#2638) * Make the stream catch errors in the query * Fix another case in which stream doesnt emits error * Linter stuff * Remove setTimeout in tests * Make a test not to check the MySQL error code * Fix stream error catching for MariaDB and PostgreSQL * Fix stream error catching in Oracle * Throw the error after emitting it to the stream * Throw the error without instantiating a new Error * Various fixes to mssql dialect (#2653) * Fixed float type of mssql to be float * Many tests where postgres test was not actually ran at all * Migrations to be mssql compatible Mssql driver doesn't handle if multiple queries are sent to same transaction concurrently. * Prevented mssql failing when invalid schema builder was executed by accident Instead of trying to generate sql from broken schema calls, just make exception to leak before query compiling is started * Fixed mssql trx rollback to always throw an error Also modified some connection test query to be mssql compatible * Fixed various bugs from MSSQL driver to make tests run * Fixed mssql unique index to be compatible with other dialect implementations * Enable running mssql tests on CI * Test for #2588 * Updated tests to not be dependend on tables left from previous test rans * Trying to make mssql server work on travis * Updated changelog and version * Drop mariadb support (#2681) * Dropped support for mariasql * ESLint * Fixed docs to build again * Fix knex.initialize() and adds test (#2477) * Fix knex.initialize() and adds test * Fix knex.initialize() and adds test * Added test for reinitializing pool after destroy * Fixed destroy / reinitialization test * Fixed the fix * Implement missing schema support for mssql dialect * chore: Update dependencies. Remove estraverse (#2691) * Update dependencies. Remove estraverse * Fix compatibility with new Sinon * Increase mssql timeout * Normalized and validated driverNames of test db clients and fixed oracle test setup (#2692) * Normalized and validated driverNames of test db clients and fixed oracle test setup * Fixed failed queries from old query building tests which hadn't been ran in ages * Allow selecting node version which is used to run oracledb docker tests * Improved sql tester error messages * Fixed rest of the oracledb tests * Removed invalid flag from docker-compose * Print mssql logs if initialization fails * Fixed syntax error + final tests * Added restart of failure for mssql DB initialization to try again if server was not ready * Printout always mssql logs after container is started * Fixed wait time printing after trying to connect * Use npm run oracledb:test for testing oracle in travis * Add Prettier (#2697) * Add prettier * Run files through prettier * Istanbul -> NYC (#2700) * istanbul -> nyc * Update mocha * Enforce code coverage (#2702) * Enforce code coverage * Update coverage numbers with current values * version assignment on base class, copy on tx client, fix #2705 * Update changelog * v0.15.1 * Added build step to test script. Fixes #2541 (#2712) * Revert "Added build step to test script. Fixes #2541 (#2712)" (#2714) This reverts commit 90ed8db. * Allow oracle failures for now * Fix issue with select(0) (#2711) * Fix issue with select(0). Fixes #2658 * Refactor migrator (#2695) * Refactor migrator * Fix exports * Fix ESLint * Fix migrator * Fix reference to config * Split some more * Fix table builder * Fix argument order * Merge branch 'master' of https://github.com/tgriesser/knex into feature/2690-support-multiple-migration-dirs # Conflicts: # src/migrate/index.js # test/index.js # test/unit/migrate/migrator.js * Fix #2715 (#2721) * Fix #2715, explicitly set precision in datetime & timestamp * Allow for precision in knex.fn.now, mysql time * Add test for datetime with precision * Bump changelog * 0.15.2 * Fix issues with warnPromise when migration does not return a promise. Fixes #2725 (#2730) * Add tests for multiple union arguments with callbacks and builders (#2749) * Add tests for multiple union arguments * Add some callback and raw tests to union unit tests * Compile with before update so that bindings are put in correct order (#2733) * Fix join using builder withSchema. (#2744) * Use Datetime2 for MSSQL datetime + timestamp types (#2757) * Use Datetime2 for MSSQL datetime + timestamp types Datetime2 is now the recommended column type for new date work: https://docs.microsoft.com/en-us/sql/t-sql/data-types/datetime-transact-sql?view=sql-server-2017 * Add tests for MSSQL datetime2 changes * General/document breaking change (#2774) * Add changelog entry for a breaking change * Improve entry * Allow timestamp with timezone on mssql databases (#2724) * Allow timestamp with timezone on mssql databases * Change timestamp parameter to use object instead of boolean * Update dependencies (#2772) * Feature/2690: Multiple migration directories (#2735) * Implement support for multiple migration directories * Add tests for new functionality * Fix tape tests * Pass migration directory upwards * Fix multiple directory support * Fix bugs * Rollback correctly * Fix remaining tests * Address comments * #2758: Implement fail-fast logic for dialect resolution (#2776) * Implement fail-fast logic for dialect resolution, clean-up code around. * Remove method that was deprecated long time ago * Address additional comments * Try addressing comments * Set client explicitly * Fix compatibility with older Node versions * Use columnize instead of wrap in using(). (#2713) * Use columnize instead of wrap in using(). This is an attempt to fix #2136. Also added an integration test, couldn't find any existing. * Exclude MSSQL from test as it does not support .using * Change test to not use subquery * Change test * Introduced abstraction for getting migrations (#2775) * Introduced abstraction for getting migrations This would allow a webpack migration source which is compatible with bundling. * Fixed migration validation with custom migration source * Fixed issues after rebasing on muti directory PR * Renamed parameter and fixed error message * Addressed some PR comments * Finished comment * Moved filename extension filtering into fs-migrator * Added test showing in memory custom migration source * Cleaned up how to get config * Fixed failing test * Hopefully fix tests * Fix Node.js 10 support in tests * Test for correctly releasing cancel query connection

baronfel added 3 commits February 7, 2017 15:32

Add json support to the query builder for mysql

3fb207b

ensure that mariadb doesn't use json columns, because they don't supp…

1b1bef5

…ort it

be more defensive about parsing version strings from servers

a9d92d8

baronfel force-pushed the mysql_json_column branch from 24758f8 to a9d92d8 Compare February 8, 2017 17:41

elhigu reviewed Feb 8, 2017

View reviewed changes

address some review notes around notifying of breaking changes around…

99c464d

… mysql json handling

elhigu requested changes Feb 8, 2017

View reviewed changes

elhigu closed this Feb 16, 2017

elhigu reopened this Feb 16, 2017

thetutlage mentioned this pull request May 12, 2017

Using MySQL JSON functions adonisjs/core#554

Closed

kibertoad mentioned this pull request May 29, 2018

Add json support to the query builder for mysql #2635

Merged

elhigu pushed a commit that referenced this pull request May 30, 2018

Add json support to the query builder for mysql (#2635)

4d4e4ca

* Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version

elhigu pushed a commit that referenced this pull request Jun 1, 2018

Remove readable-stream and safe-buffer (#2640)

d9cd54b

* Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations

elhigu closed this Jun 1, 2018

elhigu pushed a commit that referenced this pull request Jun 18, 2018

Remove WebSQL dialect (#2647)

d76cbff

* Add json support to the query builder for mysql refs #1036 Based on #1902 * Clarify supported version * Use native Buffer and Stream implementations * Remove WebSQL dialect

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add json support to the query builder for mysql #1902

Add json support to the query builder for mysql #1902

baronfel commented Feb 7, 2017

baronfel commented Feb 8, 2017

elhigu commented Feb 8, 2017

elhigu Feb 8, 2017

baronfel Feb 8, 2017

elhigu Feb 8, 2017

baronfel Feb 8, 2017

elhigu Feb 8, 2017

igor-savin-ht May 24, 2018

baronfel commented Feb 8, 2017

elhigu Feb 8, 2017

elhigu Feb 8, 2017

elhigu Feb 8, 2017

baronfel Feb 8, 2017

elhigu left a comment

baronfel commented Feb 8, 2017

elhigu commented Feb 16, 2017

johannessjoberg commented Mar 7, 2017

baronfel commented Mar 7, 2017

sourcec0de commented Aug 16, 2017 •

edited

Loading

elhigu commented Oct 31, 2017

igor-savin-ht commented May 23, 2018

elhigu commented May 23, 2018

kibertoad commented Jun 1, 2018

Add json support to the query builder for mysql #1902

Add json support to the query builder for mysql #1902

Conversation

baronfel commented Feb 7, 2017

baronfel commented Feb 8, 2017

elhigu commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baronfel commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elhigu left a comment

Choose a reason for hiding this comment

baronfel commented Feb 8, 2017

elhigu commented Feb 16, 2017

johannessjoberg commented Mar 7, 2017

baronfel commented Mar 7, 2017

sourcec0de commented Aug 16, 2017 • edited Loading

UPDATE

elhigu commented Oct 31, 2017

igor-savin-ht commented May 23, 2018

elhigu commented May 23, 2018

kibertoad commented Jun 1, 2018

sourcec0de commented Aug 16, 2017 •

edited

Loading