Merge pull request #24 from TemoaProject/develop

Author: ParticularlyPythonicBS

src/datamanager/__main__.py

@@ -288,18 +288,23 @@ def _run_prepare_logic(ctx: typer.Context, name: str, file: Path) -> None:
             old_path = Path(tempdir) / "prev.sqlite"
             # Download from the PRODUCTION bucket
             core.download_from_r2(client, latest_version["r2_object_key"], old_path)
-            diff_text = core.generate_sql_diff(old_path, file)
-
-        if diff_text.count("\n") <= settings.max_diff_lines:
-            diff_filename = f"diff-{latest_version['version']}-to-{new_version}.diff"
-            # Store diffs in a dedicated top-level directory
-            diff_git_path = Path("diffs") / name / diff_filename
-            diff_git_path.parent.mkdir(parents=True, exist_ok=True)
-            diff_git_path.write_text(diff_text)
-            subprocess.run(["git", "add", str(diff_git_path)])
-            console.print(f"📝  Diff stored in Git at: [green]{diff_git_path}[/]")
+
+        full_diff, summary = core.generate_sql_diff(old_path, file)
+
+        if full_diff.count("\n") <= settings.max_diff_lines:
+            to_save = summary + full_diff
+            msg = "📝  Full diff stored in Git at:"
         else:
-            console.print("📝  Diff is too large – omitted from Git.")
+            to_save = summary
+            msg = "📝  Full diff too large; summary stored in Git at:"
+
+        diff_filename = f"diff-{latest_version['version']}-to-{new_version}.diff"
+        diff_git_path = Path("diffs") / name / diff_filename
+        diff_git_path.parent.mkdir(parents=True, exist_ok=True)
+        diff_git_path.write_text(to_save)
+        subprocess.run(["git", "add", str(diff_git_path)])
+
+        console.print(f"{msg} [green]{diff_git_path}[/]")
 
         new_entry = {
             "version": new_version,

src/datamanager/core.py

@@ -5,7 +5,6 @@
 import shutil
 import sqlite3
 import subprocess
-import tempfile
 from pathlib import Path, PurePath
 import os
 import uuid
@@ -127,40 +126,37 @@ def pull_and_verify(object_key: str, expected_hash: str, output_path: Path) -> b
         return False
 
 
-def generate_sql_diff(old_file: Path, new_file: Path) -> str:
+def generate_sql_diff(old_file: Path, new_file: Path) -> tuple[str, str]:
     """
-    Return a unified diff of the SQL schema/data between two SQLite files.
+    Return (full_diff, summary) between two SQLite files.
 
-    - If both sqlite cli and diff are available, use them (fast path).
-    - Otherwise fall back to std-lib `sqlite3` + `difflib` (pure-Python).
+    - If `sqldiff` CLI is available, use that for both full and summary.
+    - Otherwise fall back to sqlite3 + difflib, and synthesize a summary.
     """
 
-    has_cli = shutil.which("sqlite3") and shutil.which("diff")
-
-    if has_cli:
-        with tempfile.TemporaryDirectory() as tmp:
-            old_dump, new_dump = Path(tmp) / "old.sql", Path(tmp) / "new.sql"
-            subprocess.run(
-                ["sqlite3", str(old_file), ".dump"],
-                text=True,
-                check=True,
-                stdout=old_dump.open("w"),
-            )
-            subprocess.run(
-                ["sqlite3", str(new_file), ".dump"],
-                text=True,
-                check=True,
-                stdout=new_dump.open("w"),
-            )
-            proc = subprocess.run(
-                ["diff", "-u", str(old_dump), str(new_dump)],
-                text=True,
-                capture_output=True,
-            )
-            return proc.stdout
+    # Try sqldiff CLI first
+    if shutil.which("sqldiff"):
+        # Full diff
+        proc_full = subprocess.run(
+            ["sqldiff", str(old_file), str(new_file)],
+            text=True,
+            capture_output=True,
+            check=True,
+        )
+        full_diff = proc_full.stdout
+
+        # Summary (sqldiff --summary)
+        proc_sum = subprocess.run(
+            ["sqldiff", "--summary", str(old_file), str(new_file)],
+            text=True,
+            capture_output=True,
+            check=True,
+        )
+        summary = proc_sum.stdout
 
-    # pure-Python fallback
+        return full_diff, summary
 
+    # Pure-Python fallback: dump both DBs and diff
     def _dump(db: Path) -> str:
         buf = io.StringIO()
         con = sqlite3.connect(db)
@@ -169,14 +165,31 @@ def _dump(db: Path) -> str:
         con.close()
         return buf.getvalue()
 
-    old_sql, new_sql = _dump(old_file), _dump(new_file)
+    old_sql = _dump(old_file)
+    new_sql = _dump(new_file)
+
     diff_iter = difflib.unified_diff(
         old_sql.splitlines(keepends=True),
         new_sql.splitlines(keepends=True),
         fromfile=str(PurePath(old_file).name),
         tofile=str(PurePath(new_file).name),
     )
-    return "".join(diff_iter)
+    full_diff = "".join(diff_iter)
+
+    # Synthesize a summary: count ADDs and DELs
+    adds = sum(
+        1
+        for ln in full_diff.splitlines()
+        if ln.startswith("+") and not ln.startswith("+++")
+    )
+    dels = sum(
+        1
+        for ln in full_diff.splitlines()
+        if ln.startswith("-") and not ln.startswith("---")
+    )
+    summary = f"# summary: {adds} additions, {dels} deletions\n"
+
+    return full_diff, summary
 
 
 def delete_from_r2(client: S3Client, object_key: str) -> None:

tests/test_core.py

@@ -40,9 +40,21 @@ def test_generate_sql_diff(tmp_path: Path) -> None:
     con.commit()
     con.close()
 
-    diff = core.generate_sql_diff(old_db_path, new_db_path)
-    assert "-INSERT INTO users VALUES(1,'Alice');" in diff
-    assert "+INSERT INTO users VALUES(2,'Bob');" in diff
+    full_diff, summary = core.generate_sql_diff(old_db_path, new_db_path)
+
+    # We should see evidence of the change in full_diff:
+    #   - either an UPDATE statement from sqldiff,
+    #   - or a "+INSERT ..." / "-INSERT ..." style from the fallback.
+    assert "Bob" in full_diff, f"full_diff did not mention Bob:\n{full_diff}"
+    assert (
+        "UPDATE users SET" in full_diff
+        or "+INSERT INTO" in full_diff
+        or "-INSERT INTO" in full_diff
+    ), f"unexpected diff format:\n{full_diff}"
+
+    # And summary should be a non‐empty string
+    assert isinstance(summary, str)
+    assert summary.strip(), "Summary should not be empty"
 
 
 def test_r2_interactions(mocker: MockerFixture, tmp_path: Path) -> None:

tests/test_main.py

@@ -37,55 +37,77 @@ def test_prepare_for_create_success(test_repo: Path, mocker: MockerFixture) -> N
 def test_prepare_for_update_with_small_diff(
     test_repo: Path, mocker: MockerFixture
 ) -> None:
-    """Test an update that generates and stores a small diff."""
+    """Test an update that generates and stores both summary+full diff."""
     os.chdir(test_repo)
     mock_r2_client = mocker.patch("datamanager.core.get_r2_client").return_value
-    # Configure head_object to prevent TypeError in the download progress bar
     mock_r2_client.head_object.return_value = {"ContentLength": 1024}
     mocker.patch("datamanager.core.upload_to_staging")
     mocker.patch("datamanager.core.download_from_r2")
-    mocker.patch("datamanager.core.generate_sql_diff", return_value="--- a\n+++ b")
+
+    # Prepare a fake summary and full diff
+    fake_summary = "# summary: 1 add, 1 del\n"
+    fake_full = "--- a\n+++ b\n-foo\n+bar\n"
+    mocker.patch(
+        "datamanager.core.generate_sql_diff",
+        return_value=(fake_full, fake_summary),
+    )
 
     v3_file = test_repo / "v3_data.sqlite"
     v3_file.write_text("this is v3")
 
     result = runner.invoke(app, ["prepare", "core-dataset.sqlite", str(v3_file)])
-
     assert result.exit_code == 0, result.stdout
-    assert "Diff stored in Git" in result.stdout
+    assert "Full diff stored in Git" in result.stdout
 
-    expected_diff_path = Path("diffs/core-dataset.sqlite/diff-v2-to-v3.diff")
-    assert expected_diff_path.exists()
+    expected = Path("diffs/core-dataset.sqlite/diff-v2-to-v3.diff")
+    assert expected.exists()
 
-    dataset = manifest.get_dataset("core-dataset.sqlite")
-    assert dataset is not None, "Dataset should not be None after preparation."
-    assert dataset["history"][0]["diffFromPrevious"] == str(expected_diff_path)
+    # Now verify the file contains summary first, then full diff
+    content = expected.read_text()
+    assert content == fake_summary + fake_full
+
+    ds = manifest.get_dataset("core-dataset.sqlite")
+    assert ds is not None
+    assert ds["history"][0]["diffFromPrevious"] == str(expected)
 
 
 def test_prepare_for_update_with_large_diff(
     test_repo: Path, mocker: MockerFixture
 ) -> None:
-    """Test an update where the diff is too large and is omitted."""
+    """Test an update where the full diff is too large and only summary is stored."""
     os.chdir(test_repo)
     mock_r2_client = mocker.patch("datamanager.core.get_r2_client").return_value
     mock_r2_client.head_object.return_value = {"ContentLength": 1024}
     mocker.patch("datamanager.core.upload_to_staging")
     mocker.patch("datamanager.core.download_from_r2")
-    # Make the diff larger than the default MAX_DIFF_LINES
-    large_diff = "line\n" * (settings.max_diff_lines + 1)
-    mocker.patch("datamanager.core.generate_sql_diff", return_value=large_diff)
+    # Make the full diff larger than the default limit, but still provide a summary
+    large_full = "line\n" * (settings.max_diff_lines + 1)
+    small_summary = "# summary: huge diff, see details in PR\n"
+    mocker.patch(
+        "datamanager.core.generate_sql_diff",
+        return_value=(large_full, small_summary),
+    )
 
     v3_file = test_repo / "v3_data.sqlite"
     v3_file.write_text("this is v3")
 
     result = runner.invoke(app, ["prepare", "core-dataset.sqlite", str(v3_file)])
-
     assert result.exit_code == 0, result.stdout
-    assert "Diff is too large" in result.stdout
 
+    # We should see that the full diff was too large but a summary was stored
+    assert "too large" in result.stdout.lower()
+
+    expected_diff_path = Path("diffs/core-dataset.sqlite/diff-v2-to-v3.diff")
+    assert expected_diff_path.exists(), "Summary file should have been written"
+
+    # The manifest should record the path
     dataset = manifest.get_dataset("core-dataset.sqlite")
-    assert dataset is not None, "Dataset should not be None after preparation."
-    assert dataset["history"][0]["diffFromPrevious"] is None
+    assert dataset is not None
+    assert dataset["history"][0]["diffFromPrevious"] == str(expected_diff_path)
+
+    # And the contents of that file should be exactly our summary
+    content = expected_diff_path.read_text()
+    assert content == small_summary
 
 
 def test_prepare_no_changes(test_repo: Path, mocker: MockerFixture) -> None: