Answer workspace refusals instead of crashing on them
json_error takes (status_code, message). Every refusal in the two workspace routers passed them the other way round, which builds a JSONResponse with a string status and raises TypeError inside Response.init_headers before a byte is written. The branch meant to explain a limit to the user returned 500 instead. A member already holding the default two workspaces therefore got a 500 from the workspace page's Open workspace button rather than the quota message. Twelve call sites corrected across both routers. workspace_open also let ContainerError escape as a second 500 on the same button when the ppy image is not built; it now returns 400 through the shared fail helper. tests/unit/responses.py AST-scans the whole package for both argument orders, so the swap cannot reappear anywhere.
This commit is contained in:
parent
192df12b1d
commit
56becfb3f7
@ -60,6 +60,7 @@ Every page/redirect endpoint also returns JSON when the client asks. Core in `de
|
||||
|
||||
- **Page GETs:** `return respond(request, "x.html", context, model=XOut)` - HTML renders the template; JSON does `XOut.model_validate(context).model_dump()`. One context, two renderings.
|
||||
- **Action POSTs:** `return action_result(request, url, data=<resource|None>)` - HTML 302 redirects; JSON returns `{ok, redirect, data}`. (Set cookies on the returned response after calling it, as `auth.py` login/signup do.)
|
||||
- **Refusals:** `return json_error(status_code, message)` - **status first, message second**. Swapping them is not a lint-level mistake, it is a guaranteed 500: `JSONResponse(..., status_code="some message")` raises `TypeError` inside `Response.init_headers` before any byte is written, so the branch that was supposed to explain a limit to the user crashes instead. It was a real production bug - every refusal in the two workspace routers was written `json_error(message, status)`, so a member already at the default 2-workspace quota got a 500 from the workspace page's **Open workspace** button rather than the quota message. `tests/unit/responses.py` now AST-scans the whole package for both argument orders, so the swap cannot come back anywhere.
|
||||
|
||||
Response models live in `devplacepy/schemas.py` (Pydantic v2, `extra="ignore"`, all-Optional so they validate the existing context dicts directly). **Always project users through `UserOut`** (and `AdminUserOut`) - the raw user rows contain `email`/`api_key`/`password_hash`, and the models drop them; never serialize a raw user row. List item shapes vary (feed/gists/news/admin-news are wrapped `{post|gist|article: ...}`; projects are flat rows with `author_name`/`my_vote`) - match the context exactly. Errors negotiate centrally: `main.py` 404/500/validation handlers and the rate-limit/maintenance middleware, plus `utils.require_user`/`require_admin` (401/403 for JSON, 303 redirect for browsers). The four legacy AJAX endpoints (votes/reactions/bookmarks/polls) keep their original flat JSON shapes and are left untouched. Documented in `docs_api.py`'s Conventions group.
|
||||
|
||||
|
||||
@ -117,7 +117,7 @@ async def admin_workspace_suspend(
|
||||
instance = _instance_or_404(uid)
|
||||
reason = (data.reason or "").strip()
|
||||
if not reason:
|
||||
return json_error("a reason is required and is shown to the owner", 400)
|
||||
return json_error(400, "a reason is required and is shown to the owner")
|
||||
provision.suspend(instance, admin["uid"], reason)
|
||||
_audit(request, admin, "container.workspace.suspend", instance, metadata={"reason": reason})
|
||||
owner = instance.get("workspace_owner_uid", "")
|
||||
@ -246,7 +246,7 @@ async def admin_workspace_quota(
|
||||
if not isinstance(admin, dict):
|
||||
return admin
|
||||
if not data.owner_id:
|
||||
return json_error("owner_id is required", 400)
|
||||
return json_error(400, "owner_id is required")
|
||||
table = get_table(quota.RULES_TABLE)
|
||||
existing = table.find_one(
|
||||
owner_kind="user", owner_id=data.owner_id, deleted_at=None
|
||||
|
||||
@ -15,11 +15,12 @@ from devplacepy.responses import action_result, json_error, respond
|
||||
from devplacepy.schemas import WorkspaceOut
|
||||
from devplacepy.services.audit import record as audit
|
||||
from devplacepy.services.containers import activity, api, forward, store
|
||||
from devplacepy.services.containers.api import ContainerError
|
||||
from devplacepy.services.containers.workspace import provision, quota, tunnels
|
||||
from devplacepy.services.containers.workspace.provision import WorkspaceError
|
||||
from devplacepy.utils import not_found, require_user
|
||||
|
||||
from ._shared import audit_instance
|
||||
from ._shared import audit_instance, fail
|
||||
|
||||
router = APIRouter()
|
||||
|
||||
@ -103,7 +104,9 @@ async def workspace_open(request: Request, slug: str):
|
||||
summary=str(error),
|
||||
result="denied",
|
||||
)
|
||||
return json_error(str(error), 400)
|
||||
return json_error(400, str(error))
|
||||
except ContainerError as error:
|
||||
return fail(error)
|
||||
audit_instance(
|
||||
request,
|
||||
user,
|
||||
@ -126,7 +129,7 @@ async def workspace_stop(request: Request, slug: str):
|
||||
project = _project_or_404(slug)
|
||||
instance = _workspace_or_404(project, user)
|
||||
if not can_manage_workspace(instance, project, user):
|
||||
return json_error("Not allowed to manage this workspace", 403)
|
||||
return json_error(403, "Not allowed to manage this workspace")
|
||||
provision.stop(instance)
|
||||
audit_instance(request, user, "container.workspace.stop", instance, project)
|
||||
return action_result(request, f"/projects/{slug}/workspace")
|
||||
@ -140,7 +143,7 @@ async def workspace_delete(request: Request, slug: str):
|
||||
project = _project_or_404(slug)
|
||||
instance = _workspace_or_404(project, user)
|
||||
if not can_manage_workspace(instance, project, user):
|
||||
return json_error("Not allowed to manage this workspace", 403)
|
||||
return json_error(403, "Not allowed to manage this workspace")
|
||||
for row in tunnels.list_for_instance(instance["uid"]):
|
||||
tunnels.soft_delete(row["uid"], user["uid"])
|
||||
store.delete_instance(instance["uid"], user["uid"])
|
||||
@ -156,7 +159,7 @@ async def tunnel_list(request: Request, slug: str):
|
||||
project = _project_or_404(slug)
|
||||
instance = _workspace_or_404(project, user)
|
||||
if not can_manage_workspace(instance, project, user):
|
||||
return json_error("Not allowed to manage this workspace", 403)
|
||||
return json_error(403, "Not allowed to manage this workspace")
|
||||
return {"tunnels": tunnels.list_for_instance(instance["uid"])}
|
||||
|
||||
|
||||
@ -170,17 +173,17 @@ async def tunnel_create(
|
||||
project = _project_or_404(slug)
|
||||
instance = _workspace_or_404(project, user)
|
||||
if not can_manage_workspace(instance, project, user):
|
||||
return json_error("Not allowed to manage this workspace", 403)
|
||||
return json_error(403, "Not allowed to manage this workspace")
|
||||
if data.container_port <= 0:
|
||||
return json_error("container_port must be between 1 and 65535", 400)
|
||||
return json_error(400, "container_port must be between 1 and 65535")
|
||||
limits = quota.resolve(instance.get("workspace_owner_uid", ""), instance)
|
||||
if limits.max_tunnels and tunnels.count_for_instance(
|
||||
instance["uid"]
|
||||
) >= limits.max_tunnels:
|
||||
return json_error(f"tunnel limit reached ({limits.max_tunnels})", 400)
|
||||
return json_error(400, f"tunnel limit reached ({limits.max_tunnels})")
|
||||
row = tunnels.create(instance, data.label, data.container_port, user["uid"])
|
||||
if not row:
|
||||
return json_error("could not create tunnel", 400)
|
||||
return json_error(400, "could not create tunnel")
|
||||
audit_instance(
|
||||
request,
|
||||
user,
|
||||
@ -202,7 +205,7 @@ async def tunnel_delete(request: Request, slug: str, uid: str):
|
||||
project = _project_or_404(slug)
|
||||
instance = _workspace_or_404(project, user)
|
||||
if not can_manage_workspace(instance, project, user):
|
||||
return json_error("Not allowed to manage this workspace", 403)
|
||||
return json_error(403, "Not allowed to manage this workspace")
|
||||
row = tunnels.get(uid)
|
||||
if not row or row.get("instance_uid") != instance["uid"]:
|
||||
raise not_found("Tunnel not found")
|
||||
@ -228,7 +231,7 @@ def _editor_guard(request: Request, slug: str, uid: str):
|
||||
if not instance or not instance.get("is_workspace"):
|
||||
raise not_found("Workspace not found")
|
||||
if not can_manage_workspace(instance, project, user):
|
||||
return None, None, json_error("Not allowed to open this workspace", 403)
|
||||
return None, None, json_error(403, "Not allowed to open this workspace")
|
||||
return project, instance, None
|
||||
|
||||
|
||||
|
||||
50
tests/unit/responses.py
Normal file
50
tests/unit/responses.py
Normal file
@ -0,0 +1,50 @@
|
||||
# retoor <retoor@molodetz.nl>
|
||||
|
||||
import ast
|
||||
from pathlib import Path
|
||||
|
||||
PACKAGE = Path(__file__).resolve().parents[2] / "devplacepy"
|
||||
|
||||
|
||||
def _json_error_calls():
|
||||
for path in sorted(PACKAGE.rglob("*.py")):
|
||||
tree = ast.parse(path.read_text(), filename=str(path))
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Call):
|
||||
continue
|
||||
name = node.func.attr if isinstance(node.func, ast.Attribute) else getattr(node.func, "id", "")
|
||||
if name == "json_error" and node.args:
|
||||
yield path.relative_to(PACKAGE.parent), node
|
||||
|
||||
|
||||
def test_json_error_never_receives_a_message_as_its_status():
|
||||
offenders = [
|
||||
f"{path}:{node.lineno}"
|
||||
for path, node in _json_error_calls()
|
||||
if isinstance(node.args[0], ast.Constant) and isinstance(node.args[0].value, str)
|
||||
]
|
||||
assert not offenders, (
|
||||
"json_error(status_code, message) - a string in the status position builds a "
|
||||
f"JSONResponse with a non-int status and raises TypeError at request time: {offenders}"
|
||||
)
|
||||
|
||||
|
||||
def test_json_error_never_receives_a_status_as_its_message():
|
||||
offenders = [
|
||||
f"{path}:{node.lineno}"
|
||||
for path, node in _json_error_calls()
|
||||
if len(node.args) > 1
|
||||
and isinstance(node.args[1], ast.Constant)
|
||||
and isinstance(node.args[1].value, int)
|
||||
]
|
||||
assert not offenders, (
|
||||
"json_error(status_code, message) - an int in the message position means the "
|
||||
f"arguments were swapped: {offenders}"
|
||||
)
|
||||
|
||||
|
||||
def test_every_json_error_status_is_a_real_http_error_code():
|
||||
for path, node in _json_error_calls():
|
||||
first = node.args[0]
|
||||
if isinstance(first, ast.Constant) and isinstance(first.value, int):
|
||||
assert 400 <= first.value <= 599, f"{path}:{node.lineno} status {first.value}"
|
||||
Loading…
Reference in New Issue
Block a user