I just hope this doesn't introduce a breaking change, but in theory it will only be a breaking change if the feature was used 'incorrectly' (used a replacement in a place where a replacement should not be treated as one, like inside of a string).

I will skip the mysql 8 tests: It would seem we are experiencing the same issue as we did with mariadb #14174

This is the cause of #14509
Not sure why Generate docs check passes while there is clearly a SyntaxError for it. @ephys would you be able to check it out?

🎉 This PR is included in version 6.19.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

This just broke my deployment, so it's a breaking change for me at the very least.

"in theory it will only be a breaking change if the feature was used 'incorrectly' (used a replacement in a place where a replacement should not be treated as one, like inside of a string)"

Not sure what you mean by replacements being used incorrectly inside a string.

The docs do this:

await sequelize.query(
  'SELECT * FROM projects WHERE status = :status',
  {
    replacements: { status: 'active' },
    type: QueryTypes.SELECT
  }
);

https://sequelize.org/docs/v6/core-concepts/raw-queries/

Please, if you break things in a minor release update then please provide an example how to use replacements with raw queries. Why is it incorrect usage?

By 'using a replacement inside a string' i meant a sql string, something like this;

await sequelize.query(
  `SELECT * FROM projects WHERE status = ':status'`,
  {
    replacements: { status: 'active' },
    type: QueryTypes.SELECT
  }
);

Note the quotes around :status. Injecting a replacement there would have resulted in sql injection. That's the thing we changed.

Your usage is correct and should not have broken. We'll look into what happened there.

this is my usage, I use it to call sql functions, I don't have any strings there

const res: bulkItem[] = await database.query(`SELECT "portfolioInvestmentId", value FROM ${metric}(:fundId, array[:pciIds]::uuid[], :untilDate, :excludeEventId)`, {
      type: QueryTypes.SELECT,
      replacements: {
        fundId,
        pciIds,
        untilDate: dateToStr(untilDate),
        excludeEventId,
      },

This results in

SELECT "portfolioInvestmentId", value FROM table('abc', array[:pciIds]::uuid[], 'ijk', 'lmn')

Looks like the culprit is array[:pciIds]::uuid[]. Technically it's not a valid spot for a SQL value, but v6 doesn't have a way to specify the type of a replacement and this is breaking a legitimate workaround to that so we'll fix the parser in v6 to allow it.

PR: #14518

I had 2 cases where using replacements broke. The first was with using it with LIMIT doing something like the below

await sequelize.query(
  'SELECT * FROM projects WHERE status = :status LIMIT :first',
  {
    replacements: { status: 'active', first: 20 },
    type: QueryTypes.SELECT
  }
);

The 2nd case was with using it with an array as well, such as WHERE "tags" @> ARRAY[:tags]::TEXT[]. Seems like the PR you submitted should fix this 2nd case but not the first

I just ran it against our latest v6 and it resulted in SELECT * FROM projects WHERE status = 'active' LIMIT 20

There may be something more going on. If you'd rather avoid posting the real query in public, you can send it to me privately on our slack (if you're comfortable with that), and I'll look into it.

Edit: It's resolved :)

Sadly this wasn't marked as breaking changes. It breaks my usage, where I previously used for INTERVAL, and it was working fine before:

CASE WHEN ("Appointments"."canceledAt" >= ("Appointments"."startDate" - INTERVAL ':cutOffPeriod hours')) THEN
     'cancelled < :cutOffPeriod hours'

In this case, the variable needs to be inside string

Any suggestion how to solve this

We apologize for that but unfortunately that is considered to be a bug that was severe enough to warrant patching in all versions as it lead to SQL injections.

In your case, unless you ensured that cutOffPeriod was a number, you were vulnerable to SQL injections. If you do ensure the input is only a number, then you can concatenate your variable in your string, it's unsafe, but it's not more unsafe than what you were doing previously:

await sequelize.query(
  `CASE WHEN ("Appointments"."canceledAt" >= ("Appointments"."startDate" - INTERVAL '${cutOffPeriod} hours')) THEN 'cancelled < ${cutOffPeriod} hours'`,
  {
    type: QueryTypes.SELECT
  }
);

Or you could do this, which is safe:

await sequelize.query(
  `CASE WHEN ("Appointments"."canceledAt" >= ("Appointments"."startDate" - INTERVAL :cutOff1)) THEN :cutOff2`,
  {
    replacements: {
      cutOff1: `${cutOffPeriod} hours`,
      cutOff2: `cancelled < ${cutOffPeriod} hours`,
    },
    type: QueryTypes.SELECT
  }
);

@ephys i see, if thats the concern that make sense to patch such thing. In my case, it's all constructed internally, so it wasn't really a problem.

And thanks for the solution! That'll work.
In my case I have to go with second solution because my queries are all in .sql files

Here are two more cases where the replacement is not detected, with PostgreSQL.

1. JSONB indexing

select ('{"abc": 1}'::jsonb)->>:key;

2. Inside a do block. I assume that $$ triggers the "inside of strings" limitation.

do $$
  begin
    if :value then
     refresh materialized view abc;
   end if;
  end
$$;
select 1;

We worked around these cases by concatenating strings, and taking care of sanitization ourselves.

Thank you! we'll look into fixing these two :)

According to postgres' documentation, DO expects a string, and bind parameters are supported in it. Going to need to add a good number of tests to ensure we properly parse strings within strings

@ephys I'm clearing up my notifications and trying to ping you in some of those so they're back on your radar

@Caerbannog I haven't been able to resolve the DO and EXECUTE use cases.

The problem is that I cannot guarantee that the code injected inside of the string is safe. It requires a much more complex escape function.

In your example, if I set value to '$$', the result will be

do $$
  begin
    if '$$' then
      refresh materialized view abc;
    end if;
  end
$$;

Which is a syntax error because postgres considers $$ to be the end of the string passed to the do block.

I don't see a way to escape $$, so this may simply be impossible

This limitation wasn't introduced by this PR, so that code was vulnerable before this PR, but I can't enable it until we've figured out a way to safely escape the value that needs to be injected inside of the string.

Hi @ephys

With earlier sequelize versions, passing undefined as positional parameter was consider as null but now an error is been thrown resulting in a breaking change.

Hi @ephys

With earlier sequelize versions, passing undefined as positional parameter was consider as null but now an error is been thrown resulting in a breaking change.

This is also how it broke us. It is often not uncommon in javascript to pass undefined in as positional arguments. It's worse that this slipped in as not a major breaking change - this is a pretty major, breaking API change.