Read a NULL notification flag as off instead of dropping every provider (issue #2827)

Adding on_stock_reorder_alert and on_stock_break_alert to the provider
schema made them required on the way out as well as in: the response model
inherits the write model. Every on_* column on notification_providers is
nullable with no server default, and where the table was created from
Base.metadata before run_migrations, the ALTER ... DEFAULT false that
introduced those columns was swallowed as a duplicate and never backfilled
existing rows. Those NULLs were harmless until the flags were read, at
which point the row failed validation -- and a list is validated as a
whole, so one row took every provider with it. The route returned 500 and
the UI rendered an empty list, so configured providers looked deleted.

Backfill them to off, which is what the sender already assumed: it selects
providers with IS TRUE, so a NULL flag never sent anything. A NULL flag now
also reads as off rather than failing the response, across all of them, so
the next flag added to this schema cannot repeat it. Writes are unchanged.

backend/app/core/database.py

@@ -3939,6 +3939,34 @@ async def run_migrations(conn):
             conn, "ALTER TABLE notification_providers ADD COLUMN on_stock_break_alert BOOLEAN DEFAULT false"
         )
 
+    # Backfill the two flags above. The DEFAULT on those ALTERs only reaches
+    # existing rows when the ALTER is the statement that adds the column -- and
+    # on an install whose notification_providers table was (re)created from
+    # Base.metadata, create_all() had already added them by the time migrations
+    # ran, so _safe_execute swallowed the ALTER as a duplicate column and every
+    # pre-existing row kept NULL. Harmless while nothing read the flags; a 500
+    # on the whole provider list once #2827 declared them on the response
+    # schema, because pydantic will not accept None for a bool.
+    #
+    # false matches both the intent of the DEFAULT above and the behaviour the
+    # rows already have: _get_providers_for_event filters on `.is_(True)`, so a
+    # NULL flag never sent anything. Idempotent -- the WHERE matches nothing on
+    # the second run.
+    async with conn.begin_nested():
+        stock_backfill = await conn.execute(
+            text(
+                "UPDATE notification_providers SET on_stock_reorder_alert = :off WHERE on_stock_reorder_alert IS NULL"
+            ),
+            {"off": False},
+        )
+        stock_backfill_break = await conn.execute(
+            text("UPDATE notification_providers SET on_stock_break_alert = :off WHERE on_stock_break_alert IS NULL"),
+            {"off": False},
+        )
+    repaired = (stock_backfill.rowcount or 0) + (stock_backfill_break.rowcount or 0)
+    if repaired:
+        logger.info("Backfilled %s NULL inventory stock alert flag(s) on notification_providers", repaired)
+
     # Migration: Heal orphan auth-related rows left behind by user-delete
     # on SQLite. user_oidc_links, user_totp, user_otp_codes (introduced in
     # PR #933) and long_lived_tokens (PR #1108) all declare ON DELETE

backend/app/schemas/notification.py

@@ -3,7 +3,7 @@
 from datetime import datetime
 from typing import Any
 
-from pydantic import BaseModel, Field, field_validator
+from pydantic import BaseModel, Field, field_validator, model_validator
 
 from backend.app.core.compat import StrEnum
 
@@ -223,6 +223,42 @@ class NotificationProviderUpdate(BaseModel):
 class NotificationProviderResponse(NotificationProviderBase):
     """Schema for notification provider API responses."""
 
+    @model_validator(mode="before")
+    @classmethod
+    def _null_event_flags_read_as_off(cls, data: Any) -> Any:
+        """Read a NULL event flag as off instead of failing the whole response.
+
+        Every on_* column on notification_providers is nullable with no server
+        default -- the values come from the ORM at INSERT time. A row created
+        before a flag's column existed keeps NULL there forever unless a
+        migration backfills it, and one that did not (the column was created by
+        Base.metadata before run_migrations, so the ALTER ... DEFAULT false was
+        swallowed as a duplicate) leaves NULLs behind on a live install.
+
+        Those NULLs are harmless until the flag is declared on this schema: the
+        Response inherits the write model, so `bool` is then required on the way
+        out, pydantic rejects None, and every provider row fails at once -- the
+        list route 500s and the UI renders an empty list, which reads to the user
+        as "my providers are gone". That is exactly what shipped in #2827.
+
+        Off is not a guess: _get_providers_for_event selects on `.is_(True)`, so
+        the sender already skips a NULL flag. This makes the read agree with the
+        behaviour the row already has, rather than with the field's declared
+        default -- some of which are True, and none of which should switch a
+        notification on as a side effect of repairing a legacy row.
+
+        Writes are untouched: Create and Update inherit from the base, not here,
+        so a payload sending null for a flag is still a 422.
+        """
+        # Every route returns _provider_to_dict(); anything else (an ORM object
+        # via from_attributes) is passed through for pydantic to handle.
+        if not isinstance(data, dict):
+            return data
+        flags = [name for name, f in cls.model_fields.items() if f.annotation is bool]
+        if any(data.get(name, False) is None for name in flags):
+            data = {**data, **{name: False for name in flags if data.get(name, False) is None}}
+        return data
+
     id: int
     last_success: datetime | None = None
     last_error: str | None = None

backend/tests/integration/test_notifications_api.py

@@ -5,6 +5,7 @@ Tests the full request/response cycle for /api/v1/notifications/ endpoints.
 
 import pytest
 from httpx import AsyncClient
+from sqlalchemy import text
 
 
 class TestNotificationsAPI:
@@ -38,6 +39,52 @@ class TestNotificationsAPI:
         assert len(data) >= 1
         assert any(p["name"] == "Test Provider" for p in data)
 
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_a_row_with_null_event_flags_is_still_listable(
+        self, async_client: AsyncClient, notification_provider_factory, db_session
+    ):
+        """A legacy row whose flag columns were never backfilled must not 500 the list.
+
+        Every on_* column is nullable with no server default, so a row created
+        before a flag existed keeps NULL there until a migration backfills it --
+        and #1184's ALTER ... DEFAULT false silently did not, on any install
+        where create_all() had already added the column. Declaring those flags
+        on the response schema in #2827 turned those NULLs into a hard failure:
+        pydantic rejects None for a bool, so every provider row failed at once
+        and the list came back empty to the UI.
+
+        Written against the two flags that actually broke, but the whole set is
+        checked -- the next flag added to the schema has the same exposure.
+        """
+        provider = await notification_provider_factory(name="Legacy Provider")
+
+        flags = ["on_stock_reorder_alert", "on_stock_break_alert"]
+        await db_session.execute(
+            text(f"UPDATE notification_providers SET {', '.join(f'{f} = NULL' for f in flags)} WHERE id = :id"),
+            {"id": provider.id},
+        )
+        await db_session.commit()
+
+        stored = await db_session.execute(
+            text(f"SELECT {', '.join(flags)} FROM notification_providers WHERE id = :id"), {"id": provider.id}
+        )
+        assert all(value is None for value in stored.one()), "row under test must actually hold NULLs"
+
+        response = await async_client.get("/api/v1/notifications/")
+
+        assert response.status_code == 200
+        listed = next(p for p in response.json() if p["name"] == "Legacy Provider")
+        # Off, not the field default: the sender selects on `.is_(True)`, so a
+        # NULL flag never sent anything, and repairing the read must not switch
+        # a notification on.
+        assert all(listed[flag] is False for flag in flags)
+
+        # The single-provider route reads through the same schema.
+        single = await async_client.get(f"/api/v1/notifications/{provider.id}")
+        assert single.status_code == 200
+        assert all(single.json()[flag] is False for flag in flags)
+
     # ========================================================================
     # Create endpoints
     # ========================================================================