sequelize: Escaped $ signs throw bind parameter errors when they should not

Issue Description

What are you doing?

I am trying to use sequelize.fn with JSON_MERGE_PATCH to update a JSON column in a MySQL database with text containing a $. I noticed buggy behavior with (1) the escaping of $ signs for sequelize.fn and then (2) more inconsistent behavior related to the regex for determining bind parameters.

These result in false “Error: Named bind parameter “$status” has no value in the given object.” errors. I was able to determine the source of the issues and potential fixes.

This references two closed issues and PRs around similar $ problems that have introduced some unintended side effects.

1. Sequelize.fn doesn’t work with bind replacements if value has $ in it. (https://github.com/sequelize/sequelize/issues/11533)

PR: https://github.com/sequelize/sequelize/pull/11606

Issue

This solution in this PR was to add an escape in sequelize.fn text in abstract/query-generator, but it only escapes the first $ sign, so will still throw errors if there is more than one dollar sign.

class MyModel extends Sequelize.Model {}

const schema = {
  id: {
    type: Sequelize.STRING,
    primaryKey: true
  },
  metadata: {
    type: Sequelize.JSON
  }
};

MyModel.init(schema, { tableName: 'my_table', sequelize });

await MyModel.upsert({
  id: 'abc',
  metadata: {
    foo: {
      bar: true
    },
    description: 'hello'
  }
});

let myModel = await MyModel.findByPk('abc');

const update = {
  description: 'text with literal $1 and literal $status'
};

await myModel.update({
  metadata: Sequelize.fn('JSON_MERGE_PATCH', Sequelize.col('metadata'), JSON.stringify(update))
});

// Only the first dollar sign is escaped.
// UPDATE `my_table` SET `metadata`=JSON_MERGE_PATCH(`metadata`, '{\"description\":\"text with literal $$1 and literal $status\"}') WHERE `id` = $1

Solution

The solution here is to update this line to do a global replace of $. https://github.com/sequelize/sequelize/blob/master/src/dialects/abstract/query-generator.js#L2365

return this.escape(typeof arg === 'string' ? arg.replace('$', '$$$') : arg);

should become:

return this.escape(typeof arg === 'string' ? arg.replace(/\$/g, '$$$') : arg);

This solves that issue, but then there is another issue with the regex for determining bind parameters improperly matching escaped $ ($$) as a side effect of the next issue.

2. When Sequelize tries to replace binding value, it also tries to replace table name which has “$” sign. ( https://github.com/sequelize/sequelize/issues/12203)

PR: https://github.com/sequelize/sequelize/pull/12250

Issue

The issue that was being worked on here is related to tablenames having a single $ in them and being interpreted as bind parameters.

INSERT INTO [RI_Contracts_Names$DIM] ([Version],[ID],[Position],[Value]) OUTPUT INSERTED.* VALUES ($1,$2,$3,$4);

The solution in this PR was to add a word boundary \B to the regex for determining bind parameters in abstract/query. The addition of a word boundary breaks escaped $’s in the middle of sentences. It sees the first $ as a non-word character after a word character and thus a new boundary, and so something like hello$$world will match $world as a parameter to bind and throw an error even though it is properly escaped.

Before this PR the regex matches all properly escaped $ ($$) correctly.

After this PR the regex no longer matches all properly escaped $ ($$).

  const update = {
    description: 'hello$world'
  };

  await myModel.update({
    metadata: Sequelize.fn('JSON_MERGE_PATCH', Sequelize.col('metadata'), JSON.stringify(update))
  });

// Properly escaped in sql statement
// UPDATE `my_table` SET `metadata`=JSON_MERGE_PATCH(`metadata`, '{\"description\":\"hello$$world\"}') WHERE `id` = $1

// Results in: Error: Named bind parameter "$world" has no value in the given object.

Solution

I think here the solution is to revert this PR and require escapes on $ signs as I believe is intended.

https://github.com/sequelize/sequelize/blob/master/src/dialects/abstract/query.js#L84 /\B\$(\$|\w+)/g would go back to /\$(\$|\w+)/g

Alternatively, the regex could be adjusted to handle all of these cases instead of the current inconsistent behavior:

• A single $ sign in the middle of a string (the issue with the tablenames above)
• An escaped $ ($$)at the beginning of a string
• An escaped $ ($$) at the end of a string
• An escaped $ ($$) in the middle of a string
• Consecutive escaped $$ ($$$$) should not throw bind parameter errors

I’m not great with regex, so I don’t know what that would look like.

What do you expect to happen?

I expect escaped dollar signs to not match as bind parameters.

What is actually happening?

Escaped $ inconsistently match as bind parameters and throw an error.

Error: Named bind parameter "$world" has no value in the given object.

Additional context

I think the fix for the first one is really easy. The second issue might require some discussion.

I am willing to contribute, but I probably don’t have enough time for a couple of weeks, and for the second issue I would likely need help coming up with a regex that fits all the criteria around $ if that route is chosen as the solution.

Environment

• Sequelize version: 6.1.0
• Node.js version: 12.16.1
• MySQL: 5.7.22
• Operating System: macOs High Sierra

Issue Template Checklist

How does this problem relate to dialects?

• I think this problem happens regardless of the dialect.
• I think this problem happens only for the following dialect(s):
• I don’t know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don’t know how to start, I would need guidance.
• No, I don’t have the time, although I believe I could do it if I had the time…
• No, I don’t have the time and I wouldn’t even know how to start.