actix / actix-web

Actix Web is a powerful, pragmatic, and extremely fast web framework for Rust.
https://actix.rs
Apache License 2.0
21.6k stars 1.67k forks source link

JsonConfig limit has no effect #1608

Closed Ploppz closed 4 years ago

Ploppz commented 4 years ago

actix-web version: 3.0.0-alpha.3

My App starts with

    HttpServer::new(move || {
        App::new()
            .wrap(actix_web::middleware::Logger::new(
                "%a \"%r\" (size: %{Content-Length}i) Response: %s %b",
            ))
            .data(web::JsonConfig::default().limit(1024 * 1024 * 50))

This is the signature of the request handler I invoke:

pub async fn post(
    body: web::Json<NewAnalysisMethod>,
) -> Result<Json<AnalysisMethodId>, Error> {

This is what it logs when I send about 5MB of payload:

Jul 16 11:03:40.391 DEBG Failed to deserialize Json from payload. Request path: /api/v1/data/analysis_method
Jul 16 11:03:40.391 DEBG Error in response: Overflow
Jul 16 11:03:40.391 INFO 127.0.0.1:58804 "POST /api/v1/data/analysis_method HTTP/1.1" (size: 4915402) Response: 413 46

Where the format of the Logger middleware is set to "%a \"%r\" (size: %{Content-Length}i) Response: %s %b".

fakeshadow commented 4 years ago

This is expected behavior. src/types/json.rs Json extractor would only look for a JsonConfig type from the app_data. If you wrap the JsonConfig in App::data and it make it a Data<JsonConfig> type. So just wrap it directly with App::app_data and ignore the Data wrapper type and it would work.

This bring up a point tough. Should we offer a dumb proof check like this in the future for certain data types like

req.app_data::<T>()
     .or_else(||req.app_data::<Data<T>>().map(Data::get_ref))
robjtede commented 4 years ago

I feel trying both T and Data<T> be a good solution to the confusion with extractor configs.

Neopallium commented 4 years ago

@Ploppz Try using .app_data() instead of .data() for the JsonConfig:

    HttpServer::new(move || {
        App::new()
            .wrap(actix_web::middleware::Logger::new(
                "%a \"%r\" (size: %{Content-Length}i) Response: %s %b",
            ))
            .app_data(web::JsonConfig::default().limit(1024 * 1024 * 50))
robjtede commented 4 years ago

going to close this because since #1610 was merged