"Error: Incorrect arguments to mysqld_stmt_execute" when a comment includes a parameter

[ferrybig]

Description

When any SQL query includes a command containing the syntax for a parameter and is passed to the execute function, the library gives an SQL error. I would expect the SQL parser in the library to properly identify that it is inside an command expression and handle the situation gracefully.

Reproducing:

Run the following code:

import mysql from 'mysql2';
const pool = mysql.createPool(....);
const db = await pool.promise();
const sql = `
     SELECT
        -- The following code return the value of the :input parameter as a single row
        :input as input
     FROM
           DUAL
 `;
const params = {
    input: 1234,
};
const [resultSet] = await db.execute(sql, params);

Expected

I get a result set back which contains [ { input: 1234 } ]

Actual:

I receive the following error

Error: Incorrect arguments to mysqld_stmt_execute
at PromisePoolConnection.execute (<hidden>/node_modules/mysql2/promise.js:110:22) {
  code: 'ER_WRONG_ARGUMENTS',
  errno: 1210,
  sqlState: 'HY000',
  sqlMessage: 'Incorrect arguments to mysqld_stmt_execute'
}

Use case

In my use case, I get complex queries from third parties, which sometimes includes comments using --, or /* */ or //. The MySQL2 library just refuses to run the queries with comments containing parameter strings with the above error, needing me to use workarounds

Current workaround

For every SQL query that I get from my third party, I have to inspect every comment and add a space after : if text follows this symbol. Missing any means I get a production crash

[sidorares]

Hi @ferrybig this is known problem, solution is worked on in #1407

pre 8.0.22 mysql server does not show this problem and happy to accept input parameters sent as a type different from what prepared statement expects

some more context here - #1239 (comment)

[sidorares]

some difficulty in solving this is that we can't completely rely on the type hints for parameters ( we can trust the results types though ). Sometimes mysql just does not know what type is expected, example: SELECT ? as test. Current code infers the type from JS type of the parameter ( on JS side ) - number / string / Date / Buffer. The correct way to do that is sometimes using PS type hints, but only when that makes sense. But maybe even better is to allow to completely give control to a user, for example with helper PreparedStatementParameter(type, value) - it'll capture both type and value and protocol serialiser downstream would use provided type instead of PS type hint or value JS type

[ruiquelhas]

That's a good point, I kind of overlooked statements where there is no underlying explicit column data type. Would the custom hinting mechanism work on a per-statement or a per-connection basis?

[ferrybig]

@sidorares We are running MySQL 5.7.32, so I'm not sure if you picked the correct duplicate target.

Type inference seems to work correctly with the space after the : in the comment, it is only when an executed statement contains an : followed by name that matches a bound parameter name that causes this bug (not sure what happens when the comment contains a name that is not defined)

EDIT:

This is buggy:

const sql = `
     SELECT
        -- The following code return the value of the :input parameter as a single row
        :input as input
     FROM
           DUAL
 `;

This is NOT buggy:

const sql = `
     SELECT
        -- The following code return the value of the : input parameter as a single row
        :input as input
     FROM
           DUAL
 `;

[sidorares]

might be a wrong duplicate indeed

are you using namedPlaceholders option @ferrybig ?

[sidorares]

Would the custom hinting mechanism work on a per-statement or a per-connection basis?

@ruiquelhas I was thinking per-execute if I understand your question correctly connection.execute('select ? as test', [PreparedStatementParameter(VARCHAR, new Date()]) would send date parameter as string

[ferrybig]

@sidorares

Yes, namedPlaceholders: true, is specified when we call mysql.createPool(...)

[sidorares]

Yes, namedPlaceholders: true

oh ok I see . So the issue is that placeholder gets replaced in both comment and query text
This is somewhat expected and not sure If I want to fix it, as proper fix would require full sql parser on the client side ( though maybe just a parser understanding comments is enough, but next step would be string quotation etc etc ).

[sidorares]

const sql = `
     SELECT
        -- The following code return the value of the :input parameter as a single row
        :input as input
     FROM
           DUAL
 `;
conn.query(sql, [{ input: 123 }])

becomes

const sql = `
     SELECT
        -- The following code return the value of the ? parameter as a single row
        ? as input
     FROM
           DUAL
 `;
conn.query(sql, [123, 123])

because of 2 :input, hence ER_WRONG_ARGUMENTS

[sidorares]

I need to double check why this error from the server, I was sure we have check on the client before even sending execute command I expect it to return error if number of parameters does not match response for matching .prepare()

[ferrybig]

though maybe just a parser understanding comments is enough, but next step would be string quotation etc etc

Looking at what you wrote already for a parser, you seems to have though of string quotations, but not of comments. (probably because comments can be parsed with just looking at 1 char, while support for -- needs to 3 chars in order to stat the comment, and don't start about the version specific MySQL comments)

https://github.com/mysqljs/named-placeholders/blob/d5134a305e9cdc6604153aa08275942e15895936/index.js#L26-L56

This also seems related to named-placeholders#9

Does this issue needs to be moved to https://github.com/mysqljs/named-placeholders because the root cause of the problem seems to be in that project?

[flipperlite]

Here's a quick and easy workaround in the meantime. It only filters out comments that start -- with a trailing space. If you need support for // or /* ... */ style comments, you could look into a broader regex and/or parser. If you need it across your code base, make it into a reusable function.

const sqlWithoutComments = sql.split('\n').filter(e => !e.trim().startsWith('-- ')).join('\n');
const [resultSet] = await db.execute(sqlWithoutComments, params);

EDIT:

After reading about the format of mysql comments, my code should be updated where -- and # are treated as inline comments (as opposed to starting the line) and /* ... */ is treated both as inline and multiline comments. The OP even mentioned comments using // inline style comments too. I didn't find any good regex or parser that contains the total of the comment styles but my workaround would alleviate the parameter issue leaving the code in a better forward state if the mysql comment parser was updated in the future. Obviously, I'd write some unit tests around the logic to remove mysql comments.

[sidorares]

Does this issue needs to be moved to https://github.com/mysqljs/named-placeholders because the root cause of the problem seems to be in that project?

@ferrybig yes, probably better to move to https://github.com/mysqljs/named-placeholders

[sidorares]

I'll close this issue for now, the root cause was identified, I'd classify it as not a bug but "there is a room for improvement" and "maybe better documentation would help". Happy to see comments parsing support PR in named-paceholders repo