Explorar el Código

fix(backup): preserve NOT NULL/DEFAULT/FK/UNIQUE in Postgres→SQLite backup (#2526)

On a PostgreSQL install, create_backup_zip() exports a portable SQLite copy
so backups move between engines. It rebuilt each table with only column name
+ type + PK, dropping NOT NULL, server_default/DEFAULT, foreign keys, and
unique constraints. Restore onto SQLite page-copies that schema straight onto
the live database, and post-restore init_db() can't repair it (create_all is
CREATE TABLE IF NOT EXISTS). So server_default columns like
spoolbuddy_devices.created_at (server_default=func.now()) ended up with no
DEFAULT: SQLAlchemy omits them on INSERT, the DB wrote NULL, and the next read
500'd on Pydantic validation. Every server_default column was exposed the same
way; the FK/unique loss followed from the same simplified CREATE TABLE.

Build the portable schema with Base.metadata.create_all() against a SQLite
engine instead of the hand-rolled loop, so it emits the exact DDL a native
SQLite install gets (NOT NULL, DEFAULT func.now() -> CURRENT_TIMESTAMP, FKs,
unique constraints, indexes). The data-export insert path is unchanged, and
the #1333 OIDC-icon guard is preserved automatically (LargeBinary -> BLOB),
which lets the now-redundant _sqlalchemy_type_to_sqlite_type() helper be
removed. Fixes newly-created backups; a backup from an older build still
carries the degraded schema, so re-take backups after upgrading.

Replace the #1333 type-mapping unit tests with three that inspect the real
backup schema via metadata.create_all + PRAGMA table_info: icon_data is BLOB,
created_at keeps its CURRENT_TIMESTAMP DEFAULT, a NOT NULL non-PK column stays
NOT NULL.
maziggy hace 1 mes
padre
commit
9e7f6cafd9

La diferencia del archivo ha sido suprimido porque es demasiado grande
+ 0 - 0
CHANGELOG.md


+ 20 - 40
backend/app/api/routes/settings.py

@@ -35,32 +35,6 @@ _SENSITIVE_FIELDS_FOR_API_KEY = (
 )
 )
 
 
 
 
-def _sqlalchemy_type_to_sqlite_type(type_repr: str) -> str:
-    """Map a SQLAlchemy column type's ``str()`` to a SQLite-native column type.
-
-    Used by ``create_backup_zip`` to reconstruct a portable SQLite database
-    file from PostgreSQL data. Falling through to TEXT for binary columns
-    corrupts non-UTF8 bytes — the BLOB branch is the #1333 regression guard
-    for OIDC icon BLOBs.
-
-    Extracted as a pure helper so it can be unit-tested without spinning up
-    the full FastAPI app + backup pipeline.
-    """
-    type_str = type_repr.upper()
-    if "INT" in type_str:
-        return "INTEGER"
-    if "FLOAT" in type_str or "REAL" in type_str or "NUMERIC" in type_str:
-        return "REAL"
-    if "BOOL" in type_str:
-        return "BOOLEAN"
-    if "BLOB" in type_str or "BYTEA" in type_str or "BINARY" in type_str:
-        # OIDC icon BLOB column (#1333) — without this branch the column
-        # was created as TEXT and non-UTF8 bytes were corrupted during the
-        # PG→SQLite-ZIP backup round trip.
-        return "BLOB"
-    return "TEXT"
-
-
 async def get_setting(db: AsyncSession, key: str) -> str | None:
 async def get_setting(db: AsyncSession, key: str) -> str | None:
     """Get a single setting value by key."""
     """Get a single setting value by key."""
     result = await db.execute(select(Settings).where(Settings.key == key))
     result = await db.execute(select(Settings).where(Settings.key == key))
@@ -582,25 +556,31 @@ async def create_backup_zip(output_path: Path | None = None) -> tuple[Path, str]
             import json
             import json
             import sqlite3
             import sqlite3
 
 
+            from sqlalchemy import create_engine as create_sync_engine
+
             from backend.app.core.database import Base, engine
             from backend.app.core.database import Base, engine
 
 
             backup_db_path = temp_path / "bambuddy.db"
             backup_db_path = temp_path / "bambuddy.db"
-            dst = sqlite3.connect(str(backup_db_path))
             metadata = Base.metadata
             metadata = Base.metadata
 
 
-            # Create tables in SQLite backup (simplified — just column names and types)
-            for table in metadata.sorted_tables:
-                cols = []
-                pk_cols = [col.name for col in table.columns if col.primary_key]
-                for col in table.columns:
-                    col_type = _sqlalchemy_type_to_sqlite_type(str(col.type))
-                    # Only inline PRIMARY KEY for single-column PKs
-                    pk = " PRIMARY KEY" if col.primary_key and len(pk_cols) == 1 else ""
-                    cols.append(f"{col.name} {col_type}{pk}")
-                # Add composite primary key constraint if needed
-                if len(pk_cols) > 1:
-                    cols.append(f"PRIMARY KEY ({', '.join(pk_cols)})")
-                dst.execute(f"CREATE TABLE IF NOT EXISTS {table.name} ({', '.join(cols)})")  # noqa: S608
+            # Build the portable SQLite schema with SQLAlchemy's own DDL rather
+            # than a hand-rolled CREATE TABLE. metadata.create_all() emits the
+            # exact schema a native SQLite install gets — NOT NULL, DEFAULT
+            # (server_default=func.now() → CURRENT_TIMESTAMP), foreign keys,
+            # unique constraints and indexes. The previous name+type-only
+            # rebuild dropped all of these, so a Postgres→SQLite restore left
+            # server_default columns (e.g. spoolbuddy_devices.created_at) with
+            # no DEFAULT — SQLAlchemy omits such columns on INSERT and the DB
+            # then wrote NULL, which 500'd on the next read (#2526). Using the
+            # real DDL also keeps the #1333 BLOB guard: LargeBinary still
+            # renders as BLOB, so OIDC icon bytes survive the round trip.
+            schema_engine = create_sync_engine(f"sqlite:///{backup_db_path}")
+            try:
+                metadata.create_all(schema_engine)
+            finally:
+                schema_engine.dispose()
+
+            dst = sqlite3.connect(str(backup_db_path))
 
 
             # Export data from Postgres to SQLite
             # Export data from Postgres to SQLite
             async with engine.connect() as conn:
             async with engine.connect() as conn:

+ 70 - 39
backend/tests/integration/test_oidc_icon_blob_roundtrip.py

@@ -1,54 +1,85 @@
-"""Type-mapping coverage for the OIDC icon BLOB column (#1333).
-
-Bambuddy's ``create_backup_zip`` rebuilds the SQLite backup schema from
-``Base.metadata`` when the source database is PostgreSQL. The column-type
-mapping previously fell through to ``TEXT`` for any unknown SQLAlchemy
-type — including ``LargeBinary`` / ``BYTEA`` — which corrupts non-UTF8
-icon bytes during the PG → SQLite-ZIP round trip.
-
-These tests exercise the extracted ``_sqlalchemy_type_to_sqlite_type``
-helper directly so the regression guard doesn't depend on a full backup
-pipeline. The SQLite source path is just ``shutil.copy2`` of the live
-.db file and is therefore unaffected by the type mapping.
+"""Backup-schema fidelity for the PG→SQLite portable export (#1333, #2526).
+
+Bambuddy's ``create_backup_zip`` rebuilds the SQLite backup schema when the
+source database is PostgreSQL. It now uses ``Base.metadata.create_all()``
+against a SQLite engine — the same DDL a native SQLite install gets — rather
+than a hand-rolled ``name + type`` CREATE TABLE. The old rebuild dropped two
+things that these tests pin:
+
+* ``LargeBinary`` fell through to ``TEXT``, corrupting non-UTF8 OIDC icon
+  bytes during the round trip (#1333). ``create_all`` renders it as ``BLOB``.
+* ``NOT NULL`` / ``DEFAULT`` / FK / ``UNIQUE`` were all dropped, so a
+  Postgres→SQLite restore left ``server_default`` columns (e.g.
+  ``spoolbuddy_devices.created_at``) with no ``DEFAULT`` — later inserts
+  wrote ``NULL`` and 500'd on read (#2526). ``create_all`` emits the default.
+
+The SQLite *source* path is just ``shutil.copy2`` of the live .db file and is
+therefore unaffected — these guards only matter for the PostgreSQL branch.
 """
 """
 
 
 import hashlib
 import hashlib
 import sqlite3
 import sqlite3
 
 
 import pytest
 import pytest
-from sqlalchemy import Column, LargeBinary
+from sqlalchemy import create_engine
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.ext.asyncio import AsyncSession
 
 
-from backend.app.api.routes.settings import _sqlalchemy_type_to_sqlite_type
+from backend.app.core.database import Base
 from backend.tests._fixtures.oidc_icon import PNG_BYTES as _PNG_BYTES
 from backend.tests._fixtures.oidc_icon import PNG_BYTES as _PNG_BYTES
 
 
 
 
-class TestTypeMapping:
-    """Unit-level coverage of the helper that backups use for PG→SQLite."""
+def _build_backup_schema(db_path) -> dict[str, dict]:
+    """Build the portable SQLite schema exactly as create_backup_zip's
+    PostgreSQL branch does, then return ``{table: {col: PRAGMA row}}``.
 
 
-    def test_largebinary_maps_to_blob(self):
-        # Direct from a SQLAlchemy LargeBinary column — this is exactly
-        # what the create_backup_zip loop calls str() on.
-        col = Column(LargeBinary)
-        assert _sqlalchemy_type_to_sqlite_type(str(col.type)) == "BLOB"
-
-    @pytest.mark.parametrize(
-        "type_repr",
-        ["BLOB", "BYTEA", "BYTEA(1024)", "VARBINARY", "BINARY", "binary varying"],
-    )
-    def test_binary_type_strings_map_to_blob(self, type_repr):
-        assert _sqlalchemy_type_to_sqlite_type(type_repr) == "BLOB"
-
-    def test_integer_unchanged(self):
-        assert _sqlalchemy_type_to_sqlite_type("INTEGER") == "INTEGER"
-        assert _sqlalchemy_type_to_sqlite_type("BIGINT") == "INTEGER"
-
-    def test_boolean_unchanged(self):
-        assert _sqlalchemy_type_to_sqlite_type("BOOLEAN") == "BOOLEAN"
-
-    def test_unknown_falls_back_to_text(self):
-        assert _sqlalchemy_type_to_sqlite_type("VARCHAR(500)") == "TEXT"
-        assert _sqlalchemy_type_to_sqlite_type("DATETIME") == "TEXT"
+    PRAGMA table_info rows are ``(cid, name, type, notnull, dflt_value, pk)``.
+    """
+    engine = create_engine(f"sqlite:///{db_path}")
+    try:
+        Base.metadata.create_all(engine)
+    finally:
+        engine.dispose()
+
+    conn = sqlite3.connect(str(db_path))
+    try:
+        schema: dict[str, dict] = {}
+        tables = [
+            row[0]
+            for row in conn.execute("SELECT name FROM sqlite_master WHERE type='table' AND name NOT LIKE 'sqlite_%'")
+        ]
+        for table in tables:
+            schema[table] = {row[1]: row for row in conn.execute(f"PRAGMA table_info({table})")}  # noqa: S608
+        return schema
+    finally:
+        conn.close()
+
+
+class TestBackupSchemaFidelity:
+    """The real backup-schema builder (metadata.create_all on SQLite),
+    inspected via sqlite_master, keeps the constraints the old name+type
+    rebuild dropped."""
+
+    def test_icon_data_column_is_blob(self, tmp_path):
+        # #1333 — LargeBinary must render as BLOB, not TEXT, or non-UTF8
+        # OIDC icon bytes are corrupted on the PG→SQLite round trip.
+        schema = _build_backup_schema(tmp_path / "schema.db")
+        assert schema["oidc_providers"]["icon_data"][2] == "BLOB"
+
+    def test_server_default_column_keeps_default(self, tmp_path):
+        # #2526 — a server_default=func.now() column must carry a DEFAULT so
+        # inserts that omit it (SQLAlchemy does, for server-side defaults)
+        # don't write NULL after a Postgres→SQLite restore.
+        schema = _build_backup_schema(tmp_path / "schema.db")
+        created_at = schema["spoolbuddy_devices"]["created_at"]
+        assert created_at[4] is not None, "created_at lost its DEFAULT clause"
+        assert "CURRENT_TIMESTAMP" in str(created_at[4]).upper()
+
+    def test_not_null_column_keeps_not_null(self, tmp_path):
+        # #2526 — NOT NULL columns must stay NOT NULL. A single-column PK is
+        # implicitly NOT NULL, so assert on a non-PK required column.
+        schema = _build_backup_schema(tmp_path / "schema.db")
+        # notnull flag is index 3 of the PRAGMA row.
+        assert schema["spoolbuddy_devices"]["device_id"][3] == 1
 
 
 
 
 class TestSqliteBinaryRoundtrip:
 class TestSqliteBinaryRoundtrip:

Algunos archivos no se mostraron porque demasiados archivos cambiaron en este cambio