From 083944f0fdee1b4a807fbe4b814d3d3f5e043993 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 27 Jul 2026 23:12:48 -0400 Subject: [PATCH] fix(snippets): backfill must also catch JSON null, not just SQL NULL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integration lane caught it (run 2984): the backfill reported 0 rows to fill and left `data` unset. `IS NULL` was the whole predicate, but a JSONB column has two empty states. SQLAlchemy's JSON types default to `none_as_null=False`, so assigning Python `None` persists the JSON encoding of null — `IS NULL` walks straight past it. Migration 0070 left genuine SQL NULLs, so the product path was right; the test was constructing the wrong shape with `data=None`. Fixed both ways, because both states mean "no usable mirror": - predicate is now `data IS NULL OR jsonb_typeof(data) = 'null'`; - the legacy-row test OMITS `data` (a real SQL NULL, 0070's actual shape), and a second test covers the JSON-null shape and asserts the premise with `jsonb_typeof` rather than assuming it. Refs #2083. --- src/scribe/services/snippets.py | 12 ++++++-- tests/test_integration_snippet_locations.py | 31 ++++++++++++++++----- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/scribe/services/snippets.py b/src/scribe/services/snippets.py index 66d4f1a..30d1b4b 100644 --- a/src/scribe/services/snippets.py +++ b/src/scribe/services/snippets.py @@ -32,7 +32,7 @@ import asyncio import logging import re -from sqlalchemy import select +from sqlalchemy import func, or_, select from scribe.models import async_session from scribe.models.note import Note @@ -386,8 +386,16 @@ async def backfill_snippet_data(*, batch: int = 500) -> int: filled row is skipped forever after, and a snippet with no structured fields at all settles at `{}` rather than staying NULL and being re-scanned. Trashed rows are included so a later restore comes back queryable. + + "Unfilled" means SQL NULL *or* JSON `null` — two different states in a JSONB + column, and only the first is what migration 0070 left behind. SQLAlchemy's + JSON types default to ``none_as_null=False``, so assigning Python ``None`` to + this column persists the JSON encoding of null rather than SQL NULL; an + ``IS NULL`` test alone walks straight past such a row and reports nothing to + do. Both mean "no usable mirror", so both are filled. """ filled = 0 + unfilled = or_(Note.data.is_(None), func.jsonb_typeof(Note.data) == "null") async with async_session() as session: while True: rows = list( @@ -395,7 +403,7 @@ async def backfill_snippet_data(*, batch: int = 500) -> int: await session.execute( select(Note) .where(Note.note_type == SNIPPET_NOTE_TYPE) - .where(Note.data.is_(None)) + .where(unfilled) .limit(batch) ) ) diff --git a/tests/test_integration_snippet_locations.py b/tests/test_integration_snippet_locations.py index b919cb6..d430538 100644 --- a/tests/test_integration_snippet_locations.py +++ b/tests/test_integration_snippet_locations.py @@ -17,7 +17,7 @@ from unittest.mock import AsyncMock, patch import pytest import pytest_asyncio -from sqlalchemy import delete, select +from sqlalchemy import delete, func, select from scribe.models import async_session, engine from scribe.models.note import Note @@ -241,7 +241,13 @@ async def test_sql_and_python_dialects_agree_on_the_same_rows(seeded): async def test_backfill_makes_a_pre_0070_snippet_findable_by_location(seeded): """A snippet stored before migration 0070 has no `data`, so the location query can't see it. Unfilled, it reads as "nothing recorded here" — the failure the - backfill exists to prevent.""" + backfill exists to prevent. + + `data` is OMITTED from the constructor on purpose: that leaves a genuine SQL + NULL, which is the shape migration 0070 actually left behind. Passing + `data=None` would store the JSON encoding of null instead (SQLAlchemy JSON + defaults to none_as_null=False) — a different state, covered by the next test. + """ user_id, _ids = seeded locations = [_loc("Legacy", "old/path/y.py", "legacy_helper")] async with async_session() as s: @@ -251,7 +257,6 @@ async def test_backfill_makes_a_pre_0070_snippet_findable_by_location(seeded): title="legacy — recorded before the data column existed", body=compose_body(code="return 2", language="python", locations=locations), tags=["python", "snippet"], - data=None, ) s.add(old) await s.commit() @@ -274,9 +279,11 @@ async def test_backfill_makes_a_pre_0070_snippet_findable_by_location(seeded): @pytest.mark.asyncio -async def test_backfill_is_idempotent_and_settles_a_fieldless_snippet(seeded): - """Second run fills nothing, and a snippet with no structured fields lands at - `{}` rather than staying NULL and being rescanned on every boot.""" +async def test_backfill_fills_a_json_null_and_is_idempotent(seeded): + """The OTHER unfilled shape: `data=None` persists JSON `null`, not SQL NULL + (SQLAlchemy JSON defaults to none_as_null=False), so an `IS NULL` test alone + would walk past this row. Also pins idempotence — a fieldless snippet settles + at `{}` instead of staying unfilled and being rescanned on every boot.""" user_id, _ids = seeded async with async_session() as s: bare = Note( @@ -291,7 +298,17 @@ async def test_backfill_is_idempotent_and_settles_a_fieldless_snippet(seeded): await s.commit() bare_id = bare.id - await backfill_snippet_data() + # Confirm the premise rather than assuming it: this row really is JSON null, + # so the assertion below is testing the second arm of the predicate. + async with async_session() as s: + kind = ( + await s.execute( + select(func.jsonb_typeof(Note.data)).where(Note.id == bare_id) + ) + ).scalar_one() + assert kind == "null" + + assert await backfill_snippet_data() >= 1 assert await backfill_snippet_data() == 0 async with async_session() as s: