Skip to content

Commit 72f91a4

Browse files
committed
fix(moonapi): the emitted OpenAPI path was not a template; enforce what a route declares.
The document keyed paths by the router's own `:id` syntax while declaring parameters `in: path`. OpenAPI requires the name to correspond to a template expression in the path, so every document with a path parameter was invalid and Swagger UI had nothing to substitute into. The router keeps `:id`; only the emitted key is templated. A route's `endpoint` descriptor was emitted into the document and then never checked, so a spec promising `id: integer` was a promise the framework did not keep. It is enforced before the handler now, answering 422 with the same body shape as every other validation error; `validate=false` opts a route out. An unrecognised verb was answered as a GET, running a handler the client never asked for. A 405 now carries `Allow`, a GET route answers HEAD with no body, and both 404 and 405 are JSON `{"detail": …}` like every other error rather than text. `enable_docs` served a document permanently titled "moonapi" — the metadata that already existed could not reach it. `App::describe` sets it once, and both the served route and `App::openapi` read it. Signed-off-by: Leo Cheng (heke1228) <chengkelfan@qq.com>
1 parent b66fb3b commit 72f91a4

7 files changed

Lines changed: 354 additions & 48 deletions

File tree

app.mbt

Lines changed: 122 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,32 @@ pub(all) enum Method {
1111
} derive(Eq)
1212

1313
///|
14-
fn parse_method(s : String) -> Method {
14+
fn parse_method(s : String) -> Method? {
1515
match s {
16-
"GET" => Get
17-
"POST" => Post
18-
"PUT" => Put
19-
"PATCH" => Patch
20-
"DELETE" => Delete
21-
"HEAD" => Head
22-
"OPTIONS" => Options
23-
_ => Get
16+
"GET" => Some(Get)
17+
"POST" => Some(Post)
18+
"PUT" => Some(Put)
19+
"PATCH" => Some(Patch)
20+
"DELETE" => Some(Delete)
21+
"HEAD" => Some(Head)
22+
"OPTIONS" => Some(Options)
23+
// TRACE, CONNECT, or something invented. Answering it as a GET would run a
24+
// handler the caller never asked for.
25+
_ => None
26+
}
27+
}
28+
29+
///|
30+
/// A method's name as it appears in an `Allow` header.
31+
fn method_name(m : Method) -> String {
32+
match m {
33+
Get => "GET"
34+
Post => "POST"
35+
Put => "PUT"
36+
Patch => "PATCH"
37+
Delete => "DELETE"
38+
Head => "HEAD"
39+
Options => "OPTIONS"
2440
}
2541
}
2642

@@ -82,6 +98,10 @@ struct Route {
8298
// FastAPI's `include_in_schema`: a route that serves the documentation itself has
8399
// no business appearing in the document it serves.
84100
include_in_schema : Bool
101+
// Whether the declared `endpoint` is checked before the handler runs. On by
102+
// default: a route that advertises constraints in its documentation and does not
103+
// apply them is worse than one that declares nothing.
104+
validate : Bool
85105
}
86106

87107
///|
@@ -103,6 +123,10 @@ pub struct App {
103123
// so a resource is torn down before whatever it was built on.
104124
startup_hooks : Array[() -> Unit raise]
105125
shutdown_hooks : Array[() -> Unit raise]
126+
// What the emitted document says about itself. `enable_docs` serves the same
127+
// values, so the page a reader opens is not a different API from the one
128+
// `App::openapi` describes.
129+
mut info : ApiInfo
106130
mut clock : () -> Int64
107131
}
108132

@@ -122,10 +146,38 @@ pub fn App::new() -> App {
122146
mounts: [],
123147
startup_hooks: [],
124148
shutdown_hooks: [],
149+
info: ApiInfo::new(),
125150
clock: () => 0L,
126151
}
127152
}
128153

154+
///|
155+
/// Set what the OpenAPI document says about this API (← FastAPI's `FastAPI(title=…,
156+
/// description=…, contact=…, license_info=…, servers=…)`). `App::openapi` and the
157+
/// `/openapi.json` route `enable_docs` registers both read it, so the document a
158+
/// client fetches and the one a test builds cannot describe different APIs.
159+
pub fn App::describe(
160+
self : App,
161+
title? : String,
162+
api_version? : String,
163+
description? : String,
164+
terms_of_service? : String,
165+
contact? : Contact?,
166+
license? : License?,
167+
servers? : Array[Server],
168+
) -> Unit {
169+
let cur = self.info
170+
self.info = {
171+
title: title.unwrap_or(cur.title),
172+
api_version: api_version.unwrap_or(cur.api_version),
173+
description: description.unwrap_or(cur.description),
174+
terms_of_service: terms_of_service.unwrap_or(cur.terms_of_service),
175+
contact: contact.unwrap_or(cur.contact),
176+
license: license.unwrap_or(cur.license),
177+
servers: servers.unwrap_or(cur.servers),
178+
}
179+
}
180+
129181
///|
130182
/// Run `hook` when the server starts, before it accepts the first request (←
131183
/// FastAPI's `on_event("startup")`). Hooks run in the order they were registered;
@@ -197,6 +249,7 @@ pub fn App::route(
197249
endpoint? : Endpoint? = None,
198250
security? : Array[SecurityRequirement] = [],
199251
include_in_schema? : Bool = true,
252+
validate? : Bool = true,
200253
) -> Unit {
201254
self.routes.push({
202255
verb,
@@ -208,6 +261,7 @@ pub fn App::route(
208261
endpoint,
209262
security,
210263
include_in_schema,
264+
validate,
211265
})
212266
}
213267

@@ -226,6 +280,7 @@ pub fn App::route_bg(
226280
endpoint? : Endpoint? = None,
227281
security? : Array[SecurityRequirement] = [],
228282
include_in_schema? : Bool = true,
283+
validate? : Bool = true,
229284
) -> Unit {
230285
self.routes.push({
231286
verb,
@@ -237,6 +292,7 @@ pub fn App::route_bg(
237292
endpoint,
238293
security,
239294
include_in_schema,
295+
validate,
240296
})
241297
}
242298

@@ -253,6 +309,7 @@ pub fn App::get(
253309
endpoint? : Endpoint? = None,
254310
security? : Array[SecurityRequirement] = [],
255311
include_in_schema? : Bool = true,
312+
validate? : Bool = true,
256313
) -> Unit {
257314
self.route(
258315
Get,
@@ -264,6 +321,7 @@ pub fn App::get(
264321
endpoint~,
265322
security~,
266323
include_in_schema~,
324+
validate~,
267325
)
268326
}
269327

@@ -282,8 +340,8 @@ pub fn App::enable_docs(
282340
docs_url? : String? = Some("/docs"),
283341
redoc_url? : String? = Some("/redoc"),
284342
version? : OpenApiVersion = OpenApi31,
285-
title? : String = "moonapi",
286343
) -> Unit {
344+
let title = self.info.title
287345
if openapi_url is Some(url) {
288346
self.get(
289347
url,
@@ -324,6 +382,7 @@ pub fn App::post(
324382
endpoint? : Endpoint? = None,
325383
security? : Array[SecurityRequirement] = [],
326384
include_in_schema? : Bool = true,
385+
validate? : Bool = true,
327386
) -> Unit {
328387
self.route(
329388
Post,
@@ -335,6 +394,7 @@ pub fn App::post(
335394
endpoint~,
336395
security~,
337396
include_in_schema~,
397+
validate~,
338398
)
339399
}
340400

@@ -350,6 +410,7 @@ pub fn App::put(
350410
endpoint? : Endpoint? = None,
351411
security? : Array[SecurityRequirement] = [],
352412
include_in_schema? : Bool = true,
413+
validate? : Bool = true,
353414
) -> Unit {
354415
self.route(
355416
Put,
@@ -361,6 +422,7 @@ pub fn App::put(
361422
endpoint~,
362423
security~,
363424
include_in_schema~,
425+
validate~,
364426
)
365427
}
366428

@@ -376,6 +438,7 @@ pub fn App::patch(
376438
endpoint? : Endpoint? = None,
377439
security? : Array[SecurityRequirement] = [],
378440
include_in_schema? : Bool = true,
441+
validate? : Bool = true,
379442
) -> Unit {
380443
self.route(
381444
Patch,
@@ -387,6 +450,7 @@ pub fn App::patch(
387450
endpoint~,
388451
security~,
389452
include_in_schema~,
453+
validate~,
390454
)
391455
}
392456

@@ -402,6 +466,7 @@ pub fn App::delete(
402466
endpoint? : Endpoint? = None,
403467
security? : Array[SecurityRequirement] = [],
404468
include_in_schema? : Bool = true,
469+
validate? : Bool = true,
405470
) -> Unit {
406471
self.route(
407472
Delete,
@@ -413,6 +478,7 @@ pub fn App::delete(
413478
endpoint~,
414479
security~,
415480
include_in_schema~,
481+
validate~,
416482
)
417483
}
418484

@@ -608,21 +674,38 @@ fn App::route_and_dispatch(
608674
bg : BackgroundTasks,
609675
) -> @moonasgi.Response {
610676
let verb = parse_method(request.http_method)
611-
let mut method_mismatch = false
677+
let allowed : Array[String] = []
612678
for route in self.routes {
613679
match match_path(route.path, request.path) {
614680
Some(params) =>
615-
if route.verb == verb {
681+
// A GET route answers HEAD too, with the body dropped — the same rule
682+
// Starlette applies, and what a client probing a resource expects.
683+
if verb is Some(v) &&
684+
(route.verb == v || (v is Head && route.verb is Get)) {
616685
let ctx : Context = { request, params, }
617686
match self.enforce_security(ctx, route.security) {
618687
Some(denied) => return self.apply_status(ctx, denied)
619688
None => ()
620689
}
621-
return (route.run)(ctx, bg) catch {
690+
if route.validate && route.endpoint is Some(ep) {
691+
let errs = ep.validate(ctx)
692+
if errs.length() > 0 {
693+
return self.apply_status(ctx, unprocessable(errs))
694+
}
695+
}
696+
let resp = (route.run)(ctx, bg) catch {
622697
e => self.dispatch_exception(ctx, e)
623698
}
699+
return if verb is Some(Head) && route.verb is Get {
700+
@moonasgi.Response::new(resp.status, resp.headers, b"")
701+
} else {
702+
resp
703+
}
624704
} else {
625-
method_mismatch = true
705+
allowed.push(method_name(route.verb))
706+
if route.verb is Get {
707+
allowed.push("HEAD")
708+
}
626709
}
627710
None => ()
628711
}
@@ -646,13 +729,35 @@ fn App::route_and_dispatch(
646729
}
647730
}
648731
let ctx : Context = { request, params: Map([]), }
649-
if method_mismatch {
650-
self.apply_status(ctx, text(405, "Method Not Allowed"))
732+
if allowed.length() > 0 {
733+
// RFC 9110 §15.5.6 requires the Allow header on a 405; a client that probes
734+
// a resource reads it rather than guessing.
735+
self.apply_status(
736+
ctx,
737+
http_exception_response(405, "Method Not Allowed".to_json(), [
738+
("allow", dedupe(allowed).join(", ")),
739+
]),
740+
)
651741
} else {
652-
self.apply_status(ctx, text(404, "Not Found"))
742+
self.apply_status(
743+
ctx,
744+
http_exception_response(404, "Not Found".to_json(), []),
745+
)
653746
}
654747
}
655748

749+
///|
750+
/// The distinct entries of `names`, in first-seen order.
751+
fn dedupe(names : Array[String]) -> Array[String] {
752+
let out : Array[String] = []
753+
for n in names {
754+
if !out.contains(n) {
755+
out.push(n)
756+
}
757+
}
758+
out
759+
}
760+
656761
///|
657762
/// Route a request through the middleware chain and return both the response and
658763
/// the background queue the handler filled — the caller (`to_asgi`, or a test)

0 commit comments

Comments
 (0)