fix(bind): add escsape of null character for postgres bind parameters #10716
Conversation
holm
force-pushed
the
fix/postgres_escape
branch
from
f7e2238
to
b30431f
Compare
Codecov Report
@@ Coverage Diff @@
## master #10716 +/- ##
==========================================
+ Coverage 96.3% 96.3% +<.01%
==========================================
Files 93 93
Lines 8976 8980 +4
==========================================
+ Hits 8644 8648 +4
Misses 332 332
Continue to review full report at Codecov.
|
lib/dialects/postgres/query.js
Outdated
| @@ -20,11 +20,20 @@ class Query extends AbstractQuery { | |||
| * @private | |||
| */ | |||
| static formatBindParameters(sql, values, dialect) { | |||
| let bindParam = []; | |||
| const stringReplaceFunc = value => { | |||
| if (_.isString(value)) { | |||
There was a problem hiding this comment.
Nit: You can make this value => typeof value === 'string' ? value.replace(...) : value
There was a problem hiding this comment.
I thought that wasn’t a complete check due to the issues explained at https://stackoverflow.com/questions/4059147/check-if-a-variable-is-a-string-in-javascript
There was a problem hiding this comment.
Most of the codebase already uses typeof checks, https://stackoverflow.com/questions/5750656/whats-the-point-of-new-stringx-in-javascript explains the debacle pretty well.
In the end, sequelize doesn't really support these boxed types and I don't think it should as it would increase complexity in hundreds of places.
holm
force-pushed
the
fix/postgres_escape
branch
from
b30431f
to
7763561
Compare
|
I addressed the comment now. The build failure doesn't seem related to my changes. |
|
🎉 This PR is included in version 5.2.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Thanks for the quick merge and release |
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
Postgres doesn't support null characters in strings. This has generally been handled via https://github.com/sequelize/sequelize/blob/master/lib/sql-string.js#L75. However with Sequelize 5 using bind parameters for insert and update, the replacement of null characters no longer happens. This PR adds replacement of null characters in case of bind parameters being used.
I have added tests, but have not been able to run them across the other database. I think it should pass though, otherwise I would appreciate help getting them fixed.