Repository navigation
Add WHERE detection for UPDATE and DELETE - #101
eliasthecactus wants to merge 2 commits into
Conversation
UPDATE and DELETE results get a where flag, set when the statement has a WHERE of its own. A WHERE in a subquery or a CTE does not count, nor does one in a string, a comment or a quoted identifier, and LIMIT or TOP is not a WHERE. This lets a client warn before an UPDATE or DELETE that changes every row.
rathboma
left a comment
There was a problem hiding this comment.
Thanks for this, it looks good overall. I found a few bugs, ranked by how much they matter for the Beekeeper warning:
1. Backslash-escaped quotes cause a false where: true
mysql: UPDATE posts SET body = 'It\'s where we live' → where: true
psql: UPDATE posts SET body = E'It\'s where we live' → where: true
scanString only handles doubled quotes (''), so the string ends at \' and the where inside it is read as a keyword. The tokenizer bug isn't new, but it now affects a safety check: an unfiltered UPDATE is reported as filtered, so the warning won't show. MySQL has backslash escapes on by default, so this isn't just theoretical.
Can we handle \ escapes in scanString for mysql, and for E'...' in psql? A spec for each would be good too. Postgres nested block comments (/* a /* b */ where */) hit the same thing, but that's much rarer.
2. DELETEs and UPDATEs inside a CTE are never checked
WITH d AS (DELETE FROM a RETURNING *) SELECT * FROM d → SELECT, no flag
WITH d AS (DELETE FROM a) DELETE FROM b WHERE id = 1 → where: true (a is wiped)
This is probably fine to leave out of scope here, but the README should say so, since "a CTE does not count" reads as if it's handled.
3. where after . or @ counts as a WHERE
psql/mysql: UPDATE t SET x = t.where → where: true
mssql: UPDATE t SET x = @where → where: true
A column or variable called where is legal in these forms. It's rare, but it's the same risky direction as #1. Ignoring the keyword when the previous token is . or @ would fix it. t.limit from #98 has the same problem.
README wording: "where: false means the statement is not filtered by a condition" isn't quite right. DELETE u FROM users u JOIN bans b ON ... is filtered by its join but gives where: false, and WHERE 1=1 gives where: true. Maybe: "whether the statement has its own WHERE clause; the condition itself isn't checked."
Also - looks like this PR was written by an AI agent of some sort -- def open a new agent session when you're done with dev and ask it to scan for bugs or logic issues. A bunch of stuff like this should be fairly easy for an agent to catch.
- mysql, bigquery, snowflake: handle backslash escapes in strings - psql: handle backslash escapes in E'...' strings - oracle: handle q'[...]' and nq'[...]' strings - psql, mssql: handle nested block comments - don't treat a word after . or @ as a keyword (t.where, @where, t.limit) - set where=false when another statement follows without a semicolon (mssql) - README: clarify what the where flag means and that CTEs aren't checked
|
Thanks for the review, all good points.
Took your README wording as well. And fair point about the review pass. Before committing this I had Opus 5.5 scan the branch for bugs, it found a few more of the same kind, so those are fixed in here too. Specs for all of it, tests/lint/build pass. |
Adds a
whereflag toUPDATEandDELETEresults, set when the statement has aWHEREof its own. It works the same way as thelimit/offsetflags from #98.This is for beekeeper-studio/beekeeper-studio#4866, which warns before running an
UPDATEorDELETEthat changes every row. In review, @rathboma suggested the detection belongs here rather than in Beekeeper.What counts
WHEREat the statement's own level. Parentheses are now counted forUPDATEandDELETEas they already are forSELECT, so aWHEREin a subquery (inSET, in a PostgresUPDATE ... FROM (...)) is ignored. A CTE'sWHEREnever reaches the statement parser.WHEREthat holds a subquery still counts (WHERE id IN (SELECT ...)).SET note = 'where'or"where"don't count.LIMIT(MySQL) andTOP(MSSQL) don't count: they cap the rows but still hit arbitrary ones.Changes
WHEREis added to the tokenizer's keywords, asFROMandLIMITwere in Add limit and offset detection #98. The only visible effect is that it becomes its ownkeywordtoken inparse()output. I updated the existing parser specs that print token lists. I also comparedidentify()output with 3.2.0 for some SELECT and INSERT queries withWHEREclauses, tables and columns, and it's identical apart from the new flag.DELETE, PostgresDELETE ... USINGand multiple statements.limit,offsetandwhere, since none of them were documented yet.npm test(842 passing),npm run lintandnpm run buildpass locally on Node 24.