Fix dropped parentheses on column-level CHECK constraints

[gmr]

Summary

Fixes gmr/pgfmt#11. A column-level CHECK constraint requires its expression to be parenthesized. format_col_constraint_elem emitted it unparenthesized, so CHECK (a > 0) formatted to CHECK a > 0 — invalid PostgreSQL. Re-running the formatter on that output mis-parsed the broken SQL and cascaded into a corrupted table (columns split apart).

Problem

-- input
CREATE TABLE t (a int CHECK (a > 0));
-- before: invalid SQL, breaks on the next pass
CREATE TABLE t (a INTEGER CHECK a > 0);

Solution

• Wrap the column-level CHECK expression in parentheses, gated on has_check so DEFAULT and other column expressions are left alone.
• Add is_wrapped_in_parens so an already-parenthesized expression (e.g. pg_dump's redundant CHECK ((x))) collapses to a single pair rather than doubling up. This keeps the create_domain fixture stable.
• Stop padding the type column of a trailing constraint-less column, which left trailing whitespace on the line. Extract a shared render_aligned_column helper used by both the CREATE TABLE and CREATE FOREIGN TABLE river renderers.
• Add a regression test built from the issue's exact example covering paren preservation, idempotency, and no trailing whitespace.

Testing

• cargo test — full suite passes (including the new regression test and all fixtures).
• cargo clippy --all-targets -- -D warnings — clean.
• Verified the issue's example is stable across repeated formatting passes.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved formatting for CREATE TABLE and CREATE FOREIGN TABLE column definitions, with consistent alignment of column names and types.
  • Fixed column-level CHECK formatting to preserve required parentheses and avoid incorrect wrapping.
  • Removed trailing whitespace from formatted SQL output.
• Tests
  • Added regression tests to confirm CHECK parentheses are preserved and formatting is idempotent across repeated runs, covering both table and foreign table cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 454ea52e-31d0-44a8-9e64-dbb91da04f03

📥 Commits

Reviewing files that changed from the base of the PR and between 042e7cc and b0e4875.

📒 Files selected for processing (1)

• tests/test_parens.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/test_parens.rs

---

📝 Walkthrough

Walkthrough

This PR preserves column-level CHECK parentheses during repeated formatting, adds shared river-style column alignment, and extends regression coverage for CREATE TABLE and CREATE FOREIGN TABLE output.

Changes

CHECK constraint parentheses and column alignment fix

Layer / File(s)	Summary
New helper functions for alignment and paren detection src/formatter/stmt.rs	Adds render_aligned_column for column name/type padding and is_wrapped_in_parens to detect a single outer parenthesized expression while ignoring parentheses inside string literals.
CHECK constraint parenthesization logic src/formatter/stmt.rs	format_col_constraint_elem tracks kw_check and conditionally wraps column CHECK expressions in parentheses unless they are already singly wrapped.
Apply aligned column rendering and regression tests src/formatter/stmt.rs, tests/test_parens.rs	River-style CREATE TABLE and CREATE FOREIGN TABLE rendering now use render_aligned_column, and tests verify CHECK parentheses, idempotence, and no trailing whitespace.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• gmr/libpgfmt#4: It changes the same src/formatter/stmt.rs formatting paths for CREATE TABLE / CREATE FOREIGN TABLE and CHECK constraint handling.

Poem

I’m a rabbit with tidy paws,
I hop through CHECKs and column clauses.
Parens stay put, aligned just right,
and formatting repeats all night. 🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title matches the main fix: preserving CHECK parentheses during formatting.
Linked Issues check	✅ Passed	The change addresses issue #11 by preserving column-level CHECK parentheses and keeping repeated formatting idempotent.
Out of Scope Changes check	✅ Passed	The added foreign-table coverage and shared test helper are directly tied to the same formatting fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_parens.rs (1)

29-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding a CREATE FOREIGN TABLE case.

Per the cohort description, render_aligned_column is also applied to CREATE FOREIGN TABLE rendering, but this regression test only exercises CREATE TABLE. A parallel case would close the gap on that code path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_parens.rs` around lines 29 - 61, Add a regression case for CREATE
FOREIGN TABLE alongside preserve_col_check_parens_and_idempotent, since
render_aligned_column is used there too and the current test only covers CREATE
TABLE. Extend the test in tests/test_parens.rs (or add a sibling test) to format
a CREATE FOREIGN TABLE statement with a column-level CHECK and verify the
parentheses are preserved, reformatting stays idempotent, and no trailing
whitespace is introduced, using format(...) with Style::Aweber as in the
existing test.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_parens.rs`:
- Around line 29-61: Add a regression case for CREATE FOREIGN TABLE alongside
preserve_col_check_parens_and_idempotent, since render_aligned_column is used
there too and the current test only covers CREATE TABLE. Extend the test in
tests/test_parens.rs (or add a sibling test) to format a CREATE FOREIGN TABLE
statement with a column-level CHECK and verify the parentheses are preserved,
reformatting stays idempotent, and no trailing whitespace is introduced, using
format(...) with Style::Aweber as in the existing test.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 43c6d664-6d90-4f6f-940b-930d35f734f3

📥 Commits

Reviewing files that changed from the base of the PR and between 0ee0ed8 and d715b20.

📒 Files selected for processing (2)

• src/formatter/stmt.rs
• tests/test_parens.rs

[gmr]

🤖 This comment was posted by Claude on behalf of @gmr

Thanks @coderabbitai — good catch. Addressed the nitpick in 042e7cc by adding preserve_foreign_col_check_parens_and_idempotent, a parallel regression test that exercises the render_aligned_column path via CREATE FOREIGN TABLE. It asserts the column-level CHECK parentheses are preserved, reformatting stays idempotent, and no trailing whitespace is emitted. Full test suite, clippy (-D warnings), and rustfmt all pass locally.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cd /tmp && rm -rf repo && git clone --quiet https://github.com/gmr/libpgfmt.git repo && cd repo && git log --oneline -5 && git show 042e7cc --stat

Length of output: 1301

---

Verified — commit 042e7cc adds preserve_foreign_col_check_parens_and_idempotent to tests/test_parens.rs, covering the CREATE FOREIGN TABLE path through render_aligned_column with the same assertions (parentheses preserved, idempotency, no trailing whitespace).

@gmr Thanks for the quick follow-up! This closes the gap nicely — both CREATE TABLE and CREATE FOREIGN TABLE paths through render_aligned_column are now covered by regression tests. Nothing further from me on this thread.

(っ◕‿◕)っ

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_parens.rs (1)

62-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

LGTM overall — solid regression coverage for the foreign-table path.

Correctly mirrors the CREATE TABLE test, verifying paren preservation, idempotence, and no trailing whitespace against render_aligned_column/is_wrapped_in_parens.

One minor nit: this test and preserve_col_check_parens_and_idempotent (lines 29-61) duplicate the same three-part assertion pattern (parens check → idempotence → trailing-whitespace loop). Could extract a small helper (e.g. assert_idempotent_and_clean(sql, expected_substr)) to cut duplication, but not blocking.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_parens.rs` around lines 62 - 91, The new foreign-table regression
test duplicates the same assertion flow already used in
preserve_col_check_parens_and_idempotent, so factor the repeated
parens/idempotence/trailing-whitespace checks into a small shared helper in
tests/test_parens.rs. Update preserve_foreign_col_check_parens_and_idempotent to
call that helper with the SQL string and expected CHECK substring, keeping the
behavior identical while reducing duplication.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_parens.rs`:
- Around line 62-91: The new foreign-table regression test duplicates the same
assertion flow already used in preserve_col_check_parens_and_idempotent, so
factor the repeated parens/idempotence/trailing-whitespace checks into a small
shared helper in tests/test_parens.rs. Update
preserve_foreign_col_check_parens_and_idempotent to call that helper with the
SQL string and expected CHECK substring, keeping the behavior identical while
reducing duplication.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 88b654e6-d4c1-49de-a6bb-d60a9673552f

📥 Commits

Reviewing files that changed from the base of the PR and between d715b20 and 042e7cc.

📒 Files selected for processing (1)

• tests/test_parens.rs

[gmr]

🤖 This comment was posted by Claude on behalf of @gmr

Thanks @coderabbitai — addressed the duplication nitpick in b0e4875 by extracting the shared assert_idempotent_and_clean(sql, expected_substr) helper, which both preserve_col_check_parens_and_idempotent and preserve_foreign_col_check_parens_and_idempotent now call. Behavior is identical. Tests, clippy (-D warnings), and rustfmt all pass locally.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cd /tmp && rm -rf repo && git clone --quiet https://github.com/gmr/libpgfmt.git repo && cd repo && git show b0e4875 --stat && echo "---" && git show b0e4875 -- tests/test_parens.rs | head -150

Length of output: 5096

---

@gmr Verified — commit b0e4875 cleanly extracts assert_idempotent_and_clean(sql, expected_substr) and both regression tests now delegate to it with identical assertions (substring check, idempotency, no trailing whitespace). Behavior is preserved. Thanks for the quick turnaround on this thread!

(っ◕‿◕)っ