Fix PostgreSQL CHECK constraint reflection stripping unrelated parens by LeSingh1 · Pull Request #13303 · sqlalchemy/sqlalchemy · GitHub
Skip to content

Fix PostgreSQL CHECK constraint reflection stripping unrelated parens#13303

Open
LeSingh1 wants to merge 2 commits into
sqlalchemy:mainfrom
LeSingh1:fix-pg-check-paren-stripping-13157
Open

Fix PostgreSQL CHECK constraint reflection stripping unrelated parens#13303
LeSingh1 wants to merge 2 commits into
sqlalchemy:mainfrom
LeSingh1:fix-pg-check-paren-stripping-13157

Conversation

@LeSingh1

Copy link
Copy Markdown

Fixes #13157.

The second pass over the CHECK constraint body in get_multi_check_constraints used a naive

re.sub(r"^[\s\n]*\((.+)\)[\s\n]*$", r"\1", m.group(1))

which greedily paired the leading ( with the trailing ). For an expression like

(x IS NULL OR y IS NULL) AND (x IS NULL OR y IS NULL)

the substitution dropped both and produced syntactically invalid SQL:

x IS NULL OR y IS NULL) AND (x IS NULL OR y IS NULL

Replaces the regex with a small _strip_outer_parens helper 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 from test_reflect_extra_newlines.
  • ReflectionTest.test_reflect_check_constraint_unwrapped_parens — mock-based end-to-end exercising get_check_constraints with 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.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.
@LeSingh1

Copy link
Copy Markdown
Author

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.

PostgreSQL check constraint reflection strips brackets naively

2 participants