From 43c362779dabb6f021d42e345d214aaea043adf9 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 20 Jul 2020 17:40:58 +0100 Subject: [PATCH] also try extracting payload config as Data (#1610) --- CHANGES.md | 5 ++ src/types/payload.rs | 133 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 116 insertions(+), 22 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fca46d1c..4b6697a7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,10 +1,15 @@ # Changes ## Unreleased - 2020-xx-xx +### Changed +* `PayloadConfig` is now also considered in `Bytes` and `String` extractors when set + using `App::data`. [#1610] + ### Fixed * Memory leak of app data in pooled requests. [#1609] [#1609]: https://github.com/actix/actix-web/pull/1609 +[#1610]: https://github.com/actix/actix-web/pull/1610 ## 3.0.0-beta.1 - 2020-07-13 diff --git a/src/types/payload.rs b/src/types/payload.rs index 0efdc2c0..653abf08 100644 --- a/src/types/payload.rs +++ b/src/types/payload.rs @@ -13,10 +13,10 @@ use futures_util::future::{err, ok, Either, FutureExt, LocalBoxFuture, Ready}; use futures_util::StreamExt; use mime::Mime; -use crate::dev; use crate::extract::FromRequest; use crate::http::header; use crate::request::HttpRequest; +use crate::{dev, web}; /// Payload extractor returns request 's payload stream. /// @@ -142,13 +142,8 @@ impl FromRequest for Bytes { #[inline] fn from_request(req: &HttpRequest, payload: &mut dev::Payload) -> Self::Future { - let tmp; - let cfg = if let Some(cfg) = req.app_data::() { - cfg - } else { - tmp = PayloadConfig::default(); - &tmp - }; + // allow both Config and Data + let cfg = PayloadConfig::from_req(req); if let Err(e) = cfg.check_mimetype(req) { return Either::Right(err(e)); @@ -197,13 +192,7 @@ impl FromRequest for String { #[inline] fn from_request(req: &HttpRequest, payload: &mut dev::Payload) -> Self::Future { - let tmp; - let cfg = if let Some(cfg) = req.app_data::() { - cfg - } else { - tmp = PayloadConfig::default(); - &tmp - }; + let cfg = PayloadConfig::from_req(req); // check content-type if let Err(e) = cfg.check_mimetype(req) { @@ -237,7 +226,12 @@ impl FromRequest for String { ) } } -/// Payload configuration for request's payload. + +/// Configuration for request's payload. +/// +/// Applies to the built-in `Bytes` and `String` extractors. Note that the Payload extractor does +/// not automatically check conformance with this configuration to allow more flexibility when +/// building extractors on top of `Payload`. #[derive(Clone)] pub struct PayloadConfig { limit: usize, @@ -284,14 +278,28 @@ impl PayloadConfig { } Ok(()) } + + /// Allow payload config extraction from app data checking both `T` and `Data`, in that + /// order, and falling back to the default payload config. + fn from_req(req: &HttpRequest) -> &PayloadConfig { + req.app_data::() + .or_else(|| { + req.app_data::>() + .map(|d| d.as_ref()) + }) + .unwrap_or_else(|| &DEFAULT_PAYLOAD_CONFIG) + } } +// Allow shared refs to default. +static DEFAULT_PAYLOAD_CONFIG: PayloadConfig = PayloadConfig { + limit: 262_144, // 2^18 bytes (~256kB) + mimetype: None, +}; + impl Default for PayloadConfig { fn default() -> Self { - PayloadConfig { - limit: 262_144, - mimetype: None, - } + DEFAULT_PAYLOAD_CONFIG.clone() } } @@ -407,8 +415,9 @@ mod tests { use bytes::Bytes; use super::*; - use crate::http::header; - use crate::test::TestRequest; + use crate::http::{header, StatusCode}; + use crate::test::{call_service, init_service, TestRequest}; + use crate::{web, App, Responder}; #[actix_rt::test] async fn test_payload_config() { @@ -428,6 +437,86 @@ mod tests { assert!(cfg.check_mimetype(&req).is_ok()); } + #[actix_rt::test] + async fn test_config_recall_locations() { + async fn bytes_handler(_: Bytes) -> impl Responder { + "payload is probably json bytes" + } + + async fn string_handler(_: String) -> impl Responder { + "payload is probably json string" + } + + let mut srv = init_service( + App::new() + .service( + web::resource("/bytes-app-data") + .app_data( + PayloadConfig::default().mimetype(mime::APPLICATION_JSON), + ) + .route(web::get().to(bytes_handler)), + ) + .service( + web::resource("/bytes-data") + .data(PayloadConfig::default().mimetype(mime::APPLICATION_JSON)) + .route(web::get().to(bytes_handler)), + ) + .service( + web::resource("/string-app-data") + .app_data( + PayloadConfig::default().mimetype(mime::APPLICATION_JSON), + ) + .route(web::get().to(string_handler)), + ) + .service( + web::resource("/string-data") + .data(PayloadConfig::default().mimetype(mime::APPLICATION_JSON)) + .route(web::get().to(string_handler)), + ), + ) + .await; + + let req = TestRequest::with_uri("/bytes-app-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/bytes-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/string-app-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/string-data").to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let req = TestRequest::with_uri("/bytes-app-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + let req = TestRequest::with_uri("/bytes-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + let req = TestRequest::with_uri("/string-app-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + let req = TestRequest::with_uri("/string-data") + .header(header::CONTENT_TYPE, mime::APPLICATION_JSON) + .to_request(); + let resp = call_service(&mut srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + } + #[actix_rt::test] async fn test_bytes() { let (req, mut pl) = TestRequest::with_header(header::CONTENT_LENGTH, "11")