From ae7f71e317d40a4ebe58621a2695f0af45dfe947 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Fri, 21 Jan 2022 17:18:07 +0000 Subject: [PATCH] remove ambiguous `HttpResponseBuilder::del_cookie` (#2591) --- CHANGES.md | 2 + Cargo.toml | 4 ++ src/response/builder.rs | 120 ++++++++++++++++------------------------ tests/test_server.rs | 12 ++-- 4 files changed, 59 insertions(+), 79 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index fae671072..44bbc30f9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -6,9 +6,11 @@ ### Removed - `HttpRequest::req_data[_mut]()`; request-local data is still available through `.extensions()`. [#2585] +- `HttpRequestBuilder::del_cookie`. [#2591] [#2585]: https://github.com/actix/actix-web/pull/2585 [#2586]: https://github.com/actix/actix-web/pull/2586 +[#2591]: https://github.com/actix/actix-web/pull/2591 ## 4.0.0-beta.20 - 2022-01-14 diff --git a/Cargo.toml b/Cargo.toml index 6c64a9e87..ce7eaeb61 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -157,6 +157,10 @@ awc = { path = "awc" } name = "test_server" required-features = ["compress-brotli", "compress-gzip", "compress-zstd", "cookies"] +[[test]] +name = "compression" +required-features = ["compress-brotli", "compress-gzip", "compress-zstd"] + [[example]] name = "basic" required-features = ["compress-gzip"] diff --git a/src/response/builder.rs b/src/response/builder.rs index c8e44729a..f50aad9f4 100644 --- a/src/response/builder.rs +++ b/src/response/builder.rs @@ -6,18 +6,17 @@ use std::{ task::{Context, Poll}, }; -use actix_http::{ - error::HttpError, - header::{self, HeaderName, TryIntoHeaderPair, TryIntoHeaderValue}, - ConnectionType, Extensions, Response, ResponseHead, StatusCode, -}; +use actix_http::{error::HttpError, Response, ResponseHead}; use bytes::Bytes; use futures_core::Stream; use serde::Serialize; use crate::{ body::{BodyStream, BoxBody, MessageBody}, + dev::Extensions, error::{Error, JsonPayloadError}, + http::header::{self, HeaderName, TryIntoHeaderPair, TryIntoHeaderValue}, + http::{ConnectionType, StatusCode}, BoxError, HttpRequest, HttpResponse, Responder, }; @@ -26,9 +25,7 @@ use crate::{ /// This type can be used to construct an instance of `Response` through a builder-like pattern. pub struct HttpResponseBuilder { res: Option>, - err: Option, - #[cfg(feature = "cookies")] - cookies: Option, + error: Option, } impl HttpResponseBuilder { @@ -37,9 +34,7 @@ impl HttpResponseBuilder { pub fn new(status: StatusCode) -> Self { Self { res: Some(Response::with_body(status, BoxBody::new(()))), - err: None, - #[cfg(feature = "cookies")] - cookies: None, + error: None, } } @@ -68,7 +63,7 @@ impl HttpResponseBuilder { Ok((key, value)) => { parts.headers.insert(key, value); } - Err(e) => self.err = Some(e.into()), + Err(e) => self.error = Some(e.into()), }; } @@ -90,7 +85,7 @@ impl HttpResponseBuilder { if let Some(parts) = self.inner() { match header.try_into_pair() { Ok((key, value)) => parts.headers.append(key, value), - Err(e) => self.err = Some(e.into()), + Err(e) => self.error = Some(e.into()), }; } @@ -109,14 +104,14 @@ impl HttpResponseBuilder { K::Error: Into, V: TryIntoHeaderValue, { - if self.err.is_some() { + if self.error.is_some() { return self; } match (key.try_into(), value.try_into_value()) { (Ok(name), Ok(value)) => return self.insert_header((name, value)), - (Err(err), _) => self.err = Some(err.into()), - (_, Err(err)) => self.err = Some(err.into()), + (Err(err), _) => self.error = Some(err.into()), + (_, Err(err)) => self.error = Some(err.into()), } self @@ -134,14 +129,14 @@ impl HttpResponseBuilder { K::Error: Into, V: TryIntoHeaderValue, { - if self.err.is_some() { + if self.error.is_some() { return self; } match (key.try_into(), value.try_into_value()) { (Ok(name), Ok(value)) => return self.append_header((name, value)), - (Err(err), _) => self.err = Some(err.into()), - (_, Err(err)) => self.err = Some(err.into()), + (Err(err), _) => self.error = Some(err.into()), + (_, Err(err)) => self.error = Some(err.into()), } self @@ -214,18 +209,23 @@ impl HttpResponseBuilder { Ok(value) => { parts.headers.insert(header::CONTENT_TYPE, value); } - Err(e) => self.err = Some(e.into()), + Err(e) => self.error = Some(e.into()), }; } self } - /// Set a cookie. + /// Add a cookie to the response. /// + /// To send a "removal" cookie, call [`.make_removal()`](cookie::Cookie::make_removal) on the + /// given cookie. See [`HttpResponse::add_removal_cookie()`] to learn more. + /// + /// # Examples + /// Send a new cookie: /// ``` /// use actix_web::{HttpResponse, cookie::Cookie}; /// - /// HttpResponse::Ok() + /// let res = HttpResponse::Ok() /// .cookie( /// Cookie::build("name", "value") /// .domain("www.rust-lang.org") @@ -236,45 +236,31 @@ impl HttpResponseBuilder { /// ) /// .finish(); /// ``` - #[cfg(feature = "cookies")] - pub fn cookie<'c>(&mut self, cookie: cookie::Cookie<'c>) -> &mut Self { - if self.cookies.is_none() { - let mut jar = cookie::CookieJar::new(); - jar.add(cookie.into_owned()); - self.cookies = Some(jar) - } else { - self.cookies.as_mut().unwrap().add(cookie.into_owned()); - } - self - } - - /// Remove cookie. - /// - /// A `Set-Cookie` header is added that will delete a cookie with the same name from the client. /// + /// Send a removal cookie: /// ``` - /// use actix_web::{HttpRequest, HttpResponse, Responder}; + /// use actix_web::{HttpResponse, cookie::Cookie}; /// - /// async fn handler(req: HttpRequest) -> impl Responder { - /// let mut builder = HttpResponse::Ok(); + /// // the name, domain and path match the cookie created in the previous example + /// let mut cookie = Cookie::build("name", "value-does-not-matter") + /// .domain("www.rust-lang.org") + /// .path("/") + /// .finish(); + /// cookie.make_removal(); /// - /// if let Some(ref cookie) = req.cookie("name") { - /// builder.del_cookie(cookie); - /// } - /// - /// builder.finish() - /// } + /// let res = HttpResponse::Ok() + /// .cookie(cookie) + /// .finish(); /// ``` #[cfg(feature = "cookies")] - pub fn del_cookie(&mut self, cookie: &cookie::Cookie<'_>) -> &mut Self { - if self.cookies.is_none() { - self.cookies = Some(cookie::CookieJar::new()) + pub fn cookie(&mut self, cookie: cookie::Cookie<'_>) -> &mut Self { + match cookie.to_string().try_into_value() { + Ok(hdr_val) => self.append_header((header::SET_COOKIE, hdr_val)), + Err(err) => { + self.error = Some(err.into()); + self + } } - let jar = self.cookies.as_mut().unwrap(); - let cookie = cookie.clone().into_owned(); - jar.add_original(cookie.clone()); - jar.remove(cookie); - self } /// Returns a reference to the response-local data/extensions container. @@ -297,6 +283,9 @@ impl HttpResponseBuilder { /// Set a body and build the `HttpResponse`. /// + /// Unlike [`message_body`](Self::message_body), errors are converted into error + /// responses immediately. + /// /// `HttpResponseBuilder` can not be used after this call. pub fn body(&mut self, body: B) -> HttpResponse where @@ -312,7 +301,7 @@ impl HttpResponseBuilder { /// /// `HttpResponseBuilder` can not be used after this call. pub fn message_body(&mut self, body: B) -> Result, Error> { - if let Some(err) = self.err.take() { + if let Some(err) = self.error.take() { return Err(err.into()); } @@ -322,20 +311,7 @@ impl HttpResponseBuilder { .expect("cannot reuse response builder") .set_body(body); - #[allow(unused_mut)] // mut is only unused when cookies are disabled - let mut res = HttpResponse::from(res); - - #[cfg(feature = "cookies")] - if let Some(ref jar) = self.cookies { - for cookie in jar.delta() { - match actix_http::header::HeaderValue::from_str(&cookie.to_string()) { - Ok(val) => res.headers_mut().append(header::SET_COOKIE, val), - Err(err) => return Err(err.into()), - }; - } - } - - Ok(res) + Ok(HttpResponse::from(res)) } /// Set a streaming body and build the `HttpResponse`. @@ -384,14 +360,12 @@ impl HttpResponseBuilder { pub fn take(&mut self) -> Self { Self { res: self.res.take(), - err: self.err.take(), - #[cfg(feature = "cookies")] - cookies: self.cookies.take(), + error: self.error.take(), } } fn inner(&mut self) -> Option<&mut ResponseHead> { - if self.err.is_some() { + if self.error.is_some() { return None; } diff --git a/tests/test_server.rs b/tests/test_server.rs index 987e51a65..ade48a485 100644 --- a/tests/test_server.rs +++ b/tests/test_server.rs @@ -11,7 +11,7 @@ use std::{ }; use actix_web::{ - cookie::{Cookie, CookieBuilder}, + cookie::Cookie, http::{header, StatusCode}, middleware::{Compress, NormalizePath, TrailingSlash}, web, App, Error, HttpResponse, @@ -773,7 +773,7 @@ async fn test_server_cookies() { App::new().default_service(web::to(|| { HttpResponse::Ok() .cookie( - CookieBuilder::new("first", "first_value") + Cookie::build("first", "first_value") .http_only(true) .finish(), ) @@ -787,13 +787,13 @@ async fn test_server_cookies() { let res = req.send().await.unwrap(); assert!(res.status().is_success()); - let first_cookie = CookieBuilder::new("first", "first_value") + let first_cookie = Cookie::build("first", "first_value") .http_only(true) .finish(); - let second_cookie = Cookie::new("second", "second_value"); + let second_cookie = Cookie::new("second", "first_value"); let cookies = res.cookies().expect("To have cookies"); - assert_eq!(cookies.len(), 2); + assert_eq!(cookies.len(), 3); if cookies[0] == first_cookie { assert_eq!(cookies[1], second_cookie); } else { @@ -809,7 +809,7 @@ async fn test_server_cookies() { .get_all(http::header::SET_COOKIE) .map(|header| header.to_str().expect("To str").to_string()) .collect::>(); - assert_eq!(cookies.len(), 2); + assert_eq!(cookies.len(), 3); if cookies[0] == first_cookie { assert_eq!(cookies[1], second_cookie); } else {