{{ message }}
Fix PostgreSQL CHECK constraint reflection stripping unrelated parens#13303
Open
LeSingh1 wants to merge 2 commits into
Open
Fix PostgreSQL CHECK constraint reflection stripping unrelated parens#13303LeSingh1 wants to merge 2 commits into
LeSingh1 wants to merge 2 commits into
Conversation
The second pass over the CHECK constraint body used a naive
re.sub(r'^[\s\n]*\((.+)\)[\s\n]*$', ...) which, for an expression
like '(x IS NULL OR y IS NULL) AND (x IS NULL OR y IS NULL)', greedily
paired the leading '(' with the trailing ')'. The substitution stripped
both and produced syntactically invalid SQL:
x IS NULL OR y IS NULL) AND (x IS NULL OR y IS NULL
Replace it with a paren-counting helper that only strips when the
position-zero '(' actually closes at the position-last ')', and that
ignores parens inside single- or double-quoted literals (handling
PostgreSQL's '' / "" doubled-quote escapes).
Fixes sqlalchemy#13157
CaselIT
requested changes
May 19, 2026
CaselIT
left a comment
Member
There was a problem hiding this comment.
Hi,
If you want to work on this please try at least to address the comment in the last attempt: #13200 (review)
thanks
…re it Per @CaselIT's review on PR sqlalchemy#13200: - New util.find_matching_paren(text, start) walks a string finding the ')' that matches the '(' at start. Skips single- and double-quoted string literals, and treats '' / "" as doubled-quote escapes inside their respective contexts (matching PG / SQLite conventions). Returns -1 when the opening paren is never closed. - New util.strip_outer_parens(text) builds on find_matching_paren to remove one layer of outer parens iff they wrap the whole stripped expression. This is the postgresql/base.py call site. - sqlite/base.py's existing inline paren-counter (in get_check_constraints) is refactored to call find_matching_paren instead of its own loop. The sqlite version did the same thing minus the doubled-quote-escape handling, which is a strict improvement. Both helpers covered by unit tests under test/base/test_utils.py (FindMatchingParenTest + StripOuterParensTest, 21 cases). The postgresql RegexTest unit tests now import from sqlalchemy.util. The existing sqlite check-constraint reflection tests (11) still pass with the refactor.
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Fixes #13157.
The second pass over the CHECK constraint body in
get_multi_check_constraintsused a naivewhich greedily paired the leading
(with the trailing). For an expression likethe substitution dropped both and produced syntactically invalid SQL:
Replaces the regex with a small
_strip_outer_parenshelper that only strips when the position-zero(actually closes at the position-last), walking the string and ignoring parens inside single- and double-quoted literals (handling PostgreSQL's''and""doubled-quote escapes). This matches the paren-counting approach already used by the sqlite dialect's CHECK reflection.Tests in
test/dialect/postgresql/test_reflection.py:RegexTest.test_strip_outer_parens— 10 parametrized cases covering the regression, the existing strip-once cases enumerated in the source comments, no-op cases, literals containing parens, doubled-quote escapes, and the newline cases fromtest_reflect_extra_newlines.ReflectionTest.test_reflect_check_constraint_unwrapped_parens— mock-based end-to-end exercisingget_check_constraintswith the issue repro plus a literal-containing-parens case.pytest test/dialect/postgresql/test_reflection.py::RegexTest: 40 passed (33 existing + 7 new strip-paren cases).I did not extend this PR to the broader "we should have a dedicated SQL expression parser" direction discussed in the thread (zzzeek's note). Happy to do that as a follow-up if useful.