Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 37 additions & 6 deletions lib/llm/src/http/service/openai.rs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vLLM does not explicitly set this strictness check to True, but in recent versions of vLLM 0.26.0+ -- It uses a newer FastAPI version and inherits FastAPI’s default which is newly strict by default :

Image

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chanh just want to double check if we really want to allow empty content-type, rather than continue to reject it. It seems like latest vllm serve versions also reject it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and the PR's premise is wrong. I checked the FastAPI source rather than argue from memory, and it does not say what my description claims.

strict_content_type landed in FastAPI 0.132.0 and defaults to Default(True):

0.131.0  fastapi/routing.py: 0 occurrences
0.132.0  fastapi/routing.py: 17 occurrences   <- introduced here
0.136.1  fastapi/routing.py: 17
0.141.1  fastapi/routing.py: 26

vLLM 0.26.0 builds FastAPI(lifespan=lifespan) with no override, so it inherits the strict default, exactly as you said. My PR description asserts "the strict variant is opt-in and defaults off" — that was true before 0.132.0 and is false for every FastAPI version vLLM 0.26.0 accepts. The production behaviour I measured was against an older vLLM, and I generalised from it without checking the current default. My mistake.

One correction to both of us, though: under strict, FastAPI does not return 415. The enforcement is a fallthrough, not a rejection (routing.py 0.136.1, L409-426):

body_bytes = await request.body()
if body_bytes:
    json_body: Any = Undefined
    content_type_value = request.headers.get("content-type")
    if not content_type_value:
        if not actual_strict_content_type:
            json_body = await request.json()
    else:
        ...  # application/json or */*+json -> parse
    if json_body != Undefined:
        body = json_body
    else:
        body = body_bytes          # <- raw bytes, not an error

With strict on and no Content-Type, json_body stays Undefined, the body is handed to the model as raw bytes, and Pydantic validation fails — so vllm serve answers 422, not 415. Two further details that matter here:

  • if not content_type_value is falsy for the empty string as well, so FastAPI groups absent and empty identically. My PR added a separate "empty → accept" branch; the grouping is right, the outcome is not.
  • The check only fires when the body is non-empty, so an empty body with no Content-Type never reaches it.

So the honest picture is that neither current Dynamo nor this PR matches vLLM: Dynamo returns 415, vLLM returns 422, and this PR returns 200.

Given that, plus the CSRF rationale FastAPI documents for the default — a browser can send a no-Content-Type cross-origin POST with no CORS preflight — I don't think "accept it" is defensible. I'd rather not ship it.

Two options, and I'll take your call:

  1. Close this PR. Current behaviour is safe and stricter than vLLM's; the only divergence is 415 vs 422, which no client should be branching on.
  2. Narrow it to the status code: keep rejecting, but group empty with absent the way FastAPI does, and return 422 instead of 415 so a client migrating off vllm serve sees the same status. No change in what is accepted, only in how the rejection is reported.

I lean toward (1) — the divergence is cosmetic, and 415 is arguably the more correct status for this condition regardless of what FastAPI happens to do. Say the word and I'll close it, or push (2).

Either way I'll drop the "FastAPI treats an absent Content-Type as JSON" claim from the description so it doesn't mislead anyone reading the history.

Original file line number Diff line number Diff line change
Expand Up @@ -2003,12 +2003,17 @@ fn json_deserialize_error(error: serde_json::Error) -> ErrorResponse {
}

fn ensure_json_content_type(headers: &HeaderMap) -> Result<(), ErrorResponse> {
let Some(content_type) = headers
.get(axum::http::header::CONTENT_TYPE)
.and_then(|value| value.to_str().ok())
else {
// FastAPI treats an absent or empty Content-Type as JSON, so vLLM/SGLang accept these.
// A header that is present and asserts some other format still fails.
let Some(content_type) = headers.get(axum::http::header::CONTENT_TYPE) else {
return Ok(());
Comment thread
coderabbitai[bot] marked this conversation as resolved.
};
let Ok(content_type) = content_type.to_str() else {
return Err(unsupported_media_type_error());
};
if content_type.trim().is_empty() {
return Ok(());
}

if is_json_content_type(content_type) {
Ok(())
Expand Down Expand Up @@ -4638,10 +4643,36 @@ mod tests {
}

#[test]
fn test_ensure_json_content_type_rejects_missing_or_non_json() {
fn test_ensure_json_content_type_accepts_absent_header() {
let headers = HeaderMap::new();
let err = ensure_json_content_type(&headers).expect_err("missing content type should fail");
assert!(ensure_json_content_type(&headers).is_ok());
}

#[test]
fn test_ensure_json_content_type_accepts_empty_header() {
let mut headers = HeaderMap::new();
headers.insert(axum::http::header::CONTENT_TYPE, "".parse().unwrap());
assert!(ensure_json_content_type(&headers).is_ok());
}

#[test]
fn test_ensure_json_content_type_rejects_undecodable_header() {
let mut headers = HeaderMap::new();
headers.insert(
axum::http::header::CONTENT_TYPE,
axum::http::HeaderValue::from_bytes(&[0xff, 0xfe]).unwrap(),
);
let err = ensure_json_content_type(&headers).expect_err("undecodable must be rejected");
assert_eq!(err.0, StatusCode::UNSUPPORTED_MEDIA_TYPE);
}

#[test]
fn test_ensure_json_content_type_rejects_non_json() {
let headers = HeaderMap::new();
assert!(
ensure_json_content_type(&headers).is_ok(),
"an absent Content-Type is treated as JSON, matching FastAPI"
);

let mut headers = HeaderMap::new();
headers.insert(
Expand Down
Loading