From ad8872410a2d3bebaa87217a91ca114dc7deaa52 Mon Sep 17 00:00:00 2001 From: Tianzhou Date: Fri, 10 Apr 2026 08:01:53 -0700 Subject: [PATCH] fix: table-level CHECK constraints with IS NOT NULL silently omitted from dump (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inspector was using strings.Contains(checkClause, "IS NOT NULL") to skip system-generated NOT NULL constraints, but this incorrectly filtered out any CHECK constraint whose expression contained IS NOT NULL — including complex user-defined constraints like CHECK (status = 'active' OR reason IS NOT NULL). Narrowed the filter to only skip simple single-identifier NOT NULL checks (e.g., CHECK ((value IS NOT NULL))) by stripping the CHECK wrapper and parentheses and verifying the prefix is a single word with no spaces. Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/dump/dump_integration_test.go | 7 ++++ ir/inspector.go | 15 +++++-- .../manifest.json | 9 +++++ .../pgdump.sql | 40 +++++++++++++++++++ .../pgschema.sql | 21 ++++++++++ .../raw.sql | 19 +++++++++ 6 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 testdata/dump/issue_396_check_constraint_is_not_null/manifest.json create mode 100644 testdata/dump/issue_396_check_constraint_is_not_null/pgdump.sql create mode 100644 testdata/dump/issue_396_check_constraint_is_not_null/pgschema.sql create mode 100644 testdata/dump/issue_396_check_constraint_is_not_null/raw.sql diff --git a/cmd/dump/dump_integration_test.go b/cmd/dump/dump_integration_test.go index 4941c3d4..ffccff4d 100644 --- a/cmd/dump/dump_integration_test.go +++ b/cmd/dump/dump_integration_test.go @@ -130,6 +130,13 @@ func TestDumpCommand_Issue345ArrayCast(t *testing.T) { runExactMatchTest(t, "issue_345_array_cast") } +func TestDumpCommand_Issue396CheckConstraintIsNotNull(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + runExactMatchTest(t, "issue_396_check_constraint_is_not_null") +} + func TestDumpCommand_Issue191FunctionProcedureOverload(t *testing.T) { if testing.Short() { t.Skip("Skipping integration test in short mode") diff --git a/ir/inspector.go b/ir/inspector.go index 06768763..10625958 100644 --- a/ir/inspector.go +++ b/ir/inspector.go @@ -517,9 +517,18 @@ func (i *Inspector) buildConstraints(ctx context.Context, schema *IR, targetSche // Handle check constraints if cType == ConstraintTypeCheck { if checkClause := i.safeInterfaceToString(constraint.CheckClause); checkClause != "" && checkClause != "" { - // Skip system-generated NOT NULL constraints as they're redundant with column definitions - if strings.Contains(checkClause, "IS NOT NULL") { - continue + // Skip simple NOT NULL check constraints (e.g., "CHECK ((value IS NOT NULL))") + // as they're redundant with column definitions. Only skip if the entire + // expression is just "identifier IS NOT NULL". + inner := strings.TrimPrefix(strings.TrimSpace(checkClause), "CHECK ") + inner = strings.TrimSpace(inner) + for len(inner) > 2 && inner[0] == '(' && inner[len(inner)-1] == ')' && isBalancedParentheses(inner[1:len(inner)-1]) { + inner = strings.TrimSpace(inner[1 : len(inner)-1]) + } + if prefix, ok := strings.CutSuffix(inner, " IS NOT NULL"); ok { + if !strings.Contains(strings.TrimSpace(prefix), " ") { + continue + } } // Use CheckClause as-is from PostgreSQL's pg_get_constraintdef(c.oid, true) diff --git a/testdata/dump/issue_396_check_constraint_is_not_null/manifest.json b/testdata/dump/issue_396_check_constraint_is_not_null/manifest.json new file mode 100644 index 00000000..cec8d422 --- /dev/null +++ b/testdata/dump/issue_396_check_constraint_is_not_null/manifest.json @@ -0,0 +1,9 @@ +{ + "name": "issue_396_check_constraint_is_not_null", + "description": "Table-level CHECK constraints containing IS NOT NULL in complex expressions are silently omitted from schema dump", + "source": "https://github.com/pgplex/pgschema/issues/396", + "notes": [ + "The inspector incorrectly skips CHECK constraints that contain 'IS NOT NULL' anywhere in the expression", + "Only simple NOT NULL check constraints should be skipped, not complex expressions using IS NOT NULL" + ] +} diff --git a/testdata/dump/issue_396_check_constraint_is_not_null/pgdump.sql b/testdata/dump/issue_396_check_constraint_is_not_null/pgdump.sql new file mode 100644 index 00000000..374c8c8a --- /dev/null +++ b/testdata/dump/issue_396_check_constraint_is_not_null/pgdump.sql @@ -0,0 +1,40 @@ +-- +-- PostgreSQL database dump +-- + +SET statement_timeout = 0; +SET lock_timeout = 0; +SET client_encoding = 'UTF8'; +SET standard_conforming_strings = on; +SET check_function_bodies = false; +SET client_min_messages = warning; +SET row_security = off; + +-- +-- Name: test_table; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.test_table ( + id integer NOT NULL, + status text NOT NULL, + reason text, + actor_id uuid +); + +-- +-- Name: test_table test_table_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.test_table + ADD CONSTRAINT test_table_pkey PRIMARY KEY (id); + +-- +-- Name: test_table test_table_status_check; Type: CHECK CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE public.test_table + ADD CONSTRAINT test_table_status_check CHECK (((status = 'active'::text) OR ((status = 'cancelled'::text) AND (reason IS NOT NULL)) OR ((status = 'revoked'::text) AND (actor_id IS NOT NULL)))); + +-- +-- PostgreSQL database dump complete +-- diff --git a/testdata/dump/issue_396_check_constraint_is_not_null/pgschema.sql b/testdata/dump/issue_396_check_constraint_is_not_null/pgschema.sql new file mode 100644 index 00000000..322aa29e --- /dev/null +++ b/testdata/dump/issue_396_check_constraint_is_not_null/pgschema.sql @@ -0,0 +1,21 @@ +-- +-- pgschema database dump +-- + +-- Dumped from database version PostgreSQL 18.0 +-- Dumped by pgschema version 1.9.0 + + +-- +-- Name: test_table; Type: TABLE; Schema: -; Owner: - +-- + +CREATE TABLE IF NOT EXISTS test_table ( + id integer, + status text NOT NULL, + reason text, + actor_id uuid, + CONSTRAINT test_table_pkey PRIMARY KEY (id), + CONSTRAINT test_table_status_check CHECK (status = 'active'::text OR status = 'cancelled'::text AND reason IS NOT NULL OR status = 'revoked'::text AND actor_id IS NOT NULL) +); + diff --git a/testdata/dump/issue_396_check_constraint_is_not_null/raw.sql b/testdata/dump/issue_396_check_constraint_is_not_null/raw.sql new file mode 100644 index 00000000..510b9a20 --- /dev/null +++ b/testdata/dump/issue_396_check_constraint_is_not_null/raw.sql @@ -0,0 +1,19 @@ +-- +-- Test case for GitHub issue #396: Table-level CHECK constraints omitted from schema dump +-- +-- CHECK constraints containing IS NOT NULL in complex expressions +-- are silently dropped because the inspector filters out any constraint +-- with "IS NOT NULL" in its expression, not just simple NOT NULL constraints. +-- + +CREATE TABLE test_table ( + id int PRIMARY KEY, + status text NOT NULL, + reason text, + actor_id uuid, + CONSTRAINT test_table_status_check CHECK ( + (status = 'active') + OR (status = 'cancelled' AND reason IS NOT NULL) + OR (status = 'revoked' AND actor_id IS NOT NULL) + ) +);