Sequelize broken with v2.3.1

[ddolcimascolo]

Hello,

Long-time user of sequelize + mysql2 here. Our build starts to fail when upgrading to v2.3.1 of your library. I didn't investigate deeply yet but the error we're getting is

SequelizeDatabaseError: Cannot read property 'parse' of undefined
  at Query.formatError (node_modules/sequelize/lib/dialects/mysql/query.js:265:16)
  at Query.run (node_modules/sequelize/lib/dialects/mysql/query.js:77:18)

I can also open an issue there if you feel the issue is on their side, but you released a minor version wich isn't supposed to contain breaking changes.

I can provide logs, reproduction repo, anything you'd need to debug.

Cheers, David

[JordanHood]

I'm also seeing this error across a range of my service's, though only ones that have a JSON column type in the table

[rnkhouse]

It's breaking with the 2.3.1 version. The error we get is: uncaughtException: The value of "offset" is out of range. It must be >= 0 and <= 1248. Received 1252

[edgedemon]

sequelize is broken with version 2.3.1 not with 2.3.0.

the problem comes from this file: https://github.com/sidorares/node-mysql2/blob/master/lib/parsers/text_parser.js you are using readCodeFor which applies JSON.parse and then assigning that to _this which ends with _this.JSON.parse(packet.readLengthCodedString("utf8")) instead of JSON.parse(_this.packet.readLengthCodedString("utf8"))

[ddolcimascolo]

@edgedemon You're totally right, I'm editing the issue title

[edgedemon]

easiest fix i can imagine is removing the JSON.parse from the readCodeFor function

and move it to the compile function

EDIT: there was an awful typo on the second code example is now fixed

[edgedemon]

made a pull request for this, https://github.com/sidorares/node-mysql2/pull/1419 hopefully the issue will get solved over the weekend.

[sidorares]

so to me this looks like a bug in code generated for typeCast

@edgedemon - your PR would change the semantics ( return json string when you query json field. Currently user expectation is to receive deserialized json ) - is that intentional? I'd prefer to just fix a bug where we do _this.JSON.parse(...

[ddolcimascolo]

Hi @sidorares

On a side note, deprecating the version on npm would be a great move forward too. Automated dependency upgrades are spreading in user's repositories...

Cheers, David

[sidorares]

@testn looks like #1402 is causing this regression ( typeCast + JSON field )

[sidorares]

@ddolcimascolo $ npm deprecate mysql2@2.3.1 "Regression when using typeCast + JSON fiels, see https://github.com/sidorares/node-mysql2/issues/1418"

it looks like the whole package is marked as deprecated now, though I followed steps on how to deprecate a single version in https://docs.npmjs.com/deprecating-and-undeprecating-packages-or-package-versions

[sidorares]

my attempt to fix: #1420 cc @testn @edgedemon @ddolcimascolo

[sidorares]

@rnkhouse I hope I fixed parse is undefined error, can you try master and see if uncaughtException: The value of "offset" is out of range. It must be >= 0 and <= 1248. Received 1252 is still there? Or even better, can you prepare a small example for me to reproduce

[testn]

@sidorares sorry I must have missed a case with JSON.

[sidorares]

@testn I'm a bit worried about uncaughtException: The value of "offset" is out of range. It must be >= 0 and <= 1248. Received 1252 - could it be we are reading data from wrong packet reference?

[testn]

Your fix is reasonable. @sidorares where do you see uncaughtException: The value of "offset" is out of range. It must be >= 0 and <= 1248. Received 1252?

[testn]

@mkhouse Do you have a repro?

[sidorares]

@ddolcimascolo published v2.3.2

[ddolcimascolo]

Thx @sidorares, my CI runs. I'll keep you posted here

[ddolcimascolo]

@sidorares Fixed! Thx for your help

[edgedemon]

@sidorares the only issue i see with your code is you are duplicating the memory used for reading each package since you are asigning another variable and passing the whole content to it, each time for each package of each field, and that doesnt sit well with me.

_this.JSON.parse(... is exactly the issue

im pretty sure my code in the line 126, would translate to this which is JSON.parse(_this.packet

the issue would be on line 134 where you have this

parserFn(${lvalue} = ${readCode};);

not sure if it would be easier to just use 2 diferent code generators, your file would be slightly bigger but you would maintain functionality without wasting memmory.

[edgedemon]

easier yet add an optional parameter on the readCodeFor function with default true and set to false in the call of the line 126 i did it here https://github.com/edgedemon/node-mysql2/blob/fix/json-parsing-read-next/lib/parsers/text_parser.js

im making one extra if check at generation time for JSON type only, and saving memory over the acepted answer

[testn]

@sidorares let's close this?