test(arch): a unit's HTTP layer does no database work beyond its pin
The fence is switched on unit by unit: each route file's database work (SQL built, session data calls, model attributes set, unit tables imported) is pinned at today's count and may only shrink. A core route may not reach into a unit's tables. Every model has an owner.
This commit is contained in:
@@ -0,0 +1,306 @@
|
||||
"""The HTTP layer of a unit does no database work of its own, held in code rather than prose.
|
||||
|
||||
A route validates, calls its unit's `queries` (reads) or `service` (writes), serializes and commits.
|
||||
What it may not do is build SQL, call the session's data methods, change a model's attributes or
|
||||
import a unit's tables: when a route did those, ownership, the commit and the conditional `WHERE`
|
||||
lived in a different place in each handler, and a fix in one never reached its siblings.
|
||||
|
||||
The rule is switched on unit by unit. `ALLOWED` pins what each HTTP file still does today, and the
|
||||
pin is exact: a count that rises fails (new database work in a route), and one that falls fails
|
||||
until its entry is lowered (the fence only tightens, as `baseline.json` does for basedpyright). An
|
||||
empty `ALLOWED` is the end state — a file listed there is a debt, not an exemption.
|
||||
|
||||
A core route (identity, admin, ops) is not a unit's HTTP layer and may use the session; it may
|
||||
still not reach into a unit's tables.
|
||||
"""
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
from app.models import Base
|
||||
|
||||
APP = Path(__file__).parent.parent / "app"
|
||||
ROUTES = APP / "routes"
|
||||
|
||||
# Each unit's tables. A new model is placed here (or in CORE_MODELS) by the test below, so a table
|
||||
# never joins the schema without an owner.
|
||||
UNIT_MODELS = {
|
||||
"capture": {
|
||||
"MediaAsset",
|
||||
"DownloadProfile",
|
||||
"FileFolder",
|
||||
"DownloadJob",
|
||||
"DownloadShare",
|
||||
"DownloadQuota",
|
||||
"DownloadLink",
|
||||
"DownloadCredential",
|
||||
"LibraryState",
|
||||
},
|
||||
"reader": {
|
||||
"VideoState",
|
||||
"Playlist",
|
||||
"PlaylistItem",
|
||||
"SavedView",
|
||||
"ComfortPin",
|
||||
"Tag",
|
||||
"ChannelTag",
|
||||
},
|
||||
"catalog": {
|
||||
"Channel",
|
||||
"Subscription",
|
||||
"Video",
|
||||
"SearchFind",
|
||||
"ExploredChannel",
|
||||
"BlockedChannel",
|
||||
},
|
||||
"plex": {
|
||||
"PlexLibrary",
|
||||
"PlexShow",
|
||||
"PlexSeason",
|
||||
"PlexItem",
|
||||
"PlexPerson",
|
||||
"PlexCollection",
|
||||
"PlexState",
|
||||
"PlexLink",
|
||||
"PlexPlaylist",
|
||||
"PlexPlaylistItem",
|
||||
},
|
||||
}
|
||||
CORE_MODELS = {
|
||||
"User",
|
||||
"OAuthToken",
|
||||
"AuthToken",
|
||||
"Invite",
|
||||
"ApiQuotaUsage",
|
||||
"QuotaEvent",
|
||||
"AuditLog",
|
||||
"LogEvent",
|
||||
"DebugSession",
|
||||
"DebugScreenshot",
|
||||
"AppState",
|
||||
"JobRun",
|
||||
"SystemConfig",
|
||||
"Notification",
|
||||
"ApiToken",
|
||||
"ApiTokenGrant",
|
||||
}
|
||||
|
||||
# Which unit each HTTP file belongs to (path relative to app/routes; a folder covers its files).
|
||||
# Anything not listed is a core route.
|
||||
UNIT_ROUTES = {
|
||||
"downloads.py": "capture",
|
||||
"files.py": "capture",
|
||||
"public.py": "capture",
|
||||
"feed.py": "reader",
|
||||
"playlists.py": "reader",
|
||||
"saved_views.py": "reader",
|
||||
"discover.py": "reader",
|
||||
"tags.py": "reader",
|
||||
"channels.py": "catalog",
|
||||
"search.py": "catalog",
|
||||
"sync.py": "catalog",
|
||||
"plex/": "plex",
|
||||
}
|
||||
|
||||
# What each HTTP file still does, by file: the debt a unit's sprint pays off. Exact counts. A core
|
||||
# route's count is only the unit tables it imports; it is paid by the sprint of the unit it reaches.
|
||||
ALLOWED = {
|
||||
"channels.py": 83,
|
||||
"discover.py": 7,
|
||||
"downloads.py": 88,
|
||||
"feed.py": 36,
|
||||
"files.py": 58,
|
||||
"playlists.py": 72,
|
||||
"plex/_shared.py": 1,
|
||||
"plex/admin.py": 12,
|
||||
"plex/browse.py": 93,
|
||||
"plex/collections.py": 33,
|
||||
"plex/detail.py": 29,
|
||||
"plex/discover.py": 44,
|
||||
"plex/people.py": 20,
|
||||
"plex/playback.py": 42,
|
||||
"plex/playlists.py": 80,
|
||||
"public.py": 12,
|
||||
"runs.py": 1,
|
||||
"saved_views.py": 17,
|
||||
"scheduler.py": 3,
|
||||
"search.py": 46,
|
||||
"sync.py": 38,
|
||||
"tags.py": 10,
|
||||
"tokens.py": 1,
|
||||
}
|
||||
|
||||
# The session methods that read or write data. `commit` and `rollback` stay with the handler.
|
||||
SESSION_DATA = {
|
||||
"add",
|
||||
"add_all",
|
||||
"delete",
|
||||
"execute",
|
||||
"get",
|
||||
"merge",
|
||||
"flush",
|
||||
"refresh",
|
||||
"query",
|
||||
"scalar",
|
||||
"scalars",
|
||||
"expunge",
|
||||
}
|
||||
# What a route may name from SQLAlchemy: the session's type, for its `db: Session` parameter, and
|
||||
# the errors its commit raises (the commit is the handler's, so is catching what it throws).
|
||||
SQLALCHEMY_ALLOWED = {"Session"}
|
||||
SQLALCHEMY_ALLOWED_MODULES = {"sqlalchemy.exc"}
|
||||
# Objects a route owns and may set attributes on; anything else it writes to is a model.
|
||||
OWN_OBJECTS = {"self", "cls", "request", "response"}
|
||||
UNIT_MODEL_NAMES = {name: unit for unit, names in UNIT_MODELS.items() for name in names}
|
||||
|
||||
|
||||
def _unit_of(rel: str) -> str | None:
|
||||
for key, unit in UNIT_ROUTES.items():
|
||||
if rel == key or (key.endswith("/") and rel.startswith(key)):
|
||||
return unit
|
||||
return None
|
||||
|
||||
|
||||
def _session_names(tree: ast.AST) -> set[str]:
|
||||
"""The names a module binds to a database session: a parameter typed `Session`, or a name
|
||||
assigned (or `with`-bound) from `SessionLocal()`."""
|
||||
names = set()
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.arg) and isinstance(node.annotation, ast.Name):
|
||||
if node.annotation.id == "Session":
|
||||
names.add(node.arg)
|
||||
made = None
|
||||
if isinstance(node, ast.Assign) and len(node.targets) == 1:
|
||||
made, target = node.value, node.targets[0]
|
||||
elif isinstance(node, ast.withitem):
|
||||
made, target = node.context_expr, node.optional_vars
|
||||
if (
|
||||
isinstance(made, ast.Call)
|
||||
and isinstance(made.func, ast.Name)
|
||||
and made.func.id == "SessionLocal"
|
||||
and isinstance(target, ast.Name)
|
||||
):
|
||||
names.add(target.id)
|
||||
return names
|
||||
|
||||
|
||||
def _root(node: ast.expr) -> ast.expr:
|
||||
while isinstance(node, (ast.Attribute, ast.Subscript)):
|
||||
node = node.value
|
||||
return node
|
||||
|
||||
|
||||
def violations(source: str, unit: str | None) -> list[str]:
|
||||
"""Each piece of database work in an HTTP file, as `line: what`. A core route (`unit` None) is
|
||||
held only to the last rule: no unit's tables."""
|
||||
tree = ast.parse(source)
|
||||
found = []
|
||||
sql_names = set()
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.ImportFrom):
|
||||
module = node.module or ""
|
||||
for alias in node.names:
|
||||
bound = alias.asname or alias.name
|
||||
if (
|
||||
module.split(".")[0] == "sqlalchemy"
|
||||
and module not in SQLALCHEMY_ALLOWED_MODULES
|
||||
and alias.name not in SQLALCHEMY_ALLOWED
|
||||
):
|
||||
sql_names.add(bound)
|
||||
if (
|
||||
alias.name in UNIT_MODEL_NAMES
|
||||
and module.startswith("app.")
|
||||
and "models" in module
|
||||
):
|
||||
found.append(
|
||||
f"{node.lineno}: imports {UNIT_MODEL_NAMES[alias.name]}'s {alias.name}"
|
||||
)
|
||||
elif isinstance(node, ast.Import):
|
||||
for alias in node.names:
|
||||
if alias.name.split(".")[0] == "sqlalchemy":
|
||||
sql_names.add((alias.asname or alias.name).split(".")[0])
|
||||
if unit is None:
|
||||
return found
|
||||
sessions = _session_names(tree)
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Load) and node.id in sql_names:
|
||||
found.append(f"{node.lineno}: uses sqlalchemy's {node.id}")
|
||||
if isinstance(node, ast.Call):
|
||||
f = node.func
|
||||
if isinstance(f, ast.Attribute) and isinstance(f.value, ast.Name):
|
||||
if f.value.id in sessions and f.attr in SESSION_DATA:
|
||||
found.append(f"{node.lineno}: {f.value.id}.{f.attr}()")
|
||||
targets = []
|
||||
if isinstance(node, ast.Assign):
|
||||
targets = node.targets
|
||||
elif isinstance(node, (ast.AugAssign, ast.AnnAssign)):
|
||||
targets = [node.target]
|
||||
for target in targets:
|
||||
if isinstance(target, ast.Attribute):
|
||||
base = _root(target)
|
||||
if not (isinstance(base, ast.Name) and base.id in OWN_OBJECTS):
|
||||
found.append(f"{node.lineno}: sets .{target.attr}")
|
||||
return sorted(found, key=lambda v: int(v.split(":")[0]))
|
||||
|
||||
|
||||
def _http_files():
|
||||
for path in sorted(ROUTES.rglob("*.py")):
|
||||
rel = path.relative_to(ROUTES).as_posix()
|
||||
yield rel, path.read_text(encoding="utf-8")
|
||||
|
||||
|
||||
def test_every_table_has_an_owner():
|
||||
tables = {mapper.class_.__name__ for mapper in Base.registry.mappers}
|
||||
owned = set(UNIT_MODEL_NAMES) | CORE_MODELS
|
||||
assert tables - owned == set(), "a model with no unit: place it in UNIT_MODELS or CORE_MODELS"
|
||||
assert owned - tables == set(), "UNIT_MODELS/CORE_MODELS name a model that no longer exists"
|
||||
assert not set(UNIT_MODEL_NAMES) & CORE_MODELS
|
||||
|
||||
|
||||
def test_every_listed_route_exists():
|
||||
rels = [rel for rel, _ in _http_files()]
|
||||
for key in UNIT_ROUTES:
|
||||
assert any(rel == key or (key.endswith("/") and rel.startswith(key)) for rel in rels), key
|
||||
assert set(ALLOWED) <= set(rels), set(ALLOWED) - set(rels)
|
||||
|
||||
|
||||
def test_no_http_file_does_database_work_beyond_its_pin():
|
||||
drift = {}
|
||||
for rel, src in _http_files():
|
||||
found = violations(src, _unit_of(rel))
|
||||
pinned = ALLOWED.get(rel, 0)
|
||||
if len(found) != pinned:
|
||||
drift[rel] = (pinned, len(found), found if len(found) > pinned else "lower its pin")
|
||||
assert drift == {}, (
|
||||
"an HTTP file's database work moved off its pin (pinned, now, what): move new work into"
|
||||
" the unit's queries/service; when a count fell, lower ALLOWED to it"
|
||||
)
|
||||
|
||||
|
||||
# The detector itself: each kind of database work it must see, and what it must leave alone.
|
||||
DETECTOR_CASES = {
|
||||
"from sqlalchemy import select\nq = select(1)": 1,
|
||||
"import sqlalchemy as sa\nsa.select(1).where(sa.true())": 2,
|
||||
"from sqlalchemy import func\nn = func.count()": 1,
|
||||
"from sqlalchemy.orm import Session\ndef f(db: Session): db.commit(); db.rollback()": 0,
|
||||
"from sqlalchemy.orm import Session\ndef f(db: Session): db.add(x)": 1,
|
||||
"def f(conn: Session): conn.execute(q)": 1,
|
||||
"from app.db import SessionLocal\nwith SessionLocal() as s:\n s.get(X, 1)": 1,
|
||||
"from app.db import SessionLocal\ns = SessionLocal()\ns.scalars(q)": 1,
|
||||
"def f(job): job.status = 'x'": 1,
|
||||
"def f(job): job.meta['k'] = 1": 0,
|
||||
"def f(job): job.parts.count += 1": 1,
|
||||
"def f(self, request, response): self.x = 1; request.state.y = 2; response.status_code = 3": 0,
|
||||
"from app.models import DownloadJob, User": 1,
|
||||
"from app.capture.models import DownloadJob as J": 1,
|
||||
"def f(cache): cache.get('k')": 0,
|
||||
"from sqlalchemy.exc import IntegrityError\ntry:\n pass\nexcept IntegrityError:\n pass": 0,
|
||||
}
|
||||
|
||||
|
||||
def test_the_detector_sees_each_kind_of_database_work():
|
||||
for source, expected in DETECTOR_CASES.items():
|
||||
assert len(violations(source, "capture")) == expected, source
|
||||
assert violations("from app.models import Video\nfrom sqlalchemy import select", None) == [
|
||||
"1: imports catalog's Video"
|
||||
]
|
||||
Reference in New Issue
Block a user