From 3b6333e65f2da0c0ddf84f014088ef69c12dd2e2 Mon Sep 17 00:00:00 2001 From: Luca Palmieri Date: Tue, 22 Jun 2021 23:22:33 +0200 Subject: [PATCH 1/8] Propagate error cause to middlewares (#2280) --- src/response/response.rs | 5 +- tests/test_error_propagation.rs | 100 ++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 tests/test_error_propagation.rs diff --git a/src/response/response.rs b/src/response/response.rs index 9dd804be0..9a3bb2874 100644 --- a/src/response/response.rs +++ b/src/response/response.rs @@ -49,7 +49,10 @@ impl HttpResponse { /// Create an error response. #[inline] pub fn from_error(error: impl Into) -> Self { - error.into().as_response_error().error_response() + let error = error.into(); + let mut response = error.as_response_error().error_response(); + response.error = Some(error); + response } } diff --git a/tests/test_error_propagation.rs b/tests/test_error_propagation.rs new file mode 100644 index 000000000..1b56615a0 --- /dev/null +++ b/tests/test_error_propagation.rs @@ -0,0 +1,100 @@ +use actix_utils::future::{ok, Ready}; +use actix_web::dev::{Service, ServiceRequest, ServiceResponse, Transform}; +use actix_web::test::{call_service, init_service, TestRequest}; +use actix_web::{HttpResponse, ResponseError}; +use futures_util::lock::Mutex; +use std::future::Future; +use std::pin::Pin; +use std::sync::Arc; +use std::task::{Context, Poll}; + +#[derive(Debug, Clone)] +pub struct MyError; + +impl ResponseError for MyError {} + +impl std::fmt::Display for MyError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "A custom error") + } +} + +#[actix_web::get("/test")] +async fn test() -> Result { + Err(MyError)?; + Ok(HttpResponse::NoContent().finish()) +} + +#[derive(Clone)] +pub struct SpyMiddleware(Arc>>); + +impl Transform for SpyMiddleware +where + S: Service, Error = actix_web::Error>, + S::Future: 'static, + B: 'static, +{ + type Response = ServiceResponse; + type Error = actix_web::Error; + type Transform = Middleware; + type InitError = (); + type Future = Ready>; + + fn new_transform(&self, service: S) -> Self::Future { + ok(Middleware { + was_error: self.0.clone(), + service, + }) + } +} + +#[doc(hidden)] +pub struct Middleware { + was_error: Arc>>, + service: S, +} + +impl Service for Middleware +where + S: Service, Error = actix_web::Error>, + S::Future: 'static, + B: 'static, +{ + type Response = ServiceResponse; + type Error = actix_web::Error; + type Future = Pin>>>; + + fn poll_ready(&self, cx: &mut Context<'_>) -> Poll> { + self.service.poll_ready(cx) + } + + fn call(&self, req: ServiceRequest) -> Self::Future { + let lock = self.was_error.clone(); + let response_future = self.service.call(req); + Box::pin(async move { + let response = response_future.await; + if let Ok(success) = &response { + *lock.lock().await = Some(success.response().error().is_some()); + } + response + }) + } +} + +#[actix_rt::test] +async fn error_cause_should_be_propagated_to_middlewares() { + let lock = Arc::new(Mutex::new(None)); + let spy_middleware = SpyMiddleware(lock.clone()); + + let app = init_service( + actix_web::App::new() + .wrap(spy_middleware.clone()) + .service(test), + ) + .await; + + call_service(&app, TestRequest::with_uri("/test").to_request()).await; + + let was_error_captured = lock.lock().await.unwrap(); + assert!(was_error_captured); +} From 8846808804e2ac885a5a81b27817b365dfcd4c9a Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Tue, 22 Jun 2021 19:42:00 -0400 Subject: [PATCH 2/8] ServiceRequest::parts_mut (#2177) --- CHANGES.md | 2 ++ src/service.rs | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index b69b5a18c..31fa4690f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -2,6 +2,7 @@ ## Unreleased - 2021-xx-xx ### Added +* Add `ServiceRequest::parts_mut`. [#2177] * Add extractors for `Uri` and `Method`. [#2263] * Add extractor for `ConnectionInfo` and `PeerAddr`. [#2263] @@ -9,6 +10,7 @@ * Change compression algorithm features flags. [#2250] * Deprecate `App::data` and `App::data_factory`. [#2271] +[#2177]: https://github.com/actix/actix-web/pull/2177 [#2250]: https://github.com/actix/actix-web/pull/2250 [#2271]: https://github.com/actix/actix-web/pull/2271 [#2263]: https://github.com/actix/actix-web/pull/2263 diff --git a/src/service.rs b/src/service.rs index a772e20b7..8d73a87fa 100644 --- a/src/service.rs +++ b/src/service.rs @@ -80,6 +80,12 @@ impl ServiceRequest { (self.req, self.payload) } + /// Get mutable access to inner `HttpRequest` and `Payload` + #[inline] + pub fn parts_mut(&mut self) -> (&mut HttpRequest, &mut Payload) { + (&mut self.req, &mut self.payload) + } + /// Construct request from parts. pub fn from_parts(req: HttpRequest, payload: Payload) -> Self { Self { req, payload } From 7535a1ade8c1ee392ae1aa337821cf8f7c7c4c9a Mon Sep 17 00:00:00 2001 From: Jonas Malaco Date: Wed, 23 Jun 2021 12:54:25 -0300 Subject: [PATCH 3/8] Note that Form cannot require data ordering (#2283) --- src/types/form.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/types/form.rs b/src/types/form.rs index 44d1b952e..4ce075d99 100644 --- a/src/types/form.rs +++ b/src/types/form.rs @@ -80,6 +80,10 @@ use crate::{ /// }) /// } /// ``` +/// +/// # Panics +/// URL encoded forms consist of unordered `key=value` pairs, therefore they cannot be decoded into +/// any type which depends upon data ordering (eg. tuples). Trying to do so will result in a panic. #[derive(PartialEq, Eq, PartialOrd, Ord, Debug)] pub struct Form(pub T); From ed0516d724ed225b0b83c01f9278e47bd5f7f104 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Wed, 23 Jun 2021 20:47:17 +0100 Subject: [PATCH 4/8] try to fix doc test failures (#2284) --- .cargo/config.toml | 7 ++++--- .github/workflows/ci.yml | 36 ++++++++++++-------------------- .github/workflows/clippy-fmt.yml | 2 +- tests/test_server.rs | 2 +- 4 files changed, 19 insertions(+), 28 deletions(-) diff --git a/.cargo/config.toml b/.cargo/config.toml index 0cf09f710..72f445d8a 100644 --- a/.cargo/config.toml +++ b/.cargo/config.toml @@ -3,6 +3,7 @@ chk = "check --workspace --all-features --tests --examples --bins" lint = "clippy --workspace --tests --examples" ci-min = "hack check --workspace --no-default-features" ci-min-test = "hack check --workspace --no-default-features --tests --examples" -ci-default = "hack check --workspace" -ci-full = "check --workspace --bins --examples --tests" -ci-test = "test --workspace --all-features --no-fail-fast" +ci-default = "check --workspace --bins --tests --examples" +ci-full = "check --workspace --all-features --bins --tests --examples" +ci-test = "test --workspace --all-features --lib --tests --no-fail-fast -- --nocapture" +ci-doctest = "hack test --workspace --all-features --doc --no-fail-fast -- --nocapture" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c57db463a..be595e35c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -66,43 +66,33 @@ jobs: - name: check minimal uses: actions-rs/cargo@v1 - with: - command: hack - args: check --workspace --no-default-features + with: { command: ci-min } - name: check minimal + tests uses: actions-rs/cargo@v1 - with: - command: hack - args: check --workspace --no-default-features --tests --examples + with: { command: ci-min-test } + - name: check default + uses: actions-rs/cargo@v1 + with: { command: ci-default } + - name: check full uses: actions-rs/cargo@v1 - with: - command: check - args: --workspace --bins --examples --tests + with: { command: ci-full } - name: tests - uses: actions-rs/cargo@v1 - with: - command: test - args: --workspace --all-features --no-fail-fast -- --nocapture - --skip=test_h2_content_length - --skip=test_reading_deflate_encoding_large_random_rustls - - - name: tests (actix-http) uses: actions-rs/cargo@v1 timeout-minutes: 40 with: - command: test - args: --package=actix-http --no-default-features --features=rustls -- --nocapture + command: ci-test + args: --skip=test_reading_deflate_encoding_large_random_rustls - - name: tests (awc) + - name: doc tests + # due to unknown issue with running doc tests on macOS + if: matrix.target.os == 'ubuntu-latest' uses: actions-rs/cargo@v1 timeout-minutes: 40 - with: - command: test - args: --package=awc --no-default-features --features=rustls -- --nocapture + with: { command: ci-doctest } - name: Generate coverage file if: > diff --git a/.github/workflows/clippy-fmt.yml b/.github/workflows/clippy-fmt.yml index e966fa4ab..957256d32 100644 --- a/.github/workflows/clippy-fmt.yml +++ b/.github/workflows/clippy-fmt.yml @@ -36,4 +36,4 @@ jobs: uses: actions-rs/clippy-check@v1 with: token: ${{ secrets.GITHUB_TOKEN }} - args: --workspace --tests --all-features + args: --workspace --all-features --tests diff --git a/tests/test_server.rs b/tests/test_server.rs index 9131d1f29..afea39dd9 100644 --- a/tests/test_server.rs +++ b/tests/test_server.rs @@ -879,7 +879,7 @@ async fn test_brotli_encoding_large_openssl() { assert_eq!(bytes, Bytes::from(data)); } -#[cfg(all(feature = "rustls", feature = "openssl"))] +#[cfg(feature = "rustls")] mod plus_rustls { use std::io::BufReader; From 083ee05d50519897db96fb1873b6eebd5f6df186 Mon Sep 17 00:00:00 2001 From: Ibraheem Ahmed Date: Wed, 23 Jun 2021 16:30:06 -0400 Subject: [PATCH 5/8] `Route::service` (#2262) Co-authored-by: Rob Ede --- CHANGES.md | 4 +- Cargo.toml | 1 - src/extract.rs | 8 ++-- src/lib.rs | 4 +- src/request.rs | 2 +- src/route.rs | 122 +++++++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 128 insertions(+), 13 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 31fa4690f..876e1c03d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,7 +4,8 @@ ### Added * Add `ServiceRequest::parts_mut`. [#2177] * Add extractors for `Uri` and `Method`. [#2263] -* Add extractor for `ConnectionInfo` and `PeerAddr`. [#2263] +* Add extractors for `ConnectionInfo` and `PeerAddr`. [#2263] +* Add `Route::service` for using hand-written services as handlers. [#2262] ### Changed * Change compression algorithm features flags. [#2250] @@ -13,6 +14,7 @@ [#2177]: https://github.com/actix/actix-web/pull/2177 [#2250]: https://github.com/actix/actix-web/pull/2250 [#2271]: https://github.com/actix/actix-web/pull/2271 +[#2262]: https://github.com/actix/actix-web/pull/2262 [#2263]: https://github.com/actix/actix-web/pull/2263 diff --git a/Cargo.toml b/Cargo.toml index 320751c66..779b52255 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -113,7 +113,6 @@ flate2 = "1.0.13" zstd = "0.7" rand = "0.8" rcgen = "0.8" -serde_derive = "1.0" tls-openssl = { package = "openssl", version = "0.10.9" } tls-rustls = { package = "rustls", version = "0.19.0" } diff --git a/src/extract.rs b/src/extract.rs index d7b67cd90..592f7ab83 100644 --- a/src/extract.rs +++ b/src/extract.rs @@ -54,7 +54,7 @@ pub trait FromRequest: Sized { /// use actix_web::{web, dev, App, Error, HttpRequest, FromRequest}; /// use actix_web::error::ErrorBadRequest; /// use futures_util::future::{ok, err, Ready}; -/// use serde_derive::Deserialize; +/// use serde::Deserialize; /// use rand; /// /// #[derive(Debug, Deserialize)] @@ -145,7 +145,7 @@ where /// use actix_web::{web, dev, App, Result, Error, HttpRequest, FromRequest}; /// use actix_web::error::ErrorBadRequest; /// use futures_util::future::{ok, err, Ready}; -/// use serde_derive::Deserialize; +/// use serde::Deserialize; /// use rand; /// /// #[derive(Debug, Deserialize)] @@ -265,7 +265,7 @@ impl FromRequest for Method { #[doc(hidden)] impl FromRequest for () { type Error = Infallible; - type Future = Ready>; + type Future = Ready>; type Config = (); fn from_request(_: &HttpRequest, _: &mut Payload) -> Self::Future { @@ -376,7 +376,7 @@ mod m { mod tests { use actix_http::http::header; use bytes::Bytes; - use serde_derive::Deserialize; + use serde::Deserialize; use super::*; use crate::test::TestRequest; diff --git a/src/lib.rs b/src/lib.rs index 9d8bf62e7..920abccb6 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -149,7 +149,9 @@ pub mod dev { pub use actix_http::{Extensions, Payload, PayloadStream, RequestHead, ResponseHead}; pub use actix_router::{Path, ResourceDef, ResourcePath, Url}; pub use actix_server::Server; - pub use actix_service::{always_ready, forward_ready, Service, Transform}; + pub use actix_service::{ + always_ready, fn_factory, fn_service, forward_ready, Service, Transform, + }; pub(crate) fn insert_slash(mut patterns: Vec) -> Vec { for path in &mut patterns { diff --git a/src/request.rs b/src/request.rs index bff66f08e..5c5c43d26 100644 --- a/src/request.rs +++ b/src/request.rs @@ -347,7 +347,7 @@ impl Drop for HttpRequest { /// # Examples /// ``` /// use actix_web::{web, App, HttpRequest}; -/// use serde_derive::Deserialize; +/// use serde::Deserialize; /// /// /// extract `Thing` from request /// async fn index(req: HttpRequest) -> String { diff --git a/src/route.rs b/src/route.rs index 44f7e30b8..d85b940bd 100644 --- a/src/route.rs +++ b/src/route.rs @@ -5,7 +5,7 @@ use std::{future::Future, rc::Rc}; use actix_http::http::Method; use actix_service::{ boxed::{self, BoxService, BoxServiceFactory}, - Service, ServiceFactory, + Service, ServiceFactory, ServiceFactoryExt, }; use futures_core::future::LocalBoxFuture; @@ -128,9 +128,10 @@ impl Route { /// Set handler function, use request extractors for parameters. /// + /// # Examples /// ``` /// use actix_web::{web, http, App}; - /// use serde_derive::Deserialize; + /// use serde::Deserialize; /// /// #[derive(Deserialize)] /// struct Info { @@ -154,7 +155,7 @@ impl Route { /// /// ``` /// # use std::collections::HashMap; - /// # use serde_derive::Deserialize; + /// # use serde::Deserialize; /// use actix_web::{web, App}; /// /// #[derive(Deserialize)] @@ -184,6 +185,53 @@ impl Route { self.service = boxed::factory(HandlerService::new(handler)); self } + + /// Set raw service to be constructed and called as the request handler. + /// + /// # Examples + /// ``` + /// # use std::convert::Infallible; + /// # use futures_util::future::LocalBoxFuture; + /// # use actix_web::{*, dev::*, http::header}; + /// struct HelloWorld; + /// + /// impl Service for HelloWorld { + /// type Response = ServiceResponse; + /// type Error = Infallible; + /// type Future = LocalBoxFuture<'static, Result>; + /// + /// always_ready!(); + /// + /// fn call(&self, req: ServiceRequest) -> Self::Future { + /// let (req, _) = req.into_parts(); + /// + /// let res = HttpResponse::Ok() + /// .insert_header(header::ContentType::plaintext()) + /// .body("Hello world!"); + /// + /// Box::pin(async move { Ok(ServiceResponse::new(req, res)) }) + /// } + /// } + /// + /// App::new().route( + /// "/", + /// web::get().service(fn_factory(|| async { Ok(HelloWorld) })), + /// ); + /// ``` + pub fn service(mut self, service_factory: S) -> Self + where + S: ServiceFactory< + ServiceRequest, + Response = ServiceResponse, + Error = E, + InitError = (), + Config = (), + > + 'static, + E: Into + 'static, + { + self.service = boxed::factory(service_factory.map_err(Into::into)); + self + } } #[cfg(test)] @@ -192,9 +240,12 @@ mod tests { use actix_rt::time::sleep; use bytes::Bytes; - use serde_derive::Serialize; + use futures_core::future::LocalBoxFuture; + use serde::Serialize; - use crate::http::{Method, StatusCode}; + use crate::dev::{always_ready, fn_factory, fn_service, Service}; + use crate::http::{header, Method, StatusCode}; + use crate::service::{ServiceRequest, ServiceResponse}; use crate::test::{call_service, init_service, read_body, TestRequest}; use crate::{error, web, App, HttpResponse}; @@ -268,4 +319,65 @@ mod tests { let body = read_body(resp).await; assert_eq!(body, Bytes::from_static(b"{\"name\":\"test\"}")); } + + #[actix_rt::test] + async fn test_service_handler() { + struct HelloWorld; + + impl Service for HelloWorld { + type Response = ServiceResponse; + type Error = crate::Error; + type Future = LocalBoxFuture<'static, Result>; + + always_ready!(); + + fn call(&self, req: ServiceRequest) -> Self::Future { + let (req, _) = req.into_parts(); + + let res = HttpResponse::Ok() + .insert_header(header::ContentType::plaintext()) + .body("Hello world!"); + + Box::pin(async move { Ok(ServiceResponse::new(req, res)) }) + } + } + + let srv = init_service( + App::new() + .route( + "/hello", + web::get().service(fn_factory(|| async { Ok(HelloWorld) })), + ) + .route( + "/bye", + web::get().service(fn_factory(|| async { + Ok::<_, ()>(fn_service(|req: ServiceRequest| async { + let (req, _) = req.into_parts(); + + let res = HttpResponse::Ok() + .insert_header(header::ContentType::plaintext()) + .body("Goodbye, and thanks for all the fish!"); + + Ok::<_, Infallible>(ServiceResponse::new(req, res)) + })) + })), + ), + ) + .await; + + let req = TestRequest::get().uri("/hello").to_request(); + let resp = call_service(&srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + let body = read_body(resp).await; + assert_eq!(body, Bytes::from_static(b"Hello world!")); + + let req = TestRequest::get().uri("/bye").to_request(); + let resp = call_service(&srv, req).await; + assert_eq!(resp.status(), StatusCode::OK); + let body = read_body(resp).await; + assert_eq!( + body, + Bytes::from_static(b"Goodbye, and thanks for all the fish!") + ); + } } From 2d8d2f5ab08c7db6ccfeba5a15a5085165a06ed3 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Thu, 24 Jun 2021 15:10:51 +0100 Subject: [PATCH 6/8] app data doc improvements --- src/app.rs | 85 +++++++++++++++++++----------- src/app_service.rs | 9 +++- src/config.rs | 11 ++-- src/data.rs | 5 ++ src/resource.rs | 76 ++++++++++++++------------- src/scope.rs | 128 +++++++++++++++++++++++---------------------- src/service.rs | 3 +- src/web.rs | 1 - 8 files changed, 181 insertions(+), 137 deletions(-) diff --git a/src/app.rs b/src/app.rs index 8c622dd36..677f73805 100644 --- a/src/app.rs +++ b/src/app.rs @@ -68,36 +68,71 @@ where InitError = (), >, { - /// Set application data. Application data could be accessed - /// by using `Data` extractor where `T` is data type. + /// Set application (root level) data. /// - /// **Note**: HTTP server accepts an application factory rather than - /// an application instance. Http server constructs an application - /// instance for each thread, thus application data must be constructed - /// multiple times. If you want to share data between different - /// threads, a shared object should be used, e.g. `Arc`. Internally `Data` type - /// uses `Arc` so data could be created outside of app factory and clones could - /// be stored via `App::app_data()` method. + /// Application data stored with `App::app_data()` method is available through the + /// [`HttpRequest::app_data`](crate::HttpRequest::app_data) method at runtime. + /// + /// # [`Data`] + /// Any [`Data`] type added here can utilize it's extractor implementation in handlers. + /// Types not wrapped in `Data` cannot use this extractor. See [its docs](Data) for more + /// about its usage and patterns. /// /// ``` /// use std::cell::Cell; - /// use actix_web::{web, App, HttpResponse, Responder}; + /// use actix_web::{web, App, HttpRequest, HttpResponse, Responder}; /// /// struct MyData { - /// counter: Cell, + /// count: std::cell::Cell, /// } /// - /// async fn index(data: web::Data) -> impl Responder { - /// data.counter.set(data.counter.get() + 1); - /// HttpResponse::Ok() + /// async fn handler(req: HttpRequest, counter: web::Data) -> impl Responder { + /// // note this cannot use the Data extractor because it was not added with it + /// let incr = *req.app_data::().unwrap(); + /// assert_eq!(incr, 3); + /// + /// // update counter using other value from app data + /// counter.count.set(counter.count.get() + incr); + /// + /// HttpResponse::Ok().body(counter.count.get().to_string()) /// } /// - /// let app = App::new() - /// .data(MyData{ counter: Cell::new(0) }) - /// .service( - /// web::resource("/index.html").route( - /// web::get().to(index))); + /// let app = App::new().service( + /// web::resource("/") + /// .app_data(3usize) + /// .app_data(web::Data::new(MyData { count: Default::default() })) + /// .route(web::get().to(handler)) + /// ); /// ``` + /// + /// # Shared Mutable State + /// [`HttpServer::new`](crate::HttpServer::new) accepts an application factory rather than an + /// application instance; the factory closure is called on each worker thread independently. + /// Therefore, if you want to share a data object between different workers, a shareable object + /// needs to be created first, outside the `HttpServer::new` closure and cloned into it. + /// [`Data`] is an example of such a sharable object. + /// + /// ```ignore + /// let counter = web::Data::new(AppStateWithCounter { + /// counter: Mutex::new(0), + /// }); + /// + /// HttpServer::new(move || { + /// // move counter object into the closure and clone for each worker + /// + /// App::new() + /// .app_data(counter.clone()) + /// .route("/", web::get().to(handler)) + /// }) + /// ``` + pub fn app_data(mut self, ext: U) -> Self { + self.extensions.insert(ext); + self + } + + /// Add application (root) data after wrapping in `Data`. + /// + /// Deprecated in favor of [`app_data`](Self::app_data). #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")] pub fn data(self, data: U) -> Self { self.app_data(Data::new(data)) @@ -138,18 +173,6 @@ where self } - /// Set application level arbitrary data item. - /// - /// Application data stored with `App::app_data()` method is available - /// via `HttpRequest::app_data()` method at runtime. - /// - /// This method could be used for storing `Data` as well, in that case - /// data could be accessed by using `Data` extractor. - pub fn app_data(mut self, ext: U) -> Self { - self.extensions.insert(ext); - self - } - /// Run external configuration as part of the application building /// process /// diff --git a/src/app_service.rs b/src/app_service.rs index a9247b19f..feb9f22a4 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -144,7 +144,9 @@ where } } -/// Service that takes a [`Request`] and delegates to a service that take a [`ServiceRequest`]. +/// The [`Service`] that is passed to `actix-http`'s server builder. +/// +/// Wraps a service receiving a [`ServiceRequest`] into one receiving a [`Request`]. pub struct AppInitService where T: Service, Error = Error>, @@ -275,6 +277,7 @@ impl ServiceFactory for AppRoutingFactory { } } +/// The Actix Web router default entry point. pub struct AppRouting { router: Router, default: HttpService, @@ -299,6 +302,10 @@ impl Service for AppRouting { true }); + // you might expect to find `req.add_data_container()` called here but `HttpRequest` objects + // are created with the root data already set (in `AppInitService::call`) and root data is + // retained when releasing requests back to the pool + if let Some((srv, _info)) = res { srv.call(req) } else { diff --git a/src/config.rs b/src/config.rs index d22bc856e..966141193 100644 --- a/src/config.rs +++ b/src/config.rs @@ -62,6 +62,8 @@ impl AppService { (self.config, self.services) } + /// Clones inner config and default service, returning new `AppService` with empty service list + /// marked as non-root. pub(crate) fn clone_config(&self) -> Self { AppService { config: self.config.clone(), @@ -71,12 +73,12 @@ impl AppService { } } - /// Service configuration + /// Returns reference to configuration. pub fn config(&self) -> &AppConfig { &self.config } - /// Default resource + /// Returns default handler factory. pub fn default_service(&self) -> Rc { self.default.clone() } @@ -116,7 +118,7 @@ impl AppConfig { AppConfig { secure, host, addr } } - /// Needed in actix-test crate. + /// Needed in actix-test crate. Semver exempt. #[doc(hidden)] pub fn __priv_test_new(secure: bool, host: String, addr: SocketAddr) -> Self { AppConfig::new(secure, host, addr) @@ -192,6 +194,7 @@ impl ServiceConfig { /// Add shared app data item. /// /// Counterpart to [`App::data()`](crate::App::data). + #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")] pub fn data(&mut self, data: U) -> &mut Self { self.app_data(Data::new(data)); self @@ -257,6 +260,8 @@ mod tests { use crate::test::{call_service, init_service, read_body, TestRequest}; use crate::{web, App, HttpRequest, HttpResponse}; + // allow deprecated `ServiceConfig::data` + #[allow(deprecated)] #[actix_rt::test] async fn test_data() { let cfg = |cfg: &mut ServiceConfig| { diff --git a/src/data.rs b/src/data.rs index 51db6ce4c..174faba37 100644 --- a/src/data.rs +++ b/src/data.rs @@ -36,6 +36,11 @@ pub(crate) type FnDataFactory = /// If route data is not set for a handler, using `Data` extractor would cause *Internal /// Server Error* response. /// +// TODO: document `dyn T` functionality through converting an Arc +// TODO: note equivalence of req.app_data> and Data extractor +// TODO: note that data must be inserted using Data in order to extract it +/// +/// # Examples /// ``` /// use std::sync::Mutex; /// use actix_web::{web, App, HttpResponse, Responder}; diff --git a/src/resource.rs b/src/resource.rs index 9455895e9..7a4c1248b 100644 --- a/src/resource.rs +++ b/src/resource.rs @@ -169,41 +169,38 @@ where self } - /// Provide resource specific data. This method allows to add extractor - /// configuration or specific state available via `Data` extractor. - /// Provided data is available for all routes registered for the current resource. - /// Resource data overrides data registered by `App::data()` method. - /// - /// ``` - /// use actix_web::{web, App, FromRequest}; - /// - /// /// extract text data from request - /// async fn index(body: String) -> String { - /// format!("Body {}!", body) - /// } - /// - /// fn main() { - /// let app = App::new().service( - /// web::resource("/index.html") - /// // limit size of the payload - /// .data(String::configure(|cfg| { - /// cfg.limit(4096) - /// })) - /// .route( - /// web::get() - /// // register handler - /// .to(index) - /// )); - /// } - /// ``` - #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")] - pub fn data(self, data: U) -> Self { - self.app_data(Data::new(data)) - } - /// Add resource data. /// - /// Data of different types from parent contexts will still be accessible. + /// Data of different types from parent contexts will still be accessible. Any `Data` types + /// set here can be extracted in handlers using the `Data` extractor. + /// + /// # Examples + /// ``` + /// use std::cell::Cell; + /// use actix_web::{web, App, HttpRequest, HttpResponse, Responder}; + /// + /// struct MyData { + /// count: std::cell::Cell, + /// } + /// + /// async fn handler(req: HttpRequest, counter: web::Data) -> impl Responder { + /// // note this cannot use the Data extractor because it was not added with it + /// let incr = *req.app_data::().unwrap(); + /// assert_eq!(incr, 3); + /// + /// // update counter using other value from app data + /// counter.count.set(counter.count.get() + incr); + /// + /// HttpResponse::Ok().body(counter.count.get().to_string()) + /// } + /// + /// let app = App::new().service( + /// web::resource("/") + /// .app_data(3usize) + /// .app_data(web::Data::new(MyData { count: Default::default() })) + /// .route(web::get().to(handler)) + /// ); + /// ``` pub fn app_data(mut self, data: U) -> Self { self.app_data .get_or_insert_with(Extensions::new) @@ -212,6 +209,14 @@ where self } + /// Add resource data after wrapping in `Data`. + /// + /// Deprecated in favor of [`app_data`](Self::app_data). + #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")] + pub fn data(self, data: U) -> Self { + self.app_data(Data::new(data)) + } + /// Register a new route and add handler. This route matches all requests. /// /// ``` @@ -227,7 +232,6 @@ where /// This is shortcut for: /// /// ``` - /// # extern crate actix_web; /// # use actix_web::*; /// # fn index(req: HttpRequest) -> HttpResponse { unimplemented!() } /// App::new().service(web::resource("/").route(web::route().to(index))); @@ -695,7 +699,7 @@ mod tests { assert_eq!(resp.status(), StatusCode::NO_CONTENT); } - // allow deprecated App::data + // allow deprecated `{App, Resource}::data` #[allow(deprecated)] #[actix_rt::test] async fn test_data() { @@ -729,7 +733,7 @@ mod tests { assert_eq!(resp.status(), StatusCode::OK); } - // allow deprecated App::data + // allow deprecated `{App, Resource}::data` #[allow(deprecated)] #[actix_rt::test] async fn test_data_default_service() { diff --git a/src/scope.rs b/src/scope.rs index 86304074b..0caf06ee3 100644 --- a/src/scope.rs +++ b/src/scope.rs @@ -1,28 +1,23 @@ -use std::cell::RefCell; -use std::fmt; -use std::future::Future; -use std::rc::Rc; +use std::{cell::RefCell, fmt, future::Future, mem, rc::Rc}; use actix_http::Extensions; use actix_router::{ResourceDef, Router}; -use actix_service::boxed::{self, BoxService, BoxServiceFactory}; use actix_service::{ - apply, apply_fn_factory, IntoServiceFactory, Service, ServiceFactory, ServiceFactoryExt, - Transform, + apply, apply_fn_factory, + boxed::{self, BoxService, BoxServiceFactory}, + IntoServiceFactory, Service, ServiceFactory, ServiceFactoryExt, Transform, }; use futures_core::future::LocalBoxFuture; use futures_util::future::join_all; -use crate::config::ServiceConfig; -use crate::data::Data; -use crate::dev::{AppService, HttpServiceFactory}; -use crate::error::Error; -use crate::guard::Guard; -use crate::resource::Resource; -use crate::rmap::ResourceMap; -use crate::route::Route; -use crate::service::{ - AppServiceFactory, ServiceFactoryWrapper, ServiceRequest, ServiceResponse, +use crate::{ + config::ServiceConfig, + data::Data, + dev::{AppService, HttpServiceFactory}, + guard::Guard, + rmap::ResourceMap, + service::{AppServiceFactory, ServiceFactoryWrapper, ServiceRequest, ServiceResponse}, + Error, Resource, Route, }; type Guards = Vec>; @@ -71,16 +66,17 @@ pub struct Scope { impl Scope { /// Create a new scope pub fn new(path: &str) -> Scope { - let fref = Rc::new(RefCell::new(None)); + let factory_ref = Rc::new(RefCell::new(None)); + Scope { - endpoint: ScopeEndpoint::new(fref.clone()), + endpoint: ScopeEndpoint::new(Rc::clone(&factory_ref)), rdef: path.to_string(), app_data: None, guards: Vec::new(), services: Vec::new(), default: None, external: Vec::new(), - factory_ref: fref, + factory_ref, } } } @@ -120,40 +116,38 @@ where self } - /// Set or override application data. Application data could be accessed - /// by using `Data` extractor where `T` is data type. - /// - /// ``` - /// use std::cell::Cell; - /// use actix_web::{web, App, HttpResponse, Responder}; - /// - /// struct MyData { - /// counter: Cell, - /// } - /// - /// async fn index(data: web::Data) -> impl Responder { - /// data.counter.set(data.counter.get() + 1); - /// HttpResponse::Ok() - /// } - /// - /// fn main() { - /// let app = App::new().service( - /// web::scope("/app") - /// .data(MyData{ counter: Cell::new(0) }) - /// .service( - /// web::resource("/index.html").route( - /// web::get().to(index))) - /// ); - /// } - /// ``` - #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")] - pub fn data(self, data: U) -> Self { - self.app_data(Data::new(data)) - } - /// Add scope data. /// - /// Data of different types from parent contexts will still be accessible. + /// Data of different types from parent contexts will still be accessible. Any `Data` types + /// set here can be extracted in handlers using the `Data` extractor. + /// + /// # Examples + /// ``` + /// use std::cell::Cell; + /// use actix_web::{web, App, HttpRequest, HttpResponse, Responder}; + /// + /// struct MyData { + /// count: std::cell::Cell, + /// } + /// + /// async fn handler(req: HttpRequest, counter: web::Data) -> impl Responder { + /// // note this cannot use the Data extractor because it was not added with it + /// let incr = *req.app_data::().unwrap(); + /// assert_eq!(incr, 3); + /// + /// // update counter using other value from app data + /// counter.count.set(counter.count.get() + incr); + /// + /// HttpResponse::Ok().body(counter.count.get().to_string()) + /// } + /// + /// let app = App::new().service( + /// web::scope("/app") + /// .app_data(3usize) + /// .app_data(web::Data::new(MyData { count: Default::default() })) + /// .route("/", web::get().to(handler)) + /// ); + /// ``` pub fn app_data(mut self, data: U) -> Self { self.app_data .get_or_insert_with(Extensions::new) @@ -162,15 +156,20 @@ where self } - /// Run external configuration as part of the scope building - /// process + /// Add scope data after wrapping in `Data`. /// - /// This function is useful for moving parts of configuration to a - /// different module or even library. For example, - /// some of the resource's configuration could be moved to different module. + /// Deprecated in favor of [`app_data`](Self::app_data). + #[deprecated(since = "4.0.0", note = "Use `.app_data(Data::new(val))` instead.")] + pub fn data(self, data: U) -> Self { + self.app_data(Data::new(data)) + } + + /// Run external configuration as part of the scope building process. + /// + /// This function is useful for moving parts of configuration to a different module or library. + /// For example, some of the resource's configuration could be moved to different module. /// /// ``` - /// # extern crate actix_web; /// use actix_web::{web, middleware, App, HttpResponse}; /// /// // this function could be located in different module @@ -191,18 +190,21 @@ where /// .route("/index.html", web::get().to(|| HttpResponse::Ok())); /// } /// ``` - pub fn configure(mut self, f: F) -> Self + pub fn configure(mut self, cfg_fn: F) -> Self where F: FnOnce(&mut ServiceConfig), { let mut cfg = ServiceConfig::new(); - f(&mut cfg); + cfg_fn(&mut cfg); + self.services.extend(cfg.services); self.external.extend(cfg.external); + // TODO: add Extensions::is_empty check and conditionally insert data self.app_data .get_or_insert_with(Extensions::new) .extend(cfg.app_data); + self } @@ -419,7 +421,7 @@ where let mut rmap = ResourceMap::new(ResourceDef::root_prefix(&self.rdef)); // external resources - for mut rdef in std::mem::take(&mut self.external) { + for mut rdef in mem::take(&mut self.external) { rmap.add(&mut rdef, None); } @@ -991,7 +993,7 @@ mod tests { ); } - // allow deprecated App::data + // allow deprecated {App, Scope}::data #[allow(deprecated)] #[actix_rt::test] async fn test_override_data() { @@ -1011,7 +1013,7 @@ mod tests { assert_eq!(resp.status(), StatusCode::OK); } - // allow deprecated App::data + // allow deprecated `{App, Scope}::data` #[allow(deprecated)] #[actix_rt::test] async fn test_override_data_default_service() { diff --git a/src/service.rs b/src/service.rs index 8d73a87fa..592577467 100644 --- a/src/service.rs +++ b/src/service.rs @@ -17,9 +17,8 @@ use crate::{ dev::insert_slash, guard::Guard, info::ConnectionInfo, - request::HttpRequest, rmap::ResourceMap, - Error, HttpResponse, + Error, HttpRequest, HttpResponse, }; pub trait HttpServiceFactory { diff --git a/src/web.rs b/src/web.rs index 8662848a4..40ac46275 100644 --- a/src/web.rs +++ b/src/web.rs @@ -43,7 +43,6 @@ pub use crate::types::*; /// the exposed `Params` object: /// /// ``` -/// # extern crate actix_web; /// use actix_web::{web, App, HttpResponse}; /// /// let app = App::new().service( From 93aa86e30bd2bc664aac80d7f6e02769f9a64750 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Thu, 24 Jun 2021 15:11:01 +0100 Subject: [PATCH 7/8] clippy --- awc/src/request.rs | 7 ++++++- tests/test_error_propagation.rs | 29 ++++++++++++++--------------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/awc/src/request.rs b/awc/src/request.rs index 46dae7fa3..3f312f6e7 100644 --- a/awc/src/request.rs +++ b/awc/src/request.rs @@ -480,6 +480,7 @@ impl ClientRequest { // supported, so we cannot guess Accept-Encoding HTTP header. if slf.response_decompress { // Set Accept-Encoding with compression algorithm awc is built with. + #[allow(clippy::vec_init_then_push)] #[cfg(feature = "__compress")] let accept_encoding = { let mut encoding = vec![]; @@ -496,7 +497,11 @@ impl ClientRequest { #[cfg(feature = "compress-zstd")] encoding.push("zstd"); - assert!(!encoding.is_empty(), "encoding cannot be empty unless __compress feature has been explictily enabled."); + assert!( + !encoding.is_empty(), + "encoding can not be empty unless __compress feature has been explicitly enabled" + ); + encoding.join(", ") }; diff --git a/tests/test_error_propagation.rs b/tests/test_error_propagation.rs index 1b56615a0..3e7320920 100644 --- a/tests/test_error_propagation.rs +++ b/tests/test_error_propagation.rs @@ -1,12 +1,14 @@ -use actix_utils::future::{ok, Ready}; -use actix_web::dev::{Service, ServiceRequest, ServiceResponse, Transform}; -use actix_web::test::{call_service, init_service, TestRequest}; -use actix_web::{HttpResponse, ResponseError}; -use futures_util::lock::Mutex; -use std::future::Future; -use std::pin::Pin; use std::sync::Arc; -use std::task::{Context, Poll}; + +use actix_utils::future::{ok, Ready}; +use actix_web::{ + dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform}, + get, + test::{call_service, init_service, TestRequest}, + ResponseError, +}; +use futures_core::future::LocalBoxFuture; +use futures_util::lock::Mutex; #[derive(Debug, Clone)] pub struct MyError; @@ -19,10 +21,9 @@ impl std::fmt::Display for MyError { } } -#[actix_web::get("/test")] +#[get("/test")] async fn test() -> Result { - Err(MyError)?; - Ok(HttpResponse::NoContent().finish()) + return Err(MyError.into()); } #[derive(Clone)] @@ -62,11 +63,9 @@ where { type Response = ServiceResponse; type Error = actix_web::Error; - type Future = Pin>>>; + type Future = LocalBoxFuture<'static, Result>; - fn poll_ready(&self, cx: &mut Context<'_>) -> Poll> { - self.service.poll_ready(cx) - } + forward_ready!(service); fn call(&self, req: ServiceRequest) -> Self::Future { let lock = self.was_error.clone(); From e559a197cc909a6ec9f60900645dfa26a195ca03 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Thu, 24 Jun 2021 15:30:11 +0100 Subject: [PATCH 8/8] remove comment --- src/app_service.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/app_service.rs b/src/app_service.rs index feb9f22a4..0e590e2b7 100644 --- a/src/app_service.rs +++ b/src/app_service.rs @@ -302,10 +302,6 @@ impl Service for AppRouting { true }); - // you might expect to find `req.add_data_container()` called here but `HttpRequest` objects - // are created with the root data already set (in `AppInitService::call`) and root data is - // retained when releasing requests back to the pool - if let Some((srv, _info)) = res { srv.call(req) } else {