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

ARRAY(HSTORE) parsing error on linebreaks #3383

Closed
BridgeAR opened this Issue Mar 20, 2015 · 9 comments

Comments

2 participants
@BridgeAR
Contributor

BridgeAR commented Mar 20, 2015

Hey,

if someone uses newlines in hstores and saves that data everything is going to be fine. Reading though is going to fail here, since JSON.parse is not able to parse linebreaks.

So they have to be escaped before inserting or otherwise the reading can't be done with JSON.parse / linebreaks have to be escaped before reading with JSON.parse.

Here's a stacktrace with commit 2eadf0c.
The code changes done since then did not fix the issue.

SyntaxError: Unexpected token at Object.parse (native)
at /app/node_modules/sequelize/lib/dialects/postgres/query.js:21:24
at forOwn (/app/node_modules/sequelize/node_modules/lodash/dist/lodash.js:2105:15)
at Function.forEach (/app/node_modules/sequelize/node_modules/lodash/dist/lodash.js:3302:9)
at parseHstoreFields (/app/node_modules/sequelize/lib/dialects/postgres/query.js:15:11)
at /app/node_modules/sequelize/lib/dialects/postgres/query.js:182:13
at Array.forEach (native)
at /app/node_modules/sequelize/lib/dialects/postgres/query.js:181:16
at Promise._settlePromiseAt (/app/node_modules/sequelize/lib/promise.js:76:18)
From previous event:
at new SequelizePromise (/app/node_modules/sequelize/lib/promise.js:28:17)
at module.exports.Query.run (/app/node_modules/sequelize/lib/dialects/postgres/query.js:70:19)
at /app/node_modules/sequelize/lib/sequelize.js:724:20
at Promise._settlePromiseAt (/app/node_modules/sequelize/lib/promise.js:76:18)
From previous event:
at module.exports.Sequelize.query (/app/node_modules/sequelize/lib/sequelize.js:720:20)
at module.exports.QueryInterface.select (/app/node_modules/sequelize/lib/query-interface.js:733:27) at null. (/app/node_modules/sequelize/lib/model.js:739:34)
at Promise._settlePromiseAt (/app/node_modules/sequelize/lib/promise.js:76:18)
From previous event:
at module.exports.Model.findAll (/app/node_modules/sequelize/lib/model.js:697:20) at ...

@BridgeAR BridgeAR changed the title from HStore parsing error on linebreaks to ARRAY(HSTORE) parsing error on linebreaks Mar 20, 2015

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 20, 2015

Hmm there shouldn't really be any reason to involve json here at all. There hstore file should be able to handle all parsing...

It seems the code is creating a fake array by concating [] around the value and parsing that. Don't ask me why :-).

PR with test or a test case here in the thread would be welcomed

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Mar 20, 2015

I'll have a closer look at it later on.

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Mar 21, 2015

Well the best fix I could come up with right now is doing the following:

    return DataTypes.ARRAY.is(options.dataType, DataTypes.HSTORE) ?
      Utils._.map(JSON.parse('[' +
        value.substring(1, value.length - 1)
          .replace('\n', '\\n')
          .replace('\t', '\\t')
          .replace('\r', '\\r') +
          ']'
        ), function (v) {
          return hstore.parse(v);
        }) : hstore.parse(value);

As far as I can see it, @seth-admittedly did the right thing by using JSON.parse.

Postgres returns the Array as {""type"=>"mobile", ""type"=>"landline""}, that's the reason why he removes the first and last char and creates the fake array. The problem is, that \n\t etc. are not escaped in HStores and as far as I can see, should not be escaped.

By escaping these values before parsing the Array we bypass that problem.

There might be a nicer solution but I'm to tired to think of any at the moment.

Testwise it's easy to insert a \n or something into https://github.com/sequelize/sequelize/blob/master/test/integration/dialects/postgres/dao.test.js#L543

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Mar 21, 2015

@janmeier would you be so kind and point me to the part where you parse arrays? The search function through my cellphone is limited ;)
I guess the best way to resolve the issue is by using the array parsing already in place and parse the hstore afterwards - depending on the array parsing implementation used right now.

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 22, 2015

@BridgeAR Array parsing for hstore happens right at the place you already found. And yes, after reviewing this again, it seems your suggestion of escaping the line breaks would be fine

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Mar 22, 2015

@janmeier I still think there's a better solution to this. I know that the ARRAY(HSTORE) ist parsed right at that spot, but I wanted to know where you parse things like ARRAY(TEXT/STRING/NUMBER...). If that code part does not use JSON.parse it could be used for the array parsing with HSTORES

@janmeier janmeier closed this in a1a3107 Mar 23, 2015

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 23, 2015

Thanks for helping in debugging this @BridgeAR :)

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented Mar 23, 2015

This does not fix the issue, since I forgot to use the replace global.
It should be

.replace(/\n/g, '\\n')

But as we've spoken before I'll check if there's a better solution for this.

janmeier added a commit that referenced this issue Mar 24, 2015

@janmeier

This comment has been minimized.

Member

janmeier commented Mar 24, 2015

I've changed the code to use pg.types.arrayParser as we spoke about yesterday - much cleaner now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment