Merge pull request #10 from bitner/fix/funcchanges

Fix routine diff handling and update docs

Author: bitner

CHANGELOG.md

@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Fixed
+- `makemigration` now preserves body-only routine changes by appending `CREATE OR REPLACE` definitions when `results.schemadiff_as_sql()` misses them.
+- `makemigration` now filters unsafe routine drops only when the same routine kind and signature still exist in the target schema, avoiding false positives during function-to-procedure transitions.
+
 ## [0.1.0] - 2026-05-05
 
 ### Added

README.md

@@ -9,7 +9,8 @@ ordered `sql/` directory. `pgpkg` does everything else:
    `<prefix>--<version>.sql`.
 2. **`makemigration`** — diffs two staged versions with
    [results](https://github.com/djrobstep/results) to generate an incremental
-   migration `<prefix>--A--B.sql`.
+   migration `<prefix>--A--B.sql`, including body-only function/procedure
+   changes that require `CREATE OR REPLACE`.
 3. **`graph`** — shows the version graph and shortest paths between versions.
 4. **`migrate`** — connects to a live database (libpq env vars or psql-style
    flags), reads the currently installed version, and applies the shortest

docs/architecture.md

@@ -10,7 +10,7 @@ A short tour of the modules:
 | `pgpkg.staging` | render `sql/` into a single string with fragment markers |
 | `pgpkg.versioning` | PEP 440 + `unreleased`-last ordering |
 | `pgpkg.planner` | BFS over the catalog graph for shortest `source → target` paths |
-| `pgpkg.diff` | wrap `results.temporary_local_db` + `schemadiff_as_sql` |
+| `pgpkg.diff` | wrap `results.temporary_local_db` + `schemadiff_as_sql`, then patch in body-only routine replacements and safe routine-drop filtering |
 | `pgpkg.tracking` | default tracking DDL, version-source protocol, role-safe bookkeeping |
 | `pgpkg.executor` | run a plan in one xact with `pg_advisory_xact_lock` |
 | `pgpkg._conn` | thin psycopg helper honoring libpq env vars |

docs/cli.md

@@ -83,7 +83,9 @@ pgpkg makemigration [--from VERSION] [--to VERSION] [--base-url URL] [--output P
 Writes `<prefix>--<from>--<to>.sql`. `--base-url` is the postgres URL used
 to spawn tempdbs via `results.temporary_local_db`. Defaults to
 `postgresql:///postgres`, i.e. a local admin connection through the peer
-socket.
+socket. When the raw schema diff misses body-only function or procedure
+changes, `pgpkg` appends `CREATE OR REPLACE` definitions from the target
+version and avoids unsafe same-kind routine drops.
 
 When wrapper SQL is supplied, `pgpkg` renders the output in this order:
 

docs/index.md

@@ -15,7 +15,9 @@ code. You own:
 
 - **stageversion** — renders `sql/` into `<prefix>--<version>.sql`
 - **makemigration** — diffs two staged versions into `<prefix>--<from>--<to>.sql`
-  using `results.temporary_local_db` + `results.schemadiff_as_sql`
+  using `results.temporary_local_db` + `results.schemadiff_as_sql`, with an
+  extra routine-definition pass so body-only function/procedure changes are
+  emitted as `CREATE OR REPLACE`
 - **graph** / **plan** — show the version graph and the shortest path to a
   target version
 - **migrate** — apply the plan inside one transaction with an advisory lock,

src/pgpkg/diff.py

@@ -2,8 +2,10 @@
 
 from __future__ import annotations
 
+import re
 from dataclasses import dataclass
 from pathlib import Path
+from typing import Any
 
 from .config import ProjectConfig
 from .errors import PgpkgError
@@ -53,9 +55,131 @@ def generate_incremental_sql(
         with db_to.t() as t:
             t.execute(to_sql)
         diff_sql: str = db_from.schemadiff_as_sql(db_to)
+        target_routine_sigs = _load_routine_signatures(db_to, config.prefix)
+        diff_sql = _strip_unsafe_routine_drops(diff_sql, target_routine_sigs)
+        routine_diff_sql = _changed_routine_replacements_sql(db_from, db_to, config.prefix)
+
+    if routine_diff_sql.strip():
+        if diff_sql.strip():
+            diff_sql = f"{diff_sql.rstrip()}\n\n{routine_diff_sql.rstrip()}\n"
+        else:
+            diff_sql = f"{routine_diff_sql.rstrip()}\n"
     return diff_sql
 
 
+def _changed_routine_replacements_sql(db_from: Any, db_to: Any, schema: str) -> str:
+    """Return CREATE OR REPLACE SQL for routines whose bodies changed.
+
+    `results.schemadiff_as_sql()` can miss body-only routine changes. Detect
+    those by comparing `pg_get_functiondef` between the staged `from` and `to`
+    databases and emit replacement definitions from `to`.
+    """
+    from_map = _load_routine_definitions(db_from, schema)
+    to_map = _load_routine_definitions(db_to, schema)
+
+    replacements: list[str] = []
+    for key, to_def in sorted(to_map.items()):
+        from_def = from_map.get(key)
+        if from_def is None:
+            continue
+        if _normalize_sql(from_def) == _normalize_sql(to_def):
+            continue
+        replacements.append(f"{to_def.rstrip()}\n;")
+    return "\n\n".join(replacements)
+
+
+def _load_routine_definitions(db: Any, schema: str) -> dict[tuple[str, str, str, str], str]:
+    schema_lit = schema.replace("'", "''")
+    sql = f"""
+        SELECT
+            n.nspname,
+            p.prokind,
+            p.proname,
+            pg_get_function_identity_arguments(p.oid) AS identity_args,
+            pg_get_functiondef(p.oid) AS definition
+        FROM pg_proc p
+        JOIN pg_namespace n ON n.oid = p.pronamespace
+        WHERE n.nspname = '{schema_lit}'
+          AND p.prokind IN ('f', 'p')
+    """
+    with db.t() as t:
+        rows = t.execute(sql)
+
+    out: dict[tuple[str, str, str, str], str] = {}
+    for nspname, prokind, proname, identity_args, definition in rows:
+        key = (str(nspname), str(prokind), str(proname), str(identity_args))
+        out[key] = str(definition)
+    return out
+
+
+def _load_routine_signatures(db: Any, schema: str) -> set[tuple[str, str]]:
+    schema_lit = schema.replace("'", "''")
+    sql = f"""
+        SELECT
+            CASE p.prokind
+                WHEN 'p' THEN 'procedure'
+                ELSE 'function'
+            END,
+            n.nspname || '.' || p.proname || '(' || oidvectortypes(p.proargtypes) || ')'
+        FROM pg_proc p
+        JOIN pg_namespace n ON n.oid = p.pronamespace
+        WHERE n.nspname = '{schema_lit}'
+          AND p.prokind IN ('f', 'p')
+    """
+    with db.t() as t:
+        rows = t.execute(sql)
+    return {
+        (_normalize_routine_kind(str(kind)), _normalize_signature_text(str(sig)))
+        for kind, sig in rows
+    }
+
+
+def _strip_unsafe_routine_drops(diff_sql: str, target_signatures: set[tuple[str, str]]) -> str:
+    """Remove DROP FUNCTION/PROCEDURE statements for routines still in target.
+
+    Some diffs emit a DROP for a routine that is still present in the target
+    schema (typically a body-only or property-only change). That can fail when
+    other objects depend on the routine; `CREATE OR REPLACE` is the safe path.
+    """
+    if not diff_sql.strip():
+        return diff_sql
+
+    out_lines: list[str] = []
+    drop_re = re.compile(
+        r"^\s*drop\s+(function|procedure)\s+if\s+exists\s+(.+?)\s*;\s*$",
+        re.IGNORECASE,
+    )
+
+    for line in diff_sql.splitlines():
+        m = drop_re.match(line)
+        if m:
+            target_key = (
+                _normalize_routine_kind(m.group(1)),
+                _normalize_signature_text(m.group(2)),
+            )
+            if target_key in target_signatures:
+                continue
+        out_lines.append(line)
+    return "\n".join(out_lines)
+
+
+def _normalize_routine_kind(kind: str) -> str:
+    return kind.strip().lower()
+
+
+def _normalize_signature_text(sig: str) -> str:
+    s = sig.replace('"', "").strip()
+    s = re.sub(r"\s*,\s*", ",", s)
+    s = re.sub(r"\(\s*", "(", s)
+    s = re.sub(r"\s*\)", ")", s)
+    s = re.sub(r"\s+", " ", s)
+    return s
+
+
+def _normalize_sql(sql: str) -> str:
+    return "\n".join(line.rstrip() for line in sql.strip().splitlines())
+
+
 def write_incremental(
     config: ProjectConfig,
     *,

tests/integration/test_diff.py

@@ -80,3 +80,73 @@ def test_makemigration_wraps_diff_with_files_and_sql(
     assert '"sampleext"."extra"' in body
     assert "SELECT 42;" in body
     assert "SELECT 'done';" in body
+
+
+def test_makemigration_includes_body_only_function_changes(
+    staged_project: Path,
+    pg_url: str,
+):
+    from pgpkg.api import stage_version
+
+    # Change only function body/signature internals without adding/removing objects.
+    (staged_project / "sql" / "020_functions.sql").write_text(
+        """CREATE OR REPLACE FUNCTION sampleext.item_count()
+RETURNS bigint
+LANGUAGE sql
+AS $$
+    SELECT count(*) + 1 FROM sampleext.items;
+$$;
+"""
+    )
+    stage_version(staged_project, "unreleased")
+
+    path = generate_incremental(
+        staged_project,
+        from_version="0.2.0",
+        to_version="unreleased",
+        base_url=pg_url,
+    )
+    body = path.read_text().lower()
+    assert "create or replace function sampleext.item_count()" in body
+    assert "count(*) + 1" in body
+
+
+def test_makemigration_includes_body_only_procedure_changes(
+    staged_project: Path,
+    pg_url: str,
+):
+    from pgpkg.api import stage_version
+
+    (staged_project / "sql" / "020_functions.sql").write_text(
+        """CREATE OR REPLACE PROCEDURE sampleext.refresh_items()
+LANGUAGE plpgsql
+AS $$
+BEGIN
+    PERFORM count(*) FROM sampleext.items;
+END;
+$$;
+"""
+    )
+    stage_version(staged_project, "0.2.0")
+
+    (staged_project / "sql" / "020_functions.sql").write_text(
+        """CREATE OR REPLACE PROCEDURE sampleext.refresh_items()
+LANGUAGE plpgsql
+AS $$
+BEGIN
+    PERFORM count(*) + 1 FROM sampleext.items;
+END;
+$$;
+"""
+    )
+    stage_version(staged_project, "unreleased")
+
+    path = generate_incremental(
+        staged_project,
+        from_version="0.2.0",
+        to_version="unreleased",
+        base_url=pg_url,
+    )
+    body = path.read_text().lower()
+    assert "create or replace procedure sampleext.refresh_items()" in body
+    assert "count(*) + 1" in body

tests/unit/test_diff.py

@@ -0,0 +1,41 @@
+from __future__ import annotations
+
+from pgpkg.diff import _strip_unsafe_routine_drops
+
+
+def test_strip_unsafe_routine_drops_keeps_needed_drop() -> None:
+    diff_sql = "\n".join(
+        [
+            'drop function if exists "sampleext"."f"(integer);',
+            'drop function if exists "sampleext"."g"(text);',
+            "create table sampleext.t(id int);",
+        ]
+    )
+
+    target_sigs = {("function", "sampleext.f(integer)")}
+
+    out = _strip_unsafe_routine_drops(diff_sql, target_sigs)
+
+    assert 'drop function if exists "sampleext"."f"(integer);' not in out
+    assert 'drop function if exists "sampleext"."g"(text);' in out
+    assert "create table sampleext.t(id int);" in out
+
+
+def test_strip_unsafe_routine_drops_keeps_drop_when_kind_changes() -> None:
+    diff_sql = 'drop function if exists "sampleext"."f"(integer);'
+
+    target_sigs = {("procedure", "sampleext.f(integer)")}
+
+    out = _strip_unsafe_routine_drops(diff_sql, target_sigs)
+
+    assert 'drop function if exists "sampleext"."f"(integer);' in out
+
+
+def test_strip_unsafe_routine_drops_keeps_drop_for_case_sensitive_name() -> None:
+    diff_sql = 'drop function if exists "sampleext"."camelcase"(integer);'
+
+    target_sigs = {("function", "sampleext.CamelCase(integer)")}
+
+    out = _strip_unsafe_routine_drops(diff_sql, target_sigs)
+
+    assert 'drop function if exists "sampleext"."camelcase"(integer);' in out