fix(snippets): backfill must also catch JSON null, not just SQL NULL
CI & Build / Python lint (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 45s
CI & Build / integration (push) Successful in 36s
CI & Build / Python tests (push) Successful in 1m5s
CI & Build / Build & push image (push) Successful in 1m11s
CI & Build / Python lint (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 45s
CI & Build / integration (push) Successful in 36s
CI & Build / Python tests (push) Successful in 1m5s
CI & Build / Build & push image (push) Successful in 1m11s
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.
This commit is contained in:
@@ -32,7 +32,7 @@ import asyncio
|
|||||||
import logging
|
import logging
|
||||||
import re
|
import re
|
||||||
|
|
||||||
from sqlalchemy import select
|
from sqlalchemy import func, or_, select
|
||||||
|
|
||||||
from scribe.models import async_session
|
from scribe.models import async_session
|
||||||
from scribe.models.note import Note
|
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
|
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
|
at all settles at `{}` rather than staying NULL and being re-scanned. Trashed
|
||||||
rows are included so a later restore comes back queryable.
|
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
|
filled = 0
|
||||||
|
unfilled = or_(Note.data.is_(None), func.jsonb_typeof(Note.data) == "null")
|
||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
while True:
|
while True:
|
||||||
rows = list(
|
rows = list(
|
||||||
@@ -395,7 +403,7 @@ async def backfill_snippet_data(*, batch: int = 500) -> int:
|
|||||||
await session.execute(
|
await session.execute(
|
||||||
select(Note)
|
select(Note)
|
||||||
.where(Note.note_type == SNIPPET_NOTE_TYPE)
|
.where(Note.note_type == SNIPPET_NOTE_TYPE)
|
||||||
.where(Note.data.is_(None))
|
.where(unfilled)
|
||||||
.limit(batch)
|
.limit(batch)
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ from unittest.mock import AsyncMock, patch
|
|||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
import pytest_asyncio
|
import pytest_asyncio
|
||||||
from sqlalchemy import delete, select
|
from sqlalchemy import delete, func, select
|
||||||
|
|
||||||
from scribe.models import async_session, engine
|
from scribe.models import async_session, engine
|
||||||
from scribe.models.note import Note
|
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):
|
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
|
"""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
|
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
|
user_id, _ids = seeded
|
||||||
locations = [_loc("Legacy", "old/path/y.py", "legacy_helper")]
|
locations = [_loc("Legacy", "old/path/y.py", "legacy_helper")]
|
||||||
async with async_session() as s:
|
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",
|
title="legacy — recorded before the data column existed",
|
||||||
body=compose_body(code="return 2", language="python", locations=locations),
|
body=compose_body(code="return 2", language="python", locations=locations),
|
||||||
tags=["python", "snippet"],
|
tags=["python", "snippet"],
|
||||||
data=None,
|
|
||||||
)
|
)
|
||||||
s.add(old)
|
s.add(old)
|
||||||
await s.commit()
|
await s.commit()
|
||||||
@@ -274,9 +279,11 @@ async def test_backfill_makes_a_pre_0070_snippet_findable_by_location(seeded):
|
|||||||
|
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_backfill_is_idempotent_and_settles_a_fieldless_snippet(seeded):
|
async def test_backfill_fills_a_json_null_and_is_idempotent(seeded):
|
||||||
"""Second run fills nothing, and a snippet with no structured fields lands at
|
"""The OTHER unfilled shape: `data=None` persists JSON `null`, not SQL NULL
|
||||||
`{}` rather than staying NULL and being rescanned on every boot."""
|
(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
|
user_id, _ids = seeded
|
||||||
async with async_session() as s:
|
async with async_session() as s:
|
||||||
bare = Note(
|
bare = Note(
|
||||||
@@ -291,7 +298,17 @@ async def test_backfill_is_idempotent_and_settles_a_fieldless_snippet(seeded):
|
|||||||
await s.commit()
|
await s.commit()
|
||||||
bare_id = bare.id
|
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
|
assert await backfill_snippet_data() == 0
|
||||||
|
|
||||||
async with async_session() as s:
|
async with async_session() as s:
|
||||||
|
|||||||
Reference in New Issue
Block a user