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.

DiegoDAF added a commit to DiegoDAF/pgcli.daf that referenced this pull request Aug 18, 2026
@DiegoDAF

Copy link
Copy Markdown
Contributor

Nice catch, and thank you for fixing it!

I can confirm the bug and the fix from my fork. Against current main, with
destructive_warning = drop, unconditional_update:

update t set a = 'x where y', b = 2   ->  no warning   (should warn)

The bare where inside the string literal makes the whitespace split believe
there is a WHERE clause, so an unconditional UPDATE runs with no confirmation.
That is the bad direction for a safety check to fail in.

I cherry-picked your commit and re-ran the same cases:

statement warns
update t set x = 1 yes
update t set x = 1 where id = 2 no
update t set a = 'x where y', b = 2 yes (was the bug)
update t set x = 1 -- where yes
update t set x = (select 1 from y where z = 1) yes
select * from t no

The subquery case is a nice bonus of using the parse tree: a WHERE that only
exists inside a subquery is no longer mistaken for a top-level one, so the
statement is still flagged as unconditional.

Full suite green on my side too. Hope this helps it along.

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