Skip to content

Commit ce4f195

Browse files
scosmanscosman
authored
Merge pull request #1638 from Kiln-AI/claude/api-404-json-response-cmgxfx
Return JSON 404s for API paths instead of web app HTML
2 parents d4554ae + 0853aae commit ce4f195

3 files changed

Lines changed: 138 additions & 14 deletions

File tree

‎app/desktop/studio_server/test_webhost.py‎

Lines changed: 114 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,19 @@
66
from app.desktop.studio_server.webhost import connect_webhost
77
from fastapi import FastAPI, HTTPException
88
from fastapi.testclient import TestClient
9+
from kiln_server.custom_errors import connect_custom_errors
10+
11+
WEB_APP_404_BODY = "<html><body>custom not found</body></html>"
912

1013

1114
@pytest.fixture
1215
def temp_studio():
1316
with tempfile.TemporaryDirectory() as d:
1417
os.makedirs(d, exist_ok=True)
18+
# The compiled web app always ships a 404.html: StaticFiles in html mode
19+
# will serve it for any miss unless we prevent it on API paths.
20+
with open(os.path.join(d, "404.html"), "w", encoding="utf-8") as f:
21+
f.write(WEB_APP_404_BODY)
1522
with patch("app.desktop.studio_server.webhost.studio_path", lambda: d):
1623
yield d
1724

@@ -24,25 +31,122 @@ def app_with_webhost(temp_studio):
2431
def forced_not_found():
2532
raise HTTPException(status_code=404, detail="test missing resource")
2633

34+
@app.get("/api/get-only")
35+
def get_only():
36+
return {"ok": True}
37+
2738
connect_webhost(app)
2839
return app
2940

3041

31-
def test_not_found_handler_returns_json_for_api_http_exception(app_with_webhost):
32-
client = TestClient(app_with_webhost)
33-
response = client.get("/api/forced-not-found")
42+
@pytest.fixture
43+
def client(app_with_webhost):
44+
return TestClient(app_with_webhost)
45+
46+
47+
def assert_json_404(response, message="Not Found"):
48+
assert response.status_code == 404
49+
assert response.headers.get("content-type", "").startswith("application/json")
50+
assert response.json() == {"message": message}
51+
52+
53+
def assert_json_405(response):
54+
assert response.status_code == 405
55+
assert response.headers.get("content-type", "").startswith("application/json")
56+
57+
58+
def test_not_found_handler_returns_json_for_api_http_exception(client):
59+
assert_json_404(client.get("/api/forced-not-found"), "test missing resource")
60+
61+
62+
@pytest.mark.parametrize(
63+
"path",
64+
[
65+
"/api",
66+
"/api/",
67+
"/api/some-unmatched-path",
68+
"/api/nested/unmatched/path",
69+
"/api/unmatched.html",
70+
],
71+
)
72+
def test_unmatched_api_paths_return_json_not_web_app_404(client, path):
73+
response = client.get(path)
74+
assert_json_404(response)
75+
assert WEB_APP_404_BODY not in response.text
76+
77+
78+
def test_api_head_request_returns_json_404(client):
79+
# HEAD responses carry no body, so the JSON shape can't be asserted here.
80+
response = client.head("/api/some-unmatched-path")
3481
assert response.status_code == 404
35-
assert response.json() == {"detail": "test missing resource"}
3682
assert response.headers.get("content-type", "").startswith("application/json")
83+
assert response.text == ""
84+
85+
86+
@pytest.mark.parametrize("method", ["POST", "PUT", "DELETE", "OPTIONS"])
87+
@pytest.mark.parametrize("path", ["/api/some-unmatched-path", "/api/get-only"])
88+
def test_non_get_method_on_api_path_keeps_405(client, method, path):
89+
# The web host mount matches every path, so any method the static file
90+
# server doesn't serve reaches it and gets its 405 (which, unlike a
91+
# router-generated one, carries no Allow header). All of this predates the
92+
# JSON 404 handling and is left as-is: 405 is right for a wrong verb on a
93+
# real route, and for a path that doesn't exist at all it's arguably wrong
94+
# (404 would be more defensible) but not something this change touches.
95+
assert_json_405(client.request(method, path))
3796

3897

39-
def test_not_found_handler_serves_404_html_for_non_api_paths(temp_studio):
40-
with open(os.path.join(temp_studio, "404.html"), "w", encoding="utf-8") as f:
41-
f.write("<html><body>custom not found</body></html>")
98+
def test_matched_api_route_still_works(client):
99+
response = client.get("/api/get-only")
100+
assert response.status_code == 200
101+
assert response.json() == {"ok": True}
42102

103+
104+
@pytest.mark.parametrize(
105+
"path",
106+
[
107+
"/route-that-does-not-exist",
108+
# Paths that merely start with the letters "api" are web app paths
109+
"/apiary",
110+
"/api-keys",
111+
],
112+
)
113+
def test_non_api_paths_serve_web_app_404(client, path):
114+
response = client.get(path)
115+
assert response.status_code == 404
116+
assert response.headers.get("content-type", "").startswith("text/html")
117+
assert WEB_APP_404_BODY in response.text
118+
119+
120+
def test_non_get_method_on_non_api_path_keeps_405(client):
121+
assert_json_405(client.post("/route-that-does-not-exist"))
122+
123+
124+
def test_api_404s_with_custom_error_handlers(temp_studio):
125+
# The real server registers the shared error handlers before the web host.
126+
# This 404 status handler must keep winning over their HTTPException class
127+
# handler, and neither may reintroduce the HTML 404.
43128
app = FastAPI()
129+
130+
@app.get("/api/forced-not-found")
131+
def forced_not_found():
132+
raise HTTPException(status_code=404, detail="test missing resource")
133+
134+
connect_custom_errors(app)
44135
connect_webhost(app)
45136
client = TestClient(app)
46-
response = client.get("/route-that-does-not-exist")
47-
assert response.status_code == 404
48-
assert "custom not found" in response.text
137+
138+
assert_json_404(client.get("/api/some-unmatched-path"))
139+
assert_json_404(client.get("/api/forced-not-found"), "test missing resource")
140+
141+
web_app_response = client.get("/route-that-does-not-exist")
142+
assert web_app_response.status_code == 404
143+
assert WEB_APP_404_BODY in web_app_response.text
144+
145+
146+
def test_non_api_static_file_still_served(temp_studio, client):
147+
with open(os.path.join(temp_studio, "page.html"), "w", encoding="utf-8") as f:
148+
f.write("<html><body>real page</body></html>")
149+
150+
response = client.get("/page")
151+
assert response.status_code == 200
152+
assert "real page" in response.text

‎app/desktop/studio_server/webhost.py‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ def studio_path():
2626
return os.path.join(base_path, "../../app/web_ui/build")
2727

2828

29+
API_PATH_PREFIX = "/api"
30+
31+
32+
def is_api_path(url_path: str) -> bool:
33+
return url_path == API_PATH_PREFIX or url_path.startswith(f"{API_PATH_PREFIX}/")
34+
35+
2936
def add_no_cache_headers(response: Response):
3037
# This is already local, disable browser caching to prevent issues of old web-app trying to load old APIs and out of date web-ui
3138
response.headers["Cache-Control"] = "no-store, no-cache, must-revalidate, max-age=0"
@@ -36,6 +43,17 @@ def add_no_cache_headers(response: Response):
3643
# File server that maps /foo/bar to /foo/bar.html (Starlette StaticFiles only does index.html)
3744
class HTMLStaticFiles(StaticFiles):
3845
async def get_response(self, path: str, scope):
46+
# API paths must never be served web app content: StaticFiles in html mode
47+
# answers any miss with the web app's 404.html instead of raising, which
48+
# would bypass the JSON 404 handler below. Only guard the methods
49+
# StaticFiles serves, so other methods keep falling through to its 405.
50+
# Like the 404 handler below, this assumes an empty ASGI root_path (the
51+
# desktop server sets none); under a prefix it would need get_route_path.
52+
request_method = scope.get("method")
53+
request_path = scope.get("path", "")
54+
if request_method in ("GET", "HEAD") and is_api_path(request_path):
55+
raise StarletteHTTPException(status_code=404)
56+
3957
try:
4058
response = await super().get_response(path, scope)
4159
if response.status_code != 404:
@@ -62,11 +80,13 @@ def connect_webhost(app: FastAPI):
6280
@app.exception_handler(404)
6381
def not_found_exception_handler(request, exc):
6482
# don't handle /api routes, which return JSON errors
65-
if request.url.path.startswith("/api"):
83+
if is_api_path(request.url.path):
6684
if isinstance(exc, StarletteHTTPException):
85+
# "message" matches every other Kiln API error (custom_errors.py), and
86+
# is the key the web UI reads.
6787
return JSONResponse(
6888
status_code=exc.status_code,
69-
content={"detail": exc.detail},
89+
content={"message": exc.detail},
7090
)
7191
raise exc
7292
return FileResponse(os.path.join(studio_path(), "404.html"), status_code=404)

‎app/web_ui/src/lib/utils/task_sample_example.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ export async function fetch_task_sample_candidates(
7979
throw new Error(
8080
typeof error === "string"
8181
? error
82-
: (error as { detail?: string }).detail ?? "Failed to fetch runs",
82+
: (error as { message?: string }).message ?? "Failed to fetch runs",
8383
)
8484
}
8585

@@ -143,7 +143,7 @@ export async function build_prompt_with_task_sample(
143143
throw new Error(
144144
typeof error === "string"
145145
? error
146-
: (error as { detail?: string }).detail ?? "Failed to build prompt",
146+
: (error as { message?: string }).message ?? "Failed to build prompt",
147147
)
148148
}
149149

0 commit comments

Comments
 (0)