From 1e08177f551d2a9a41d0c9e483ed7053308dd6be Mon Sep 17 00:00:00 2001 From: Sergio Benitez Date: Sun, 8 Apr 2018 16:14:15 -0700 Subject: [PATCH] Tidy up latest routing changes. --- lib/src/fairing/mod.rs | 25 +++++-------- lib/src/request/request.rs | 12 ++++--- lib/src/rocket.rs | 18 +++++----- .../fairing_before_head_strip-issue-546.rs | 35 +++++++++++-------- 4 files changed, 45 insertions(+), 45 deletions(-) diff --git a/lib/src/fairing/mod.rs b/lib/src/fairing/mod.rs index 6989226c..33c3132d 100644 --- a/lib/src/fairing/mod.rs +++ b/lib/src/fairing/mod.rs @@ -48,14 +48,6 @@ //! Furthermore, a `Fairing` should take care to act locally so that the actions //! of other `Fairings` are not jeopardized. For instance, unless it is made //! abundantly clear, a fairing should not rewrite every request. -//! -//! ## Attention -//! -//! If Rocket receives a `HEAD` request and no appropriate Route is provided, -//! Rocket tries to fullfil this request as it were a `GET` request (Autohandling -//! `HEAD` requests). _Beware_ the request method is set to `GET` on the request which is -//! provided in the [`on_response`](/rocket/fairing/trait.Fairing.html#method.on_response) -//! method. use {Rocket, Request, Response, Data}; @@ -140,8 +132,9 @@ pub use self::info_kind::{Info, Kind}; /// /// A request callback, represented by the /// [`on_request`](/rocket/fairing/trait.Fairing.html#method.on_request) -/// method, is called just after a request is received. At this point, -/// Rocket has parsed the incoming HTTP into +/// method, is called just after a request is received, immediately after +/// pre-processing the request with method changes due to `_method` form +/// fields. At this point, Rocket has parsed the incoming HTTP request into /// [`Request`](/rocket/struct.Request.html) and /// [`Data`](/rocket/struct.Data.html) structures but has not routed the /// request. A request callback can modify the request at will and @@ -159,7 +152,12 @@ pub use self::info_kind::{Info, Kind}; /// error catchers, and has generated the would-be final response. A /// response callback can modify the response at will. For exammple, a /// response callback can provide a default response when the user fails to -/// handle the request by checking for 404 responses. +/// handle the request by checking for 404 responses. Note that a given +/// `Request` may have changed between `on_request` and `on_response` +/// invocations. Apart from any change made by other fairings, Rocket sets +/// the method for `HEAD` requests to `GET` if there is no matching `HEAD` +/// handler for that request. Additionally, Rocket will automatically strip +/// the body for `HEAD` requests _after_ response fairings have run. /// /// # Implementing /// @@ -345,11 +343,6 @@ pub trait Fairing: Send + Sync + 'static { /// this fairing. The `&Request` parameter is the request that was routed, /// and the `&mut Response` parameter is the resulting response. /// - /// ## Note - /// - /// The body of a `HEAD` request will be stripped off _after_ all response - /// `Fairings` have been performed. - /// /// ## Default Implementation /// /// The default implementation of this method does nothing. diff --git a/lib/src/request/request.rs b/lib/src/request/request.rs index 01cbaab6..7c0a1986 100644 --- a/lib/src/request/request.rs +++ b/lib/src/request/request.rs @@ -49,9 +49,11 @@ impl<'r> Request<'r> { /// parameter can be of any type that implements `Into` including /// `&str` and `String`; it must be a valid absolute URI. #[inline(always)] - pub(crate) fn new<'s: 'r, U: Into>>(rocket: &'r Rocket, - method: Method, - uri: U) -> Request<'r> { + pub(crate) fn new<'s: 'r, U: Into>>( + rocket: &'r Rocket, + method: Method, + uri: U + ) -> Request<'r> { Request { method: Cell::new(method), uri: uri.into(), @@ -111,7 +113,7 @@ impl<'r> Request<'r> { /// ``` #[inline(always)] pub fn set_method(&mut self, method: Method) { - self.method.set(method); + self._set_method(method); } /// Borrow the URI from `self`, which is guaranteed to be an absolute URI. @@ -616,7 +618,7 @@ impl<'r> Request<'r> { *self.state.params.borrow_mut() = route.get_param_indexes(self.uri()); } - /// Set the method of `self`, even if self is a shared reference + /// Set the method of `self`, even when `self` is a shared reference. #[inline(always)] pub(crate) fn _set_method(&self, method: Method) { self.method.set(method); diff --git a/lib/src/rocket.rs b/lib/src/rocket.rs index 632a5a42..eeb133df 100644 --- a/lib/src/rocket.rs +++ b/lib/src/rocket.rs @@ -193,7 +193,7 @@ impl Rocket { } } - #[inline(always)] + #[inline] pub(crate) fn dispatch<'s, 'r>( &'s self, request: &'r mut Request<'s>, @@ -201,13 +201,13 @@ impl Rocket { ) -> Response<'r> { info!("{}:", request); - // Do a bit of preprocessing before routing; run the attached fairings. + // Do a bit of preprocessing before routing. self.preprocess_request(request, &data); // Run the request fairings. self.fairings.handle_request(request, &data); - // Check if the request is a `HEAD` request. + // Remember if the request is a `HEAD` request for later body stripping. let was_head_request = request.method() == Method::Head; // Route the request and run the user's handlers. @@ -226,13 +226,13 @@ impl Rocket { response } + /// Route the request and process the outcome to eventually get a response. fn route_and_process<'s, 'r>( &'s self, request: &'r Request<'s>, data: Data ) -> Response<'r> { - // Route the request to get a response. - let response = match self.route(request, data) { + match self.route(request, data) { Outcome::Success(mut response) => { // A user's route responded! Set the cookies. for cookie in request.cookies().delta() { @@ -242,7 +242,6 @@ impl Rocket { response } Outcome::Forward(data) => { - // There was no matching route. Autohandle `HEAD` requests. if request.method() == Method::Head { info_!("Autohandling {} request.", Paint::white("HEAD")); @@ -251,13 +250,12 @@ impl Rocket { request._set_method(Method::Get); self.route_and_process(request, data) } else { + // No match was found and it can't be autohandled. 404. self.handle_error(Status::NotFound, request) } } - Outcome::Failure(status) => self.handle_error(status, request), - }; - - response + Outcome::Failure(status) => self.handle_error(status, request) + } } /// Tries to find a `Responder` for a given `request`. It does this by diff --git a/lib/tests/fairing_before_head_strip-issue-546.rs b/lib/tests/fairing_before_head_strip-issue-546.rs index 7363c5d0..7575ae34 100644 --- a/lib/tests/fairing_before_head_strip-issue-546.rs +++ b/lib/tests/fairing_before_head_strip-issue-546.rs @@ -3,10 +3,10 @@ extern crate rocket; -const RESPONSE_STRING: &'static str = "{'test': 'dont strip before fairing' }"; +const RESPONSE_STRING: &'static str = "This is the body. Hello, world!"; #[head("/")] -fn index() -> &'static str { +fn head() -> &'static str { RESPONSE_STRING } @@ -15,6 +15,8 @@ fn auto() -> &'static str { RESPONSE_STRING } +// Test that response fairings see the response body for all `HEAD` requests, +// whether they are auto-handled or not. mod fairing_before_head_strip { use super::*; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -25,35 +27,39 @@ mod fairing_before_head_strip { use rocket::http::Status; use rocket::State; - #[derive(Default)] - struct Counter { - get: AtomicUsize, - } - #[test] - fn not_empty_before_fairing() { + fn not_auto_handled() { let rocket = rocket::ignite() - .mount("/", routes![index]) + .mount("/", routes![head]) + .attach(AdHoc::on_request(|req, _| { + assert_eq!(req.method(), Method::Head); + })) .attach(AdHoc::on_response(|req, res| { assert_eq!(req.method(), Method::Head); assert_eq!(res.body_string(), Some(RESPONSE_STRING.into())); })); let client = Client::new(rocket).unwrap(); - let response = client.head("/").dispatch(); + let mut response = client.head("/").dispatch(); assert_eq!(response.status(), Status::Ok); + assert!(response.body().is_none()); } #[test] - fn not_empty_before_fairing_autohandeled() { + fn auto_handled() { + #[derive(Default)] + struct Counter(AtomicUsize); + let counter = Counter::default(); let rocket = rocket::ignite() .mount("/", routes![auto]) .manage(counter) .attach(AdHoc::on_request(|req, _| { - let c = req.guard::>().unwrap(); + assert_eq!(req.method(), Method::Head); - assert_eq!(c.get.fetch_add(1, Ordering::Release), 0); + // This should be called exactly once. + let c = req.guard::>().unwrap(); + assert_eq!(c.0.fetch_add(1, Ordering::SeqCst), 0); })) .attach(AdHoc::on_response(|req, res| { assert_eq!(req.method(), Method::Get); @@ -61,7 +67,8 @@ mod fairing_before_head_strip { })); let client = Client::new(rocket).unwrap(); - let response = client.head("/").dispatch(); + let mut response = client.head("/").dispatch(); assert_eq!(response.status(), Status::Ok); + assert!(response.body().is_none()); } }