Mysql: Client.prototype.format shouldn't treat "?" as placeholder inside string literals

[TEHEK]

Description

Client.prototype.format attempts to replace every occurrence of ? for a variable. Even if it's inside a string literal.

E.g.:

 mysql.query("SELECT * FROM ponies WHERE emotion = 'wtf?' AND id = ?", [1]) 

results in error because function expects two parameters instead of one.

Solutions

Option 1

Pretend it's a feature :) And do adhoc stuff like this:

 mysql.query("SELECT * FROM ponies WHERE emotion = 'wtf?' AND id = ?", ['?', 1]) 

Option 2

Placeholders should be left intact if in string literals. Such behavior can be observed in PHP PDO

mysql.query("SELECT * FROM ponies WHERE emotion = 'wtf?' AND id = ?", [1]) 
    // SELECT * FROM ponies WHERE emotion = 'wtf?' AND id = 1

Option 3

Add escaping convention for ?.

For example, use # or \ and do something like:

?      => value
#?     => ?
##     => #
###?   => #?

IMHO, we should stay away from \ as it can easily turn into escaping hell like in these non-real-world examples:

// SELECT * FROM ponies WHERE emotion = 'wtf?' AND id = ?
mysql.query('SELECT * FROM ponies WHERE emotion = \'wtf\\?\' AND id = ?', [1]) 

// SELECT * FROM ninjas WHERE slash_and_question = '\\?' AND id = ?'
// has to be: SELECT * FROM ninjas WHERE slash_and_question = '\\\\\?' AND id = ?'
mysql.query('SELECT * FROM ninjas WHERE slash_and_question = \'\\\\\\\\\\?\' AND id = ?', [1]) 

I have patch for options 1 and 2. Option 3 is a piece of cake. Which one should I attach? :)

[tpetry]

The best option would be replacing all question marks with their values not enclosed in backticks, single and double quotes. So you can be sure you won't replace any values or some weird columns, tables with a question mark.
This would be a much simpler way than writing a sql parser.

[TEHEK]

Good point. You know, after reading more details on the mysql docs (http://dev.mysql.com/doc/refman/5.0/en/string-syntax.html), I think it's getting too complicated and Option 3 (DIY) starts looking much more attractive

[TEHEK]

Misclick -_-

So, which escape character should we go with?

[user24]

Fair point about the more complicated cases making option 2 less attractive, though I do think that's the ideal solution. I do think that is a universally understood escape character and therefore the least surprising way of doing escaping.

[tpetry]

Question-Marks which should be replaced are more common than question mars which should not be replace. And the single quote as placeholder is a standard in mysql, breaking this would make node-mysql very complicated for people working with mysql prepared statements in any other language

[user24]

I deleted my previous comment as it was nonsense, sorry. Failed to account for all kinds of situations.

[felixge]

Option 1
Pretend it's a feature :) And do adhoc stuff like this:

There is no need to pretend, the current implementation is simple and unambiguous, I see nothing wrong with it. If you need a question mark inside a query, make it a parameter.

--fg

[tpetry]

The problem is, that it's not conform to the MySQL Standard.
How the implementation should behave:

mysql> PREPARE stmt1 FROM 'SELECT "no placeholder: ?", ?';
Query OK, 0 rows affected (0.00 sec)
Statement prepared

mysql> SET @a = 3;
Query OK, 0 rows affected (0.00 sec)

mysql> EXECUTE stmt1 USING @a;
+-------------------+---+
| no placeholder: ? | ? |
+-------------------+---+
| no placeholder: ? | 3 |
+-------------------+---+
1 row in set (0.00 sec)

When using 2 parameters (should be wrong):

mysql> SET @b = 4;
Query OK, 0 rows affected (0.00 sec)

mysql> EXECUTE stmt1 USING @a, @b;
ERROR 1210 (HY000): Incorrect arguments to EXECUTE

I know that the implementation of node-mysql does not use prepared statements but using the question mark for placeholders is in every other implementation in every language an emulation of prepared statements, and node-mysql does break this standard.

If we'll develop a standards compliant escaping function, would you merge it into node-mysql or will you insist on your opinion that the actual implementation should be like it is?

[felixge]

I know that the implementation of node-mysql does not use prepared statements but using the question mark for placeholders is in every other implementation in every language an emulation of prepared statements, and node-mysql does break this standard.

When you say standard, do you mean convention or standard? Afaik there is no standard concerning the pre-processing of SQL statements by client libraries, but I could be wrong.

That being said, I'm not at all opposed to making my driver easier to use for people used to other implementations, but I'd like to clarify if that's what we're discussing here or if there is indeed a standard on how this is supposed to work.

[user24]

I think @tpetry means that if node-mysql is trying to emulate the prepared statement standard, then it fails on this point.

[tpetry]

@user24: Correct! The question mark placeholder "convention" is more an emulation of prepared statements than an invented system for replacing values inside of a SQL string. In my opinion this emulation should be consistent to the feature it is derived from: Prepared Statements.

[felixge]

Ok, I guess with that discussion cleared up, I would certainly consider merging a patch that adds support for this. However, from looking at the docs for prepared statements, it seems like you might need to implement a sql parser for this. If you do, please put it in a separate file in order to not bloat the client.js code.

--fg

[tpetry]

I have actually a more simple approach: Iterate over the string and change the "parse" mode:
When you see double quotes ignore any char that's not a double quote. \" and "" should be treated as non double quotes. When you find the enclosing double quote you know that you are not inside of an string and every question mark will be replaced by the escaped value. The same has to be done for single quotes. I yet have to research the rules for the backticks. The patch will need some days, i am actually quite busy, will look to do it on the weekend.

@felixge: Could we reopen the issue? A patch is at the moment missing so it's an actual bug. I know that you see it differently.

[TEHEK]

Well, here you go folks!

I refactored the regexp and added a very simplified fsm-like parser. Some test cases added, but if someone would like to add more, you are welcome!

The point is not to make it 100% compliant with prepared statements but to make something that would balance convenience (no need to add adhoc '?' parameters) and simplicity/predictability.

IMO, if prepareds are to be implemented, they have to be in a separate function/api something like

var st = mysql.prepare("SELECT * FROM users WHERE id = ?");
st.exec([1], function (err, data) {})
 .exec([2], function (err, data) {})

[Leprosy]

Is this propagated in 2.0.0-alpha?

[felixge]

@Leprosy what do you mean by propagated?

[Leprosy]

I mean... I'm working with 2.0.0-alpha and we are having this exact issue, maybe the patch isn't applied in that version?

[TEHEK]

Unfortunately, patch for option 2 was not accepted -_-

But feel free to merge from my branch ^_^. Aside from unit tests, it's just a paragraph of code.

[Leprosy]

Thanks!!

[qwy]

is it still remain open bug?
kinda incredible if it is after 5 year.
i'm using "mysql": "^2.12.0", it's still making trouble there. and yet i cannot find an elegant way out.
either option 2 or 3 makes sense to me.

please do fix it.
or let me know if my info is incorrect.
thanks.

[felixge]

@qwy https://github.com/mysqljs/mysql/blob/master/License#L13-L19