False positive with `ON DUPLICATE` in `executemany`

[asottile]

This allows any text after ON DUPLICATE: https://github.com/PyMySQL/mysqlclient-python/blob/f96cb44ef18007a33d1400ff98ce9b887cd983d3/MySQLdb/cursors.py#L31

Here you claim it is not broken: #163 (comment)

[methane]

I think "ON DUPLICATE KEY" support is more important than comment, because removing it
efficiently is not easy (or impossible) than removing comment.

Any example query which .execute() works but .executemany() don't work?
If there is realistic example, I'll remove ON DUPLICATE KEY clause from the pattern.

[asottile]

So this is an acceptable false positive (where my comment PRs did not have any false positives)?

'INSERT INTO foo ... ON DUPLICATE KEY UPDATE x=x+1; DROP TABLE bar;'

[methane]

Here you claim it is not broken: #163 (comment)

It's a very sarcastic comment.
I meant "not supporting comment" is not "broken", off course.

[asottile]

Not intending to be sarcastic, this is what I was referring to:

The pattern is fundamentally incorrect, and can be shown to be broken with trivial examples.

NO! It's not broken. It just doesn't support all insert SQL.
It only support very limited pattern by design, internationally.

[methane]

'INSERT INTO foo ... ON DUPLICATE KEY UPDATE x=x+1; DROP TABLE bar;'

OK, how about ON\s+DUPLICATE\s+KEY\s+UPDATE\s+[^;/]+?

[methane]

'INSERT INTO foo ... ON DUPLICATE KEY UPDATE x=x+1; DROP TABLE bar;'

BTW, .execute() also run "; DROP TABLE bar" part.
So it's not example ".execute() works but .executemany() don't work"

[asottile]

Sounds to me like "There are no chance to accept pull request making the pattern complicated." so your suggested regex won't be accepted :P

[methane]

I meant about pull requests about accepting more query.
Pull request about make it robust, less false positive is welcome.

[asottile]

Even if it is more complicated?

So you won't accept a simple pull request, but you'll accept one which adds complexity? I'm confused by your goals here other than frustrate your users and encourage a fork.

[methane]

Even if it is more complicated?

Yes. If it makes less false positive.
And ON DUPLICATE KEY is more acceptable than comment because:

• It only happens on the last of the query. (comment may be anywhere).
• Sometime it's important for performance and efficiency. Removing it from query without performance regression is impossible sometimes.

I'm confused by your goals

My goal is make this library simple, robust, efficient, performance and maintainable.

[asottile]

But you won't accept a PR which allows comments only at the end and has zero false positives and has tests and is easy to understand? I'm baffled! This is equally for performance reasons! I don't want my query which ran as a bulk insert in a patch version earlier to suddenly be hammering our db hosts with billions of queries.

[methane]

Your PR accepts comment only at the last even comment can be happen everywhere in valid SQL.
It's different from ON DUPLICATE KEY.

Additionally, if I see PR which accepts ON DUPLICATE KEY for now, I'll reject it.

This is equally for performance reasons!

Because removing ON DUPLICATE KEY from query without performance regression
is more difficult than removing comment from query.

After implementing fallback using multi statement, I'll consider removing ON DUPLICATE from the pattern.

[asottile]

Right, mine is much simpler than accepting comments anywhere -- I only care about at the end (and most tagging tools which append such comments to queries for tracking add them at the end).

[methane]

#163 (comment)

If I accept this, why not heading comment? comment between INSERT and opening (?
comments between values?

Please stop talking about adding comment support on this issue. It's made decision.

[asottile]

There's a legitimate usecase for comments at end, there are not demonstrated legitimate usecases for comments elswhere.