test(fence): owner filters in any form, statement writes under any name, core commits by list
The owner-filter check missed an aliased() model, an import alias, .in_() and filter_by; four live inline filters on aliased VideoState go through owned_by now. Cross-unit writes are seen under any import name, through sa.update and via __table__. Core modules may commit only where CORE_COMMITS names the reason; trim_read and audit.gc flush now and their callers commit. The pin target, the debug-session header and a link's target fetch through owned_row. The docs claim what the fence holds, and say what is beyond it.
This commit is contained in:
@@ -408,7 +408,38 @@ UNIT_COMMITS = {
|
||||
"plex/watch_sync.py:run_plex_watch_reconcile",
|
||||
},
|
||||
}
|
||||
# Where core code may commit, each with why. Everything else in core flushes and leaves the commit
|
||||
# to its caller, as a unit's code does; a route's own helpers are held by test_route_conventions.
|
||||
CORE_COMMITS = {
|
||||
# Quota spend and a background run's row are durable whatever the caller does next: each opens
|
||||
# a session of its own.
|
||||
"quota.py",
|
||||
"jobruns.py",
|
||||
# The scheduler's jobs own their session.
|
||||
"scheduler.py",
|
||||
# The instance's one settings row: created on first read, and the setup steps that finish it.
|
||||
"state.py",
|
||||
# A token's last-used stamp lands ahead of the route: a read-only route never commits, and a
|
||||
# write route's rollback must not take it along. The expiry warning is a scheduled job.
|
||||
"apitokens.py:_token_caller",
|
||||
"apitokens.py:warn_expiring",
|
||||
# An account's erasure is one operation shared by the self-service and the admin delete; the
|
||||
# Google revoke and the confirmation run after it, in the background.
|
||||
"auth.py:purge_user",
|
||||
# The undelivered-email notice opens its own session (its callers are often background tasks).
|
||||
"email.py:_alert_admins",
|
||||
# One notice, committed unless its caller batches (`commit=False`).
|
||||
"notifications.py:create_notification",
|
||||
# A refreshed (or dead) YouTube grant must persist even when the call that needed it fails and
|
||||
# its caller rolls back.
|
||||
"youtube/client.py:_access_token",
|
||||
"youtube/client.py:_drop_dead_grant",
|
||||
}
|
||||
SQL_WRITES = {"insert", "update", "delete", "pg_insert"}
|
||||
ALL_MODEL_NAMES = set(UNIT_MODEL_NAMES) | CORE_MODELS
|
||||
# The column methods that compare an owner column as a filter does (`Model.user_id.in_(ids)`).
|
||||
# `.is_(None)` is not one: it asks for the rows nobody owns (the system's own tags).
|
||||
OWNER_METHODS = {"in_", "not_in", "notin_", "__eq__", "__ne__"}
|
||||
|
||||
|
||||
def _package_unit(rel: str) -> str | None:
|
||||
@@ -440,6 +471,64 @@ def _imported_models(tree: ast.AST) -> dict[str, str]:
|
||||
return out
|
||||
|
||||
|
||||
def _model_aliases(tree: ast.AST) -> dict[str, str]:
|
||||
"""Every other name a module binds to a model, by that name: an import under another name, and
|
||||
`x = aliased(Model)` — an alias is the same table, and its owner column the same filter."""
|
||||
out = {}
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.ImportFrom) and "models" in (node.module or ""):
|
||||
for alias in node.names:
|
||||
if alias.name in ALL_MODEL_NAMES:
|
||||
out[alias.asname or alias.name] = alias.name
|
||||
for node in ast.walk(tree):
|
||||
if (
|
||||
isinstance(node, ast.Assign)
|
||||
and isinstance(node.value, ast.Call)
|
||||
and _called_name(node.value) == "aliased"
|
||||
and node.value.args
|
||||
and isinstance(node.value.args[0], ast.Name)
|
||||
):
|
||||
source = node.value.args[0].id
|
||||
model = out.get(source, source if source in ALL_MODEL_NAMES else None)
|
||||
for target in node.targets:
|
||||
if model and isinstance(target, ast.Name):
|
||||
out[target.id] = model
|
||||
return out
|
||||
|
||||
|
||||
def _called_name(call: ast.Call) -> str | None:
|
||||
func = call.func
|
||||
return func.id if isinstance(func, ast.Name) else getattr(func, "attr", None)
|
||||
|
||||
|
||||
def _sql_write_names(tree: ast.AST) -> tuple[set[str], set[str]]:
|
||||
"""The names a module calls SQLAlchemy's statement writes by — `insert`/`update`/`delete` as
|
||||
imported, under any alias — and the names it binds SQLAlchemy's modules to (`sa.update(...)`)."""
|
||||
functions, modules = set(SQL_WRITES), set()
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.ImportFrom) and (node.module or "").split(".")[0] == "sqlalchemy":
|
||||
for alias in node.names:
|
||||
if alias.name in {"insert", "update", "delete"}:
|
||||
functions.add(alias.asname or alias.name)
|
||||
elif alias.name in {"dialects", "postgresql", "sql"}:
|
||||
modules.add(alias.asname or alias.name)
|
||||
elif isinstance(node, ast.Import):
|
||||
for alias in node.names:
|
||||
if alias.name.split(".")[0] == "sqlalchemy":
|
||||
modules.add(alias.asname or alias.name.split(".")[0])
|
||||
return functions, modules
|
||||
|
||||
|
||||
def _written_table(call: ast.Call) -> str | None:
|
||||
"""The name of what a statement write's first argument names: `Model`, or `Model.__table__`."""
|
||||
if not call.args:
|
||||
return None
|
||||
arg = call.args[0]
|
||||
if isinstance(arg, ast.Attribute) and arg.attr == "__table__":
|
||||
arg = arg.value
|
||||
return arg.id if isinstance(arg, ast.Name) else None
|
||||
|
||||
|
||||
def _enclosing_functions(tree: ast.AST) -> dict[ast.AST, str]:
|
||||
out = {}
|
||||
for fn in ast.walk(tree):
|
||||
@@ -461,49 +550,67 @@ def package_violations(rel: str, source: str) -> list[str]:
|
||||
continue
|
||||
if theirs not in READS.get(mine or "", set()):
|
||||
found.append(f"0: {mine or 'core'} imports {theirs}'s {name}")
|
||||
write_functions, sql_modules = _sql_write_names(tree)
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call) or not isinstance(node.func, ast.Name):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
func = node.func
|
||||
written = None
|
||||
if node.func.id in models:
|
||||
written = models[node.func.id]
|
||||
elif node.func.id in SQL_WRITES and node.args and isinstance(node.args[0], ast.Name):
|
||||
written = models.get(node.args[0].id)
|
||||
if isinstance(func, ast.Name) and func.id in models:
|
||||
written = models[func.id]
|
||||
elif (isinstance(func, ast.Name) and func.id in write_functions) or (
|
||||
isinstance(func, ast.Attribute)
|
||||
and func.attr in {"insert", "update", "delete"}
|
||||
and isinstance(_root(func), ast.Name)
|
||||
and _root(func).id in sql_modules
|
||||
):
|
||||
written = models.get(_written_table(node) or "")
|
||||
if written is None:
|
||||
continue
|
||||
theirs = UNIT_MODEL_NAMES[written]
|
||||
if theirs != mine:
|
||||
found.append(f"{node.lineno}: {mine or 'core'} writes {theirs}'s {written}")
|
||||
found += _owner_filters(tree)
|
||||
if mine is not None:
|
||||
functions = _enclosing_functions(tree)
|
||||
allowed = UNIT_COMMITS.get(mine, set())
|
||||
for node in ast.walk(tree):
|
||||
if (
|
||||
isinstance(node, ast.Call)
|
||||
and isinstance(node.func, ast.Attribute)
|
||||
and node.func.attr == "commit"
|
||||
and rel not in allowed
|
||||
and f"{rel}:{functions.get(node)}" not in allowed
|
||||
):
|
||||
found.append(f"{node.lineno}: commits in {functions.get(node)}")
|
||||
functions = _enclosing_functions(tree)
|
||||
allowed = UNIT_COMMITS.get(mine, set()) if mine is not None else CORE_COMMITS
|
||||
for node in ast.walk(tree):
|
||||
if (
|
||||
isinstance(node, ast.Call)
|
||||
and isinstance(node.func, ast.Attribute)
|
||||
and node.func.attr == "commit"
|
||||
and rel not in allowed
|
||||
and f"{rel}:{functions.get(node)}" not in allowed
|
||||
):
|
||||
found.append(f"{node.lineno}: commits in {functions.get(node)}")
|
||||
return sorted(found, key=lambda v: int(v.split(":")[0]))
|
||||
|
||||
|
||||
def _owner_filters(tree: ast.AST) -> list[str]:
|
||||
"""A comparison on a table's owner column written inline (`Model.user_id == …`), not through
|
||||
`userscope.owned_by`."""
|
||||
names = {name: name for name in ALL_MODEL_NAMES} | _model_aliases(tree)
|
||||
|
||||
def owner_column(node: ast.expr) -> bool:
|
||||
return (
|
||||
isinstance(node, ast.Attribute)
|
||||
and isinstance(node.value, ast.Name)
|
||||
and node.value.id in names
|
||||
and node.attr in OWNER_COLUMNS
|
||||
)
|
||||
|
||||
found = []
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Compare):
|
||||
for side in (node.left, *node.comparators):
|
||||
if (
|
||||
isinstance(side, ast.Attribute)
|
||||
and isinstance(side.value, ast.Name)
|
||||
and side.value.id in set(UNIT_MODEL_NAMES) | CORE_MODELS
|
||||
and side.attr in OWNER_COLUMNS
|
||||
):
|
||||
if owner_column(side):
|
||||
found.append(f"{node.lineno}: filters {side.value.id}.{side.attr} inline")
|
||||
elif isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute):
|
||||
if node.func.attr in OWNER_METHODS and owner_column(node.func.value):
|
||||
found.append(f"{node.lineno}: filters an owner column with {node.func.attr}")
|
||||
elif node.func.attr == "filter_by" and any(
|
||||
k.arg in OWNER_COLUMNS for k in node.keywords
|
||||
):
|
||||
found.append(f"{node.lineno}: filters an owner column with filter_by")
|
||||
return found
|
||||
|
||||
|
||||
@@ -524,7 +631,7 @@ def test_every_unit_package_exists():
|
||||
for prefixes in UNIT_PACKAGES.values():
|
||||
for p in prefixes:
|
||||
assert (APP / p).exists(), p
|
||||
for commits in UNIT_COMMITS.values():
|
||||
for commits in (*UNIT_COMMITS.values(), CORE_COMMITS):
|
||||
for entry in commits:
|
||||
assert (APP / entry.split(":")[0]).exists(), entry
|
||||
|
||||
@@ -544,6 +651,22 @@ PACKAGE_CASES = {
|
||||
("downloads/x.py", "q = DownloadJob.user_id == 1"): 1,
|
||||
("downloads/x.py", "q = owned_by(DownloadJob.user_id, 1)"): 0,
|
||||
("quota.py", "q = QuotaEvent.user_id == 1"): 1, # core too
|
||||
("downloads/x.py", "from app.downloads.models import DownloadJob as J\nq = J.user_id == 1"): 1,
|
||||
("downloads/x.py", "st = aliased(VideoState)\nq = st.user_id == 1"): 1,
|
||||
("downloads/x.py", "q = DownloadJob.user_id.in_([1])"): 1,
|
||||
("downloads/x.py", "q = select(DownloadJob).filter_by(user_id=1)"): 1,
|
||||
("downloads/x.py", "q = Tag.user_id.is_(None)"): 0, # the rows nobody owns
|
||||
(
|
||||
"downloads/x.py",
|
||||
"from sqlalchemy import update as up\nfrom app.models import Video\nup(Video)",
|
||||
): 1,
|
||||
(
|
||||
"downloads/x.py",
|
||||
"import sqlalchemy as sa\nfrom app.models import Video\nsa.update(Video)",
|
||||
): 1,
|
||||
("downloads/x.py", "from app.models import Video\nupdate(Video.__table__)"): 1,
|
||||
("notifications.py", "def f(db):\n db.commit()"): 1, # core flushes too
|
||||
("state.py", "def f(db):\n db.commit()"): 0,
|
||||
("downloads/x.py", "def f(db):\n db.commit()"): 1,
|
||||
("downloads/gc.py", "def f(db):\n db.commit()"): 0,
|
||||
("downloads/upload.py", "def ingest_upload(db):\n db.commit()"): 0,
|
||||
|
||||
Reference in New Issue
Block a user