Skip to content

Commit 7510bfe

Browse files
committed
Pin the contracts the session attribute had before it became a property
Turning CalibreDB.session into a property is only safe if it keeps every contract the plain attribute had. Three were found by the suite rather than by reading the code -- lazy init for __new__-built instances, a deleter for mock.patch.object teardown, and assigning None as editbooks' recovery path -- so pin all of them plus per-thread scoping and the expire_on_commit preference. Red on main (5 of 6 fail, incl. the cross-thread one), green here.
1 parent fd34c9d commit 7510bfe

1 file changed

Lines changed: 160 additions & 0 deletions

File tree

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
# Calibre-Web Automated – fork of Calibre-Web
2+
# Copyright (C) 2024-2026 Calibre-Web-NextGen contributors
3+
# SPDX-License-Identifier: GPL-3.0-or-later
4+
# See CONTRIBUTORS for full list of authors.
5+
6+
"""#1121 — the contracts `CalibreDB.session` had as a plain attribute.
7+
8+
Turning an attribute into a property is only safe if the property keeps every
9+
contract the attribute had. Three of them were found by the test suite rather
10+
than by reading the code, and all three are pinned here so a later refactor
11+
cannot quietly drop one:
12+
13+
1. Assigning ``None`` drops the thread's session, and ``ensure_session()``
14+
then builds a working replacement. This is `cps/editbooks.py`'s recovery
15+
path for a poisoned transaction, and it is the one caller in ``cps/`` that
16+
assigns to ``session`` at runtime.
17+
2. Instances built with ``CalibreDB.__new__(CalibreDB)`` — no ``__init__`` —
18+
can still read and write ``session``.
19+
3. ``mock.patch.object(calibre_db, "session", ...)`` restores by *deleting*
20+
the attribute, so the property needs a deleter.
21+
22+
Plus the per-instance ``expire_on_commit`` preference, which background tasks
23+
set to False and which must survive being resolved on a thread other than the
24+
one that constructed the instance.
25+
"""
26+
27+
from __future__ import annotations
28+
29+
import threading
30+
from unittest import mock
31+
32+
import pytest
33+
from sqlalchemy import create_engine, text
34+
from sqlalchemy.orm import scoped_session, sessionmaker
35+
from sqlalchemy.pool import StaticPool
36+
37+
pytestmark = pytest.mark.unit
38+
39+
40+
def _wire():
41+
from cps.db import CalibreDB
42+
43+
engine = create_engine("sqlite://",
44+
connect_args={"check_same_thread": False},
45+
poolclass=StaticPool, future=True)
46+
CalibreDB.engine = engine
47+
CalibreDB.session_factory = scoped_session(
48+
sessionmaker(bind=engine, autocommit=False, autoflush=True, future=True))
49+
return CalibreDB
50+
51+
52+
def test_assigning_none_then_ensure_session_yields_a_working_replacement():
53+
"""cps/editbooks.py:1063-1075 — drop a poisoned session and carry on."""
54+
cls = _wire()
55+
db = cls()
56+
57+
first = db.session
58+
db.session.rollback()
59+
db.session.close()
60+
db.session = None
61+
62+
assert db._peek_session() is None, (
63+
"assigning None must drop the thread's session, or editbooks would "
64+
"retry its commit on the same poisoned session")
65+
66+
db.ensure_session()
67+
second = db.session
68+
69+
assert second is not first, "ensure_session returned the dropped session"
70+
assert second.execute(text("select 1")).scalar() == 1
71+
assert db.session is second, "the replacement must be stable across reads"
72+
73+
74+
def test_assigning_none_on_one_thread_does_not_disturb_another():
75+
"""The point of #1121: recovery is scoped to the thread that runs it."""
76+
cls = _wire()
77+
db = cls()
78+
other_ready = threading.Event()
79+
may_finish = threading.Event()
80+
seen = {}
81+
82+
def other_thread():
83+
seen["before"] = id(db.session)
84+
other_ready.set()
85+
may_finish.wait(10)
86+
seen["after"] = id(db.session)
87+
seen["usable"] = db.session.execute(text("select 1")).scalar()
88+
89+
t = threading.Thread(target=other_thread)
90+
t.start()
91+
other_ready.wait(10)
92+
93+
db.session = None # this thread recovers
94+
db.ensure_session()
95+
96+
may_finish.set()
97+
t.join(10)
98+
99+
assert seen["after"] == seen["before"], (
100+
"another thread's session was dropped by a recovery it did not run")
101+
assert seen["usable"] == 1
102+
103+
104+
def test_an_instance_built_without_init_can_still_use_session():
105+
"""Several tests use CalibreDB.__new__ to avoid needing a Flask app."""
106+
cls = _wire()
107+
inst = cls.__new__(cls)
108+
109+
sentinel = object()
110+
inst.session = sentinel
111+
assert inst.session is sentinel
112+
113+
del inst.session
114+
assert inst.session is not sentinel, "deleting must drop the override"
115+
assert inst.session.execute(text("select 1")).scalar() == 1
116+
117+
118+
def test_patch_object_round_trips_without_raising():
119+
"""patch.object restores an absent attribute by deleting it."""
120+
cls = _wire()
121+
db = cls()
122+
real = db.session
123+
124+
with mock.patch.object(db, "session", "stub"):
125+
assert db.session == "stub"
126+
127+
assert db.session is real, (
128+
"after the patch exits, session must resolve through the registry again")
129+
130+
131+
def test_expire_on_commit_preference_survives_on_another_thread():
132+
"""Background tasks construct CalibreDB(expire_on_commit=False)."""
133+
cls = _wire()
134+
worker = cls(expire_on_commit=False)
135+
seen = {}
136+
137+
def run():
138+
seen["value"] = worker.session.expire_on_commit
139+
140+
t = threading.Thread(target=run)
141+
t.start()
142+
t.join(10)
143+
144+
assert seen["value"] is False, (
145+
"a worker instance resolved a session on its own thread with "
146+
"expire_on_commit=True, so its detached objects would re-query")
147+
148+
149+
def test_session_is_none_when_there_is_no_factory():
150+
"""Callers check `if calibre_db.session is None` before the DB is set up."""
151+
from cps.db import CalibreDB
152+
153+
original = CalibreDB.session_factory
154+
try:
155+
CalibreDB.session_factory = None
156+
inst = CalibreDB()
157+
assert inst.session is None
158+
assert inst._peek_session() is None
159+
finally:
160+
CalibreDB.session_factory = original

0 commit comments

Comments
 (0)