Skip to content

Add JSON support in query and response #1299

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Aminadav
Copy link

@Aminadav Aminadav commented Dec 1, 2015

Now you can pass JSON object to insert/update queries, and it will stringify it.
When you run "select", it will automatically parse it to JSON object (if it success)

Using Example:

Inserting:

      conn.query('insert into table ?',{json_column:{json_object:true }},function(){
        } )

Quering:

    conn.query('select json_column from table',function(err,rows){
            console.log('rows=',rows    )
            console.log('json_object=',rows[0].json_column.json_object)
        } )

Result:

    rows= [
                 {json_column: { json_object: { me: true } } }
              ]
     json_object = true

As you can see. The JSON object converted to string for storing, and back to object, when quering.

@Aminadav
Copy link
Author

Aminadav commented Dec 1, 2015

Who see this commit, and want this merge (until it will accepted) you can manually install it using:

npm i @aminadav/node-mysql

Readme.md Outdated
[coveralls-url]: https://coveralls.io/r/felixge/node-mysql?branch=master
[downloads-image]: https://img.shields.io/npm/dm/mysql.svg
[downloads-url]: https://npmjs.org/package/mysql
https://github.com/felixge/node-mysql/pull/1299
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the README change, or we can't actually merge your PR.

@dougwilson
Copy link
Member

Thank you for this PR! I added a few comments and also, we need you to add tests for all the new functionality and add documentation to the README for the functionality as well. Also, your changes case many of our tests to fail, so please look into the failures and get the existing tests to pass.

@Aminadav
Copy link
Author

Aminadav commented Dec 2, 2015

Do I need to change the version number in package.json?

@AdriVanHoudt
Copy link

I think reverting that autocommit will do a lot?

@Aminadav
Copy link
Author

Aminadav commented Dec 2, 2015

I have reverting it... didn't pushed it yet

@Aminadav
Copy link
Author

Aminadav commented Dec 2, 2015

Now you can pass JSON object to insert/update queries, and it will stringify it.
When you run "select", it will automatically parse it to JSON object (if it success)

Using Example:

Inserting:

      conn.query('insert into table ?',{json_column:{json_object:true }},function(){
        } )

Quering:

    conn.query('select json_column from table',function(err,rows){
            console.log('rows=',rows    )
            console.log('json_object=',rows[0].json_column.json_object)
        } )

Result:

    rows= [
                 {json_column: { json_object: { me: true } } }
              ]
     json_object = true

As you can see. The JSON object converted to string for storing, and back to object, when quering.

@AdriVanHoudt
Copy link

I seems the tests are failing because of linting issues
for easiness ;) :

/home/travis/build/felixge/node-mysql/lib/protocol/Parser.js
  301:11  error  Missing semicolon      semi
  304:29  error  Missing semicolon      semi
  305:43  error  Missing semicolon      semi
  307:15  error  Empty block statement  no-empty
/home/travis/build/felixge/node-mysql/lib/protocol/SqlString.js
  42:55  error  Missing semicolon  semi
  47:36  error  Missing semicolon  semi
/home/travis/build/felixge/node-mysql/index.js
   3:1  warning  Missing JSDoc parameter description for 'config'            valid-jsdoc
   3:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  15:1  warning  Missing JSDoc parameter description for 'config'            valid-jsdoc
  15:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  27:1  warning  Missing JSDoc parameter description for 'config'            valid-jsdoc
  27:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  38:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  38:1  warning  Missing JSDoc for parameter 'sql'                           valid-jsdoc
  38:1  warning  Missing JSDoc for parameter 'values'                        valid-jsdoc
  38:1  warning  Missing JSDoc for parameter 'callback'                      valid-jsdoc
  48:1  warning  Missing JSDoc parameter description for 'value'             valid-jsdoc
  48:1  warning  Missing JSDoc parameter description for 'stringifyObjects'  valid-jsdoc
  48:1  warning  Missing JSDoc parameter description for 'timeZone'          valid-jsdoc
  48:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  61:1  warning  Missing JSDoc parameter description for 'value'             valid-jsdoc
  61:1  warning  Missing JSDoc parameter description for 'forbidQualified'   valid-jsdoc
  61:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  73:1  warning  Missing JSDoc parameter description for 'sql'               valid-jsdoc
  73:1  warning  Missing JSDoc parameter description for 'values'            valid-jsdoc
  73:1  warning  Missing JSDoc parameter description for 'stringifyObjects'  valid-jsdoc
  73:1  warning  Missing JSDoc parameter description for 'timeZone'          valid-jsdoc
  73:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  95:1  warning  Missing JSDoc @returns for function                         valid-jsdoc
  95:1  warning  Missing JSDoc for parameter 'className'                     valid-jsdoc

@Aminadav
Copy link
Author

Aminadav commented Dec 2, 2015

Fixed

@dougwilson
Copy link
Member

Hi @AminaG , unfortunately this PR is not backwards-compatible, so there is no timeline when we could ever incorporate the changes. The reason is that the changes here look at all columns and sniff their string contents to see if they look like they are a JSON object and parse it, which will break a lot of people.

This PR has the following two issues that need to be addressed:

  1. People currently storing JSON objects in their existing string columns should not suddenly get back JSON objects. This will cause their code to break.
  2. We do not want to introduce any new "maybe types" into this code. This introduces a maybe type, because the given column may be a string or an object, depending on what was in the column. This makes everyone's code very verbose, as they will have to always add guards everywhere.

@dougwilson
Copy link
Member

Also, as far as I can tell, you only have tests for the escaping part; we need tests for the parsing part as well.

@Aminadav
Copy link
Author

Aminadav commented Dec 3, 2015

What are you suggesting?

You right. if a end-user for example enter in text input field for his name {a:'1'} then it will be returned as object, but it should be string.

@naturalethic
Copy link

Seems this should be implemented in lib/protocol/packets/RowDataPacket.js:typeCast, the same way dates are.

https://github.com/felixge/node-mysql/blob/master/lib/protocol/constants/types.js#L23
https://github.com/felixge/node-mysql/blob/master/lib/protocol/packets/RowDataPacket.js#L53

@naturalethic
Copy link

I can take this over, let me know.

@dougwilson dougwilson force-pushed the master branch 4 times, most recently from 5847ec8 to be37e88 Compare May 8, 2016 05:09
@dougwilson dougwilson force-pushed the master branch 3 times, most recently from 65c4c0c to fa96a75 Compare June 8, 2016 02:00
@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 638d79d to 88bade0 Compare June 29, 2017 18:13
@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 946727b to 37fbbdd Compare November 18, 2018 02:26
dveeden pushed a commit to dveeden/mysql that referenced this pull request Jan 31, 2023
there's no need to set conn's readline if CheckConnLiveness is false,
and the ReadTimeout shall work with rawConn.Read inside conncheck.
buffer.fill will do SetReadDeadline if needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants