Skip to content

Add WHERE detection for UPDATE and DELETE - #101

Open
eliasthecactus wants to merge 2 commits into
coresql:mainfrom
eliasthecactus:where-detection
Open

eliasthecactus wants to merge 2 commits into
coresql:mainfrom
eliasthecactus:where-detection

Conversation

@eliasthecactus

Copy link
Copy Markdown

Adds a where flag to UPDATE and DELETE results, set when the statement has a WHERE of its own. It works the same way as the limit/offset flags from #98.

identify('DELETE FROM users');                 // [{ type: 'DELETE', where: false, ... }]
identify('DELETE FROM users WHERE id = 1');    // [{ type: 'DELETE', where: true, ... }]

This is for beekeeper-studio/beekeeper-studio#4866, which warns before running an UPDATE or DELETE that changes every row. In review, @rathboma suggested the detection belongs here rather than in Beekeeper.

What counts

  • Only a WHERE at the statement's own level. Parentheses are now counted for UPDATE and DELETE as they already are for SELECT, so a WHERE in a subquery (in SET, in a Postgres UPDATE ... FROM (...)) is ignored. A CTE's WHERE never reaches the statement parser.
  • A WHERE that holds a subquery still counts (WHERE id IN (SELECT ...)).
  • Strings, comments and quoted identifiers are skipped by the tokenizer, so SET note = 'where' or "where" don't count.
  • LIMIT (MySQL) and TOP (MSSQL) don't count: they cap the rows but still hit arbitrary ones.
  • Other statement types don't get the flag.

Changes

  • WHERE is added to the tokenizer's keywords, as FROM and LIMIT were in Add limit and offset detection #98. The only visible effect is that it becomes its own keyword token in parse() output. I updated the existing parser specs that print token lists. I also compared identify() output with 3.2.0 for some SELECT and INSERT queries with WHERE clauses, tables and columns, and it's identical apart from the new flag.
  • New specs under "WHERE detection" cover the cases above, plus MySQL multi-table DELETE, Postgres DELETE ... USING and multiple statements.
  • The README gets a short "Statement flags" section covering limit, offset and where, since none of them were documented yet.

npm test (842 passing), npm run lint and npm run build pass locally on Node 24.

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 rathboma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@eliasthecactus

Copy link
Copy Markdown
Author

Thanks for the review, all good points.

  1. Strings now handle \ escapes for mysql (both ' and ") and for E'...' in psql. Plain psql strings are unchanged, so 'C:\' still ends where it should. Nested block comments work for psql now, and for mssql as well since it allows them too.
  2. Left it out of scope, the README now says an UPDATE/DELETE inside a CTE isn't checked.
  3. A keyword right after . or @ isn't treated as a keyword anymore, so t.where, @where and t.limit are fixed. Spaces around the dot work too, just not across a newline, otherwise a -- comment ending with a dot. would swallow the next statement.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants