Merge pull request #281 from constructive-io/devin/1769151809-plpgsql-row-fields-out-params

pyramation · web-flow · commit d83f3338c3e3 · 2026-01-22T23:56:52.000-08:00
fix: normalize INTO clause whitespace + add OUT parameter test fixtures
diff --git a/__fixtures__/plpgsql-generated/generated.json b/__fixtures__/plpgsql-generated/generated.json
@@ -100,8 +100,10 @@
   "plpgsql_deparser_fixes-11.sql": "-- Test 11: OUT parameter function with bare RETURN\nCREATE FUNCTION test_out_params(OUT ok boolean, OUT message text)\nLANGUAGE plpgsql AS $$\nBEGIN\n  ok := true;\n  message := 'success';\n  RETURN;\nEND$$",
   "plpgsql_deparser_fixes-12.sql": "-- Test 12: RETURNS TABLE function with RETURN QUERY\nCREATE FUNCTION test_returns_table(p_prefix text)\nRETURNS TABLE(id int, name text)\nLANGUAGE plpgsql AS $$\nBEGIN\n  RETURN QUERY SELECT 1, p_prefix || '_one';\n  RETURN QUERY SELECT 2, p_prefix || '_two';\n  RETURN;\nEND$$",
   "plpgsql_deparser_fixes-13.sql": "-- Test 13: Trigger function with complex logic\nCREATE FUNCTION test_trigger_complex() RETURNS trigger\nLANGUAGE plpgsql AS $$\nDECLARE\n  defaults_record record;\n  bit_len int;\nBEGIN\n  bit_len := bit_length(NEW.permissions);\n  \n  SELECT * INTO defaults_record\n  FROM permission_defaults AS t\n  LIMIT 1;\n  \n  IF found THEN\n    NEW.is_approved := defaults_record.is_approved;\n    NEW.is_verified := defaults_record.is_verified;\n  END IF;\n  \n  IF NEW.is_owner IS TRUE THEN\n    NEW.is_admin := true;\n    NEW.is_approved := true;\n    NEW.is_verified := true;\n  END IF;\n  \n  SELECT\n    NEW.is_approved IS TRUE\n      AND NEW.is_verified IS TRUE\n      AND NEW.is_disabled IS FALSE INTO NEW.is_active;\n  \n  RETURN NEW;\nEND$$",
-  "plpgsql_deparser_fixes-14.sql": "-- Test 14: Procedure (implicit void return)\nCREATE PROCEDURE test_procedure(p_message text)\nLANGUAGE plpgsql AS $$\nBEGIN\n  RAISE NOTICE '%', p_message;\nEND$$",
-  "plpgsql_control-1.sql": "--\n-- Tests for PL/pgSQL control structures\n--\n\n-- integer FOR loop\n\ndo $$\nbegin\n  -- basic case\n  for i in 1..3 loop\n    raise notice '1..3: i = %', i;\n  end loop;\n  -- with BY, end matches exactly\n  for i in 1..10 by 3 loop\n    raise notice '1..10 by 3: i = %', i;\n  end loop;\n  -- with BY, end does not match\n  for i in 1..11 by 3 loop\n    raise notice '1..11 by 3: i = %', i;\n  end loop;\n  -- zero iterations\n  for i in 1..0 by 3 loop\n    raise notice '1..0 by 3: i = %', i;\n  end loop;\n  -- REVERSE\n  for i in reverse 10..0 by 3 loop\n    raise notice 'reverse 10..0 by 3: i = %', i;\n  end loop;\n  -- potential overflow\n  for i in 2147483620..2147483647 by 10 loop\n    raise notice '2147483620..2147483647 by 10: i = %', i;\n  end loop;\n  -- potential overflow, reverse direction\n  for i in reverse -2147483620..-2147483647 by 10 loop\n    raise notice 'reverse -2147483620..-2147483647 by 10: i = %', i;\n  end loop;\nend$$",
+    "plpgsql_deparser_fixes-14.sql": "-- Test 14: Procedure (implicit void return)\nCREATE PROCEDURE test_procedure(p_message text)\nLANGUAGE plpgsql AS $$\nBEGIN\n  RAISE NOTICE '%', p_message;\nEND$$",
+    "plpgsql_deparser_fixes-15.sql": "-- Test 15: OUT parameters with SELECT INTO multiple variables\n-- This pattern is used in auth functions (sign_in, sign_up) where we need to\n-- populate multiple OUT parameters from a single SELECT statement\nCREATE FUNCTION test_out_params_select_into(\n  p_user_id uuid,\n  OUT id uuid,\n  OUT user_id uuid,\n  OUT access_token text,\n  OUT access_token_expires_at timestamptz,\n  OUT is_verified boolean,\n  OUT totp_enabled boolean\n)\nLANGUAGE plpgsql AS $$\nDECLARE\n  v_token_id uuid;\n  v_plaintext_token text;\nBEGIN\n  v_plaintext_token := encode(gen_random_bytes(48), 'hex');\n  v_token_id := uuid_generate_v5(uuid_ns_url(), v_plaintext_token);\n  \n  INSERT INTO tokens (id, user_id, access_token_hash)\n  VALUES (v_token_id, p_user_id, digest(v_plaintext_token, 'sha256'));\n  \n  SELECT tkn.id, tkn.user_id, v_plaintext_token, tkn.access_token_expires_at, tkn.is_verified, tkn.totp_enabled\n  INTO id, user_id, access_token, access_token_expires_at, is_verified, totp_enabled\n  FROM tokens AS tkn\n  WHERE tkn.id = v_token_id;\n  \n  RETURN;\nEND$$",
+    "plpgsql_deparser_fixes-16.sql": "-- Test 16: OUT parameters with SELECT INTO and STRICT\nCREATE FUNCTION test_out_params_strict(\n  p_id uuid,\n  OUT name text,\n  OUT email text\n)\nLANGUAGE plpgsql AS $$\nBEGIN\n  SELECT u.name, u.email INTO STRICT name, email\n  FROM users u\n  WHERE u.id = p_id;\nEND$$",
+    "plpgsql_control-1.sql":"--\n-- Tests for PL/pgSQL control structures\n--\n\n-- integer FOR loop\n\ndo $$\nbegin\n  -- basic case\n  for i in 1..3 loop\n    raise notice '1..3: i = %', i;\n  end loop;\n  -- with BY, end matches exactly\n  for i in 1..10 by 3 loop\n    raise notice '1..10 by 3: i = %', i;\n  end loop;\n  -- with BY, end does not match\n  for i in 1..11 by 3 loop\n    raise notice '1..11 by 3: i = %', i;\n  end loop;\n  -- zero iterations\n  for i in 1..0 by 3 loop\n    raise notice '1..0 by 3: i = %', i;\n  end loop;\n  -- REVERSE\n  for i in reverse 10..0 by 3 loop\n    raise notice 'reverse 10..0 by 3: i = %', i;\n  end loop;\n  -- potential overflow\n  for i in 2147483620..2147483647 by 10 loop\n    raise notice '2147483620..2147483647 by 10: i = %', i;\n  end loop;\n  -- potential overflow, reverse direction\n  for i in reverse -2147483620..-2147483647 by 10 loop\n    raise notice 'reverse -2147483620..-2147483647 by 10: i = %', i;\n  end loop;\nend$$",
   "plpgsql_control-2.sql": "-- BY can't be zero or negative\ndo $$\nbegin\n  for i in 1..3 by 0 loop\n    raise notice '1..3 by 0: i = %', i;\n  end loop;\nend$$",
   "plpgsql_control-3.sql": "do $$\nbegin\n  for i in 1..3 by -1 loop\n    raise notice '1..3 by -1: i = %', i;\n  end loop;\nend$$",
   "plpgsql_control-4.sql": "do $$\nbegin\n  for i in reverse 1..3 by -1 loop\n    raise notice 'reverse 1..3 by -1: i = %', i;\n  end loop;\nend$$",
@@ -189,4 +191,4 @@
   "plpgsql_array-21.sql": "-- some types don't support arrays\ndo $$\ndeclare\n  v pg_node_tree;\n  v1 v%type[];\nbegin\nend;\n$$",
   "plpgsql_array-22.sql": "-- check functionality\ndo $$\ndeclare\n  v1 int;\n  v2 varchar;\n  a1 v1%type[];\n  a2 v2%type[];\nbegin\n  v1 := 10;\n  v2 := 'Hi';\n  a1 := array[v1,v1];\n  a2 := array[v2,v2];\n  raise notice '% %', a1, a2;\nend;\n$$",
   "plpgsql_array-23.sql": "do $$\ndeclare tg array_test_table%rowtype[];\nbegin\n  tg := array(select array_test_table from array_test_table);\n  raise notice '%', tg;\n  tg := array(select row(a,b) from array_test_table);\n  raise notice '%', tg;\nend;\n$$"
-}
+}
diff --git a/__fixtures__/plpgsql/plpgsql_deparser_fixes.sql b/__fixtures__/plpgsql/plpgsql_deparser_fixes.sql
@@ -162,3 +162,47 @@ LANGUAGE plpgsql AS $$
 BEGIN
   RAISE NOTICE '%', p_message;
 END$$;
+
+-- Test 15: OUT parameters with SELECT INTO multiple variables
+-- This pattern is used in auth functions (sign_in, sign_up) where we need to
+-- populate multiple OUT parameters from a single SELECT statement
+CREATE FUNCTION test_out_params_select_into(
+  p_user_id uuid,
+  OUT id uuid,
+  OUT user_id uuid,
+  OUT access_token text,
+  OUT access_token_expires_at timestamptz,
+  OUT is_verified boolean,
+  OUT totp_enabled boolean
+)
+LANGUAGE plpgsql AS $$
+DECLARE
+  v_token_id uuid;
+  v_plaintext_token text;
+BEGIN
+  v_plaintext_token := encode(gen_random_bytes(48), 'hex');
+  v_token_id := uuid_generate_v5(uuid_ns_url(), v_plaintext_token);
+  
+  INSERT INTO tokens (id, user_id, access_token_hash)
+  VALUES (v_token_id, p_user_id, digest(v_plaintext_token, 'sha256'));
+  
+  SELECT tkn.id, tkn.user_id, v_plaintext_token, tkn.access_token_expires_at, tkn.is_verified, tkn.totp_enabled
+  INTO id, user_id, access_token, access_token_expires_at, is_verified, totp_enabled
+  FROM tokens AS tkn
+  WHERE tkn.id = v_token_id;
+  
+  RETURN;
+END$$;
+
+-- Test 16: OUT parameters with SELECT INTO and STRICT
+CREATE FUNCTION test_out_params_strict(
+  p_id uuid,
+  OUT name text,
+  OUT email text
+)
+LANGUAGE plpgsql AS $$
+BEGIN
+  SELECT u.name, u.email INTO STRICT name, email
+  FROM users u
+  WHERE u.id = p_id;
+END$$;
diff --git a/packages/plpgsql-deparser/AGENTS.md b/packages/plpgsql-deparser/AGENTS.md
@@ -0,0 +1,104 @@
+# PL/pgSQL Deparser - Agent Instructions
+
+## Adding Test Fixtures
+
+When adding new test fixtures for the PL/pgSQL deparser, follow this workflow:
+
+### Step 1: Add SQL Fixtures
+
+Add your PL/pgSQL function/procedure definitions to the appropriate fixture file in `__fixtures__/plpgsql/`. For deparser-specific fixes, use `plpgsql_deparser_fixes.sql`.
+
+Example fixture:
+```sql
+-- Test N: Description of what this tests
+CREATE FUNCTION test_example(p_input text, OUT result text)
+LANGUAGE plpgsql AS $$
+BEGIN
+  result := p_input;
+  RETURN;
+END$$;
+```
+
+### Step 2: Generate Test Fixtures
+
+Run the fixture generation script from the plpgsql-deparser package:
+
+```bash
+cd packages/plpgsql-deparser
+pnpm fixtures
+```
+
+This script (`scripts/make-fixtures.ts`):
+1. Reads all `.sql` files from `__fixtures__/plpgsql/`
+2. Parses each file to extract PL/pgSQL statements (CREATE FUNCTION, CREATE PROCEDURE, DO blocks)
+3. Validates each statement can be parsed by the PL/pgSQL parser
+4. Outputs valid fixtures to `__fixtures__/plpgsql-generated/generated.json`
+
+### Step 3: Run Tests
+
+Run the test suite to verify your fixtures round-trip correctly:
+
+```bash
+cd packages/plpgsql-deparser
+pnpm test
+```
+
+The round-trip test (`__tests__/plpgsql-deparser.test.ts`):
+1. Loads all fixtures from `generated.json`
+2. For each fixture: parse -> deparse -> reparse
+3. Compares the AST from original parse with the AST from reparsed output
+4. Reports any failures (AST mismatches or reparse failures)
+
+### Step 4: Add Snapshot Tests (Optional but Recommended)
+
+For important deparser fixes, add explicit test cases with snapshots to `__tests__/deparser-fixes.test.ts`:
+
+```typescript
+it('should handle [description]', async () => {
+  const sql = `CREATE FUNCTION test_example(...)
+LANGUAGE plpgsql AS $$
+BEGIN
+  -- your test case
+END$$`;
+
+  await testUtils.expectAstMatch('description', sql);
+
+  const parsed = parsePlPgSQLSync(sql) as unknown as PLpgSQLParseResult;
+  const deparsed = deparseSync(parsed);
+  expect(deparsed).toMatchSnapshot();
+  // Add specific assertions
+  expect(deparsed).toContain('expected output');
+});
+```
+
+Then run tests with snapshot update:
+
+```bash
+pnpm test --updateSnapshot
+```
+
+### Step 5: Commit All Files
+
+Always commit the fixture file, generated.json, test file, AND snapshots together:
+
+```bash
+git add __fixtures__/plpgsql/plpgsql_deparser_fixes.sql
+git add __fixtures__/plpgsql-generated/generated.json
+git add packages/plpgsql-deparser/__tests__/deparser-fixes.test.ts
+git add packages/plpgsql-deparser/__tests__/__snapshots__/deparser-fixes.test.ts.snap
+git commit -m "test: add fixtures for [description]"
+```
+
+## Important Notes
+
+- The `generated.json` file is the source of truth for tests - it must be regenerated when fixtures change
+- Fixtures that fail PL/pgSQL parsing are skipped (logged as warnings during generation)
+- The test suite has a `KNOWN_FAILING_FIXTURES` set for fixtures with known issues - avoid adding to this unless necessary
+- When adding fixtures for new deparser features, ensure the fixture exercises the specific AST pattern you're testing
+
+## Fixture File Conventions
+
+- `plpgsql_deparser_fixes.sql` - Fixtures for deparser bug fixes and edge cases
+- `plpgsql_*.sql` - PostgreSQL regression test fixtures (from upstream)
+- Each fixture should have a comment describing what it tests
+- Number fixtures sequentially (Test 1, Test 2, etc.) within each file
diff --git a/packages/plpgsql-deparser/__tests__/__snapshots__/deparser-fixes.test.ts.snap b/packages/plpgsql-deparser/__tests__/__snapshots__/deparser-fixes.test.ts.snap
@@ -70,6 +70,30 @@ BEGIN
 END"
 `;
 
+exports[`plpgsql-deparser bug fixes OUT parameters with SELECT INTO multiple variables should handle SELECT INTO STRICT with multiple OUT parameters 1`] = `
+"BEGIN
+  SELECT u.name, u.email INTO STRICT name, email FROM users u
+    WHERE u.id = p_id;
+  RETURN;
+END"
+`;
+
+exports[`plpgsql-deparser bug fixes OUT parameters with SELECT INTO multiple variables should handle SELECT INTO multiple OUT parameters 1`] = `
+"DECLARE
+  v_token_id uuid;
+  v_plaintext_token text;
+BEGIN
+  v_plaintext_token := encode(gen_random_bytes(48), 'hex');
+  v_token_id := uuid_generate_v5(uuid_ns_url(), v_plaintext_token);
+  INSERT INTO tokens (id, user_id, access_token_hash)
+    VALUES (v_token_id, p_user_id, digest(v_plaintext_token, 'sha256'));
+  SELECT tkn.id, tkn.user_id, v_plaintext_token, tkn.access_token_expires_at, tkn.is_verified, tkn.totp_enabled INTO id, user_id, access_token, access_token_expires_at, is_verified, totp_enabled
+      FROM tokens AS tkn
+    WHERE tkn.id = v_token_id;
+  RETURN;
+END"
+`;
+
 exports[`plpgsql-deparser bug fixes PERFORM SELECT fix should handle PERFORM with complex expressions 1`] = `
 "BEGIN
   PERFORM set_config('search_path', 'public', true);
diff --git a/packages/plpgsql-deparser/__tests__/__snapshots__/hydrate-demo.test.ts.snap b/packages/plpgsql-deparser/__tests__/__snapshots__/hydrate-demo.test.ts.snap
@@ -98,7 +98,7 @@ BEGIN
         t.orders_scanned,
         t.gross_total,
         t.avg_total INTO v_orders_scanned, v_gross, v_avg
-      FROM totals AS t;
+          FROM totals AS t;
       IF p_apply_discount THEN
             v_rebate := round(v_gross * GREATEST(LEAST(v_discount_rate + v_jitter, 0.50), 0), p_round_to);
       ELSE
@@ -109,7 +109,7 @@ BEGIN
       SELECT
         oi.sku,
         CAST(sum(oi.quantity) AS bigint) AS qty INTO v_top_sku, v_top_sku_qty
-      FROM app_public.order_item AS oi
+          FROM app_public.order_item AS oi
       JOIN app_public.app_order AS o ON o.id = oi.order_id
       WHERE
         o.org_id = p_org_id
diff --git a/packages/plpgsql-deparser/__tests__/__snapshots__/schema-rename-mapped.test.ts.snap b/packages/plpgsql-deparser/__tests__/__snapshots__/schema-rename-mapped.test.ts.snap
@@ -194,7 +194,7 @@ exports[`schema rename mapped should transform schema names and snapshot schema
   total_count int;
 BEGIN
   SELECT count(*) INTO total_count
-  FROM myapp_v2.users AS u
+      FROM myapp_v2.users AS u
   JOIN myapp_v2.orders AS o ON o.user_id = u.id
   WHERE
     u.id = p_user_id;
@@ -270,7 +270,7 @@ CREATE FUNCTION myapp_v2.calculate_order_total(
   discount numeric;
 BEGIN
   SELECT sum(quantity * price) INTO subtotal
-  FROM myapp_v2.order_items
+      FROM myapp_v2.order_items
   WHERE
     order_id = p_order_id;
   tax_amount := myapp_v2.get_tax_rate() * subtotal;
diff --git a/packages/plpgsql-deparser/__tests__/deparser-fixes.test.ts b/packages/plpgsql-deparser/__tests__/deparser-fixes.test.ts
@@ -307,6 +307,69 @@ $$`;
     });
   });
 
+  describe('OUT parameters with SELECT INTO multiple variables', () => {
+    it('should handle SELECT INTO multiple OUT parameters', async () => {
+      const sql = `CREATE FUNCTION test_out_params_select_into(
+  p_user_id uuid,
+  OUT id uuid,
+  OUT user_id uuid,
+  OUT access_token text,
+  OUT access_token_expires_at timestamptz,
+  OUT is_verified boolean,
+  OUT totp_enabled boolean
+)
+LANGUAGE plpgsql AS $$
+DECLARE
+  v_token_id uuid;
+  v_plaintext_token text;
+BEGIN
+  v_plaintext_token := encode(gen_random_bytes(48), 'hex');
+  v_token_id := uuid_generate_v5(uuid_ns_url(), v_plaintext_token);
+  
+  INSERT INTO tokens (id, user_id, access_token_hash)
+  VALUES (v_token_id, p_user_id, digest(v_plaintext_token, 'sha256'));
+  
+  SELECT tkn.id, tkn.user_id, v_plaintext_token, tkn.access_token_expires_at, tkn.is_verified, tkn.totp_enabled
+  INTO id, user_id, access_token, access_token_expires_at, is_verified, totp_enabled
+  FROM tokens AS tkn
+  WHERE tkn.id = v_token_id;
+  
+  RETURN;
+END$$`;
+
+      await testUtils.expectAstMatch('OUT params SELECT INTO', sql);
+
+      const parsed = parsePlPgSQLSync(sql) as unknown as PLpgSQLParseResult;
+      const deparsed = deparseSync(parsed);
+      expect(deparsed).toMatchSnapshot();
+      // Verify multiple INTO targets are present
+      expect(deparsed).toMatch(/INTO\s+id\s*,\s*user_id\s*,\s*access_token/i);
+    });
+
+    it('should handle SELECT INTO STRICT with multiple OUT parameters', async () => {
+      const sql = `CREATE FUNCTION test_out_params_strict(
+  p_id uuid,
+  OUT name text,
+  OUT email text
+)
+LANGUAGE plpgsql AS $$
+BEGIN
+  SELECT u.name, u.email INTO STRICT name, email
+  FROM users u
+  WHERE u.id = p_id;
+END$$`;
+
+      await testUtils.expectAstMatch('OUT params STRICT', sql);
+
+      const parsed = parsePlPgSQLSync(sql) as unknown as PLpgSQLParseResult;
+      const deparsed = deparseSync(parsed);
+      expect(deparsed).toMatchSnapshot();
+      expect(deparsed).toContain('STRICT');
+      // Verify multiple INTO targets are present
+      expect(deparsed).toMatch(/INTO\s+STRICT\s+name\s*,\s*email/i);
+    });
+  });
+
   describe('combined scenarios', () => {
     it('should handle PERFORM with record fields', async () => {
       const sql = `CREATE FUNCTION test_perform_record() RETURNS trigger
diff --git a/packages/plpgsql-deparser/__tests__/pretty/__snapshots__/plpgsql-pretty.test.ts.snap b/packages/plpgsql-deparser/__tests__/pretty/__snapshots__/plpgsql-pretty.test.ts.snap
@@ -66,7 +66,7 @@ begin
           t.orders_scanned,
           t.gross_total,
           t.avg_total into v_orders_scanned, v_gross, v_avg
-                                                            FROM totals t;
+          FROM totals t;
       if p_apply_discount then
             v_discount := round(v_gross * GREATEST(LEAST(v_discount_rate + v_jitter, 0.50), 0), p_round_to);
       else
@@ -77,7 +77,7 @@ begin
       SELECT
           oi.sku,
           sum(oi.quantity)::bigint AS qty into v_top_sku, v_top_sku_qty
-                                        FROM app_public.order_item oi
+          FROM app_public.order_item oi
         JOIN app_public.app_order o ON o.id = oi.order_id
         WHERE o.org_id = p_org_id
           AND o.user_id = p_user_id
@@ -295,7 +295,7 @@ BEGIN
           t.orders_scanned,
           t.gross_total,
           t.avg_total INTO v_orders_scanned, v_gross, v_avg
-                                                            FROM totals t;
+          FROM totals t;
       IF p_apply_discount THEN
             v_discount := round(v_gross * GREATEST(LEAST(v_discount_rate + v_jitter, 0.50), 0), p_round_to);
       ELSE
@@ -306,7 +306,7 @@ BEGIN
       SELECT
           oi.sku,
           sum(oi.quantity)::bigint AS qty INTO v_top_sku, v_top_sku_qty
-                                        FROM app_public.order_item oi
+          FROM app_public.order_item oi
         JOIN app_public.app_order o ON o.id = oi.order_id
         WHERE o.org_id = p_org_id
           AND o.user_id = p_user_id
diff --git a/packages/plpgsql-deparser/src/plpgsql-deparser.ts b/packages/plpgsql-deparser/src/plpgsql-deparser.ts
@@ -1485,8 +1485,23 @@ export class PLpgSQLDeparser {
         // large gaps like "SELECT x INTO y                    FROM z"
         const before = sql.slice(0, insertPos);
         let after = sql.slice(insertPos);
-        // Collapse leading whitespace (but preserve a single space before the next keyword)
-        after = after.replace(/^[ \t]+/, ' ');
+        // Normalize whitespace after INTO insertion
+        // The parser strips "INTO <target>" but leaves whitespace behind, which can cause
+        // weird formatting like "SELECT x INTO y                    FROM z"
+        // We collapse all whitespace to either a single space or a newline with standard indent
+        const leadingWsMatch = after.match(/^(\s+)/);
+        if (leadingWsMatch) {
+          const ws = leadingWsMatch[1];
+          const hasNewline = /\n/.test(ws);
+          if (hasNewline) {
+            // If original had newlines, use a newline with standard 4-space indent
+            // This normalizes any weird indentation left by the parser
+            after = after.replace(/^\s+/, '\n    ');
+          } else {
+            // If original was just spaces/tabs, collapse to single space
+            after = after.replace(/^[ \t]+/, ' ');
+          }
+        }
         sql = before + intoClause + after;
       } else {
         // -1 means INTO already exists at depth 0, don't add another one
