Skip to content

Detect an unconditional UPDATE with sqlparse instead of splitting on whitespace - #1614

Open
VXNCXNX wants to merge 2 commits into
dbcli:mainfrom
VXNCXNX:fix/unconditional-update-string-literal
Open

Detect an unconditional UPDATE with sqlparse instead of splitting on whitespace#1614
VXNCXNX wants to merge 2 commits into
dbcli:mainfrom
VXNCXNX:fix/unconditional-update-string-literal

Conversation

@VXNCXNX

@VXNCXNX VXNCXNX commented Aug 15, 2026

Copy link
Copy Markdown

Description

With destructive_warning covering unconditional_update, an UPDATE with no WHERE clause skips the confirmation prompt if any string literal in it contains the standalone word "where".

Repro

Run on this checkout, not reasoned about:

$ python -c "
from pgcli.packages.parseutils import is_destructive
K = ['unconditional_update']
print(is_destructive(\"update accounts set balance = 0\", K))
print(is_destructive(\"update accounts set note = 'no where clause here'\", K))"
True
False

The second one is an unconditional UPDATE. It returns False, so the prompt never appears.

The fix

query_is_unconditional_update did formatted_sql.split() and checked "where" not in tokens. Splitting on whitespace turns the contents of a string literal into tokens, so 'no where clause here' yields a bare where.

It now uses sqlparse's parse tree, which the module already imports: confirm the first meaningful token is the DML keyword UPDATE, then look for a sqlparse.sql.Where among the statement's top-level tokens. Since sqlparse handles case and comments itself, the call site passes the raw query rather than the lowercased comment-stripped one. query_starts_with is untouched, it serves the separate keyword-prefix path and is not affected.

A WHERE inside a subquery is nested under the Parenthesis, not a top-level token, so update t set c = (select x from y where z = 1) correctly still warns.

Verification

Checked against these, all as expected:

update t set c = 0                                  True
update t set n = 'no where clause here'             True
update t set c = 'nowhere'                          True
UPDATE t SET c = 1                                  True
/* where */ update t set c = 1                      True
update t set c = 1 -- where id = 1                  True
update t set c = (select x from y where z = 1)      True
update t set c = 0 where id = 1                     False
UPDATE  t  SET  c = 1  WhErE  id = 1                False
select * from t                                     False

Added a parametrized case list to tests/parseutils/test_parseutils.py next to the existing test_is_destructive. It fails against the old implementation on the string-literal and subquery cases. pytest tests/parseutils/ passes, 115 tests.

One thing I did not change: with x as (...) update t set c = 1 returns False, because the first token is the CTE rather than the DML keyword. That is the same as before this patch, so it is a separate gap rather than a regression, and I left it alone.

Checklist

  • I've added this contribution to the changelog.rst.
  • I've added my name to the AUTHORS file (or it's already there).
  • I installed pre-commit hooks (pip install pre-commit && pre-commit install).
  • I verified that my changes work as expected (this may include manually testing them in your local environment, or in other available environments). Cross this out if not relevant (for example, if you're making a documentation change).
  • Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

Apologies for not using the template on the original description; I have restructured it above and added the changelog and AUTHORS entries that were genuinely missing. Disclosure: written with AI assistance (Claude Code); I ran the reproduction and the tests myself.

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.

1 participant