From b962959b8279aad017337d42957e7b1ab84c4c95 Mon Sep 17 00:00:00 2001 From: Roland Fredenhagen Date: Wed, 30 Mar 2022 17:41:31 +0200 Subject: [PATCH 1/8] WIP: basic implementation for `routes` macro --- actix-web-codegen/src/lib.rs | 5 + actix-web-codegen/src/route.rs | 217 ++++++++++++++++++++++++--------- 2 files changed, 161 insertions(+), 61 deletions(-) diff --git a/actix-web-codegen/src/lib.rs b/actix-web-codegen/src/lib.rs index 5ca5616b6c9..d7d0704f870 100644 --- a/actix-web-codegen/src/lib.rs +++ b/actix-web-codegen/src/lib.rs @@ -104,6 +104,11 @@ pub fn route(args: TokenStream, input: TokenStream) -> TokenStream { route::with_method(None, args, input) } +#[proc_macro_attribute] +pub fn routes(_: TokenStream, input: TokenStream) -> TokenStream { + route::with_methods(input) +} + macro_rules! method_macro { ($variant:ident, $method:ident) => { #[doc = concat!("Creates route handler with `actix_web::guard::", stringify!($variant), "`.")] diff --git a/actix-web-codegen/src/route.rs b/actix-web-codegen/src/route.rs index cb1ba1ef698..adb484c762f 100644 --- a/actix-web-codegen/src/route.rs +++ b/actix-web-codegen/src/route.rs @@ -4,7 +4,7 @@ use actix_router::ResourceDef; use proc_macro::TokenStream; use proc_macro2::{Span, TokenStream as TokenStream2}; use quote::{format_ident, quote, ToTokens, TokenStreamExt}; -use syn::{parse_macro_input, AttributeArgs, Ident, LitStr, NestedMeta}; +use syn::{parse_macro_input, Attribute, AttributeArgs, Ident, LitStr, Meta, NestedMeta, Path}; enum ResourceType { Async, @@ -20,7 +20,7 @@ impl ToTokens for ResourceType { macro_rules! method_type { ( - $($variant:ident, $upper:ident,)+ + $($variant:ident, $upper:ident, $lower:ident,)+ ) => { #[derive(Debug, PartialEq, Eq, Hash)] pub enum MethodType { @@ -42,20 +42,27 @@ macro_rules! method_type { _ => Err(format!("Unexpected HTTP method: `{}`", method)), } } + + fn from_path(method: &Path) -> Result { + match () { + $(_ if method.is_ident(stringify!($lower)) => Ok(Self::$variant),)+ + _ => Err(()), + } + } } }; } method_type! { - Get, GET, - Post, POST, - Put, PUT, - Delete, DELETE, - Head, HEAD, - Connect, CONNECT, - Options, OPTIONS, - Trace, TRACE, - Patch, PATCH, + Get, GET, get, + Post, POST, post, + Put, PUT, put, + Delete, DELETE, delete, + Head, HEAD, head, + Connect, CONNECT, connect, + Options, OPTIONS, options, + Trace, TRACE, trace, + Patch, PATCH, patch, } impl ToTokens for MethodType { @@ -90,6 +97,18 @@ impl Args { let mut wrappers = Vec::new(); let mut methods = HashSet::new(); + if args.is_empty() { + return Err(syn::Error::new( + Span::call_site(), + format!( + r#"invalid service definition, expected #[{}("")]"#, + method + .map_or("route", |it| it.as_str()) + .to_ascii_lowercase() + ), + )); + } + let is_route_macro = method.is_none(); if let Some(method) = method { methods.insert(method); @@ -184,7 +203,7 @@ impl Args { pub struct Route { name: syn::Ident, - args: Args, + args: Vec, ast: syn::ItemFn, resource_type: ResourceType, @@ -220,18 +239,6 @@ impl Route { ast: syn::ItemFn, method: Option, ) -> syn::Result { - if args.is_empty() { - return Err(syn::Error::new( - Span::call_site(), - format!( - r#"invalid service definition, expected #[{}("")]"#, - method - .map_or("route", |it| it.as_str()) - .to_ascii_lowercase() - ), - )); - } - let name = ast.sig.ident.clone(); // Try and pull out the doc comments so that we can reapply them to the generated struct. @@ -265,6 +272,41 @@ impl Route { } }; + Ok(Self { + name, + args: vec![args], + ast, + resource_type, + doc_attributes, + }) + } + + fn multiple(args: Vec, ast: syn::ItemFn) -> syn::Result { + let name = ast.sig.ident.clone(); + + // Try and pull out the doc comments so that we can reapply them to the generated struct. + // Note that multi line doc comments are converted to multiple doc attributes. + let doc_attributes = ast + .attrs + .iter() + .filter(|attr| attr.path.is_ident("doc")) + .cloned() + .collect(); + + let resource_type = if ast.sig.asyncness.is_some() { + ResourceType::Async + } else { + match ast.sig.output { + syn::ReturnType::Default => { + return Err(syn::Error::new_spanned( + ast, + "Function has no return type. Cannot be used as handler", + )); + } + syn::ReturnType::Type(_, ref typ) => guess_resource_type(typ.as_ref()), + } + }; + Ok(Self { name, args, @@ -280,38 +322,55 @@ impl ToTokens for Route { let Self { name, ast, - args: - Args { - path, - resource_name, - guards, - wrappers, - methods, - }, + args, resource_type, doc_attributes, } = self; - let resource_name = resource_name - .as_ref() - .map_or_else(|| name.to_string(), LitStr::value); - let method_guards = { - let mut others = methods.iter(); - // unwrapping since length is checked to be at least one - let first = others.next().unwrap(); - - if methods.len() > 1 { - quote! { - .guard( - ::actix_web::guard::Any(::actix_web::guard::#first()) - #(.or(::actix_web::guard::#others()))* - ) - } - } else { - quote! { - .guard(::actix_web::guard::#first()) - } - } - }; + + let registrations: TokenStream2 = args + .iter() + .map( + |Args { + path, + resource_name, + guards, + wrappers, + methods, + }| { + let resource_name = resource_name + .as_ref() + .map_or_else(|| name.to_string(), LitStr::value); + let method_guards = { + let mut others = methods.iter(); + // unwrapping since length is checked to be at least one + let first = others.next().unwrap(); + + if methods.len() > 1 { + quote! { + .guard( + ::actix_web::guard::Any(::actix_web::guard::#first()) + #(.or(::actix_web::guard::#others()))* + ) + } + } else { + quote! { + .guard(::actix_web::guard::#first()) + } + } + }; + quote! { + let __resource = ::actix_web::Resource::new(#path) + .name(#resource_name) + #method_guards + #(.guard(::actix_web::guard::fn_guard(#guards)))* + #(.wrap(#wrappers))* + .#resource_type(#name); + + ::actix_web::dev::HttpServiceFactory::register(__resource, __config); + } + }, + ) + .collect(); let stream = quote! { #(#doc_attributes)* @@ -321,14 +380,7 @@ impl ToTokens for Route { impl ::actix_web::dev::HttpServiceFactory for #name { fn register(self, __config: &mut actix_web::dev::AppService) { #ast - let __resource = ::actix_web::Resource::new(#path) - .name(#resource_name) - #method_guards - #(.guard(::actix_web::guard::fn_guard(#guards)))* - #(.wrap(#wrappers))* - .#resource_type(#name); - - ::actix_web::dev::HttpServiceFactory::register(__resource, __config) + #registrations } } }; @@ -357,6 +409,49 @@ pub(crate) fn with_method( } } +pub(crate) fn with_methods(input: TokenStream) -> TokenStream { + let mut ast = match syn::parse::(input.clone()) { + Ok(ast) => ast, + // on parse error, make IDEs happy; see fn docs + Err(err) => return input_and_compile_error(input, err), + }; + + let (methods, others): (Vec>, _) = ast + .attrs + .into_iter() + .map(|attr| match MethodType::from_path(&attr.path) { + Ok(method) => Ok((method, attr)), + Err(_) => Err(attr), + }) + .partition(Result::is_ok); + + ast.attrs = others.into_iter().map(Result::unwrap_err).collect(); + + let methods = match methods + .into_iter() + .map(Result::unwrap) + .map(|(method, attr)| { + attr.parse_meta().and_then(|args| { + if let Meta::List(args) = args { + Args::new(args.nested.into_iter().collect(), Some(method)) + } else { + Err(syn::Error::new_spanned(attr, "Invalid input for macro")) + } + }) + }) + .collect() + { + Ok(methods) => methods, + Err(err) => return input_and_compile_error(input, err), + }; + + match Route::multiple(methods, ast) { + Ok(route) => route.into_token_stream().into(), + // on macro related error, make IDEs happy; see fn docs + Err(err) => input_and_compile_error(input, err), + } +} + /// Converts the error to a token stream and appends it to the original input. /// /// Returning the original input in addition to the error is good for IDEs which can gracefully From f73c5406cb2bd29a23ba0e15510c2f21ae5afd1e Mon Sep 17 00:00:00 2001 From: Roland Fredenhagen Date: Sat, 30 Apr 2022 18:22:50 +0200 Subject: [PATCH 2/8] chore: changelog, docs, tests --- actix-web-codegen/CHANGES.md | 5 ++- actix-web-codegen/src/lib.rs | 43 ++++++++++++++++++- actix-web-codegen/tests/test_macro.rs | 25 ++++++++++- actix-web-codegen/tests/trybuild.rs | 2 + actix-web-codegen/tests/trybuild/routes-ok.rs | 23 ++++++++++ actix-web/src/lib.rs | 1 + 6 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 actix-web-codegen/tests/trybuild/routes-ok.rs diff --git a/actix-web-codegen/CHANGES.md b/actix-web-codegen/CHANGES.md index 8ee787c0a42..b999bfcabd0 100644 --- a/actix-web-codegen/CHANGES.md +++ b/actix-web-codegen/CHANGES.md @@ -1,6 +1,9 @@ # Changes -## Unreleased - 2021-xx-xx +## Unreleased - 2022-xx-xx +- Added `#[actix_web::routes]` macro to support multiple routes on one handler + +[#2718]: https://github.com/actix/actix-web/pull/2718 ## 4.0.0 - 2022-02-24 diff --git a/actix-web-codegen/src/lib.rs b/actix-web-codegen/src/lib.rs index d7d0704f870..693d9d34304 100644 --- a/actix-web-codegen/src/lib.rs +++ b/actix-web-codegen/src/lib.rs @@ -46,9 +46,20 @@ //! ``` //! //! # Multiple Path Handlers -//! There are no macros to generate multi-path handlers. Let us know in [this issue]. +//! Acts as a wrapper for multiple single method handler macros. It takes no arguments and +//! delegates those to the macros for the individual methods. See [macro@routes] macro docs. //! -//! [this issue]: https://github.com/actix/actix-web/issues/1709 +//! ``` +//! # use actix_web::HttpResponse; +//! # use actix_web_codegen::routes; +//! #[routes] +//! #[get("/test")] +//! #[get("/test2")] +//! #[delete("/test")] +//! async fn example() -> HttpResponse { +//! HttpResponse::Ok().finish() +//! } +//! ``` //! //! [actix-web attributes docs]: https://docs.rs/actix-web/latest/actix_web/#attributes //! [GET]: macro@get @@ -104,6 +115,34 @@ pub fn route(args: TokenStream, input: TokenStream) -> TokenStream { route::with_method(None, args, input) } +/// Creates resource handler, allowing multiple HTTP methods and paths. +/// +/// # Syntax +/// ```plain +/// #[routes] +/// #[("path", ...)] +/// #[("path", ...)] +/// ... +/// ``` +/// +/// # Attributes +/// The `routes` macro it self has no parameters, but allows specifying the attribute macros for +/// the different methods, e.g. [`GET`](macro@get) or [`POST`](macro@post). +/// +/// These helper attributes take the same parameters as the [single method handlers](crate#single-method-handler). +/// +/// # Examples +/// ``` +/// # use actix_web::HttpResponse; +/// # use actix_web_codegen::routes; +/// #[routes] +/// #[get("/test")] +/// #[get("/test2")] +/// #[delete("/test")] +/// async fn example() -> HttpResponse { +/// HttpResponse::Ok().finish() +/// } +/// ``` #[proc_macro_attribute] pub fn routes(_: TokenStream, input: TokenStream) -> TokenStream { route::with_methods(input) diff --git a/actix-web-codegen/tests/test_macro.rs b/actix-web-codegen/tests/test_macro.rs index 769cf2bc3ed..1146fdcf83c 100644 --- a/actix-web-codegen/tests/test_macro.rs +++ b/actix-web-codegen/tests/test_macro.rs @@ -10,7 +10,9 @@ use actix_web::{ }, web, App, Error, HttpResponse, Responder, }; -use actix_web_codegen::{connect, delete, get, head, options, patch, post, put, route, trace}; +use actix_web_codegen::{ + connect, delete, get, head, options, patch, post, put, route, routes, trace, +}; use futures_core::future::LocalBoxFuture; // Make sure that we can name function as 'config' @@ -89,6 +91,14 @@ async fn route_test() -> impl Responder { HttpResponse::Ok() } +#[routes] +#[get("/routes/test")] +#[get("/routes/test2")] +#[post("/routes/test")] +async fn routes_test() -> impl Responder { + HttpResponse::Ok() +} + #[get("/custom_resource_name", name = "custom")] async fn custom_resource_name_test<'a>(req: actix_web::HttpRequest) -> impl Responder { assert!(req.url_for_static("custom").is_ok()); @@ -186,6 +196,7 @@ async fn test_body() { .service(patch_test) .service(test_handler) .service(route_test) + .service(routes_test) .service(custom_resource_name_test) }); let request = srv.request(http::Method::GET, srv.url("/test")); @@ -242,6 +253,18 @@ async fn test_body() { let response = request.send().await.unwrap(); assert!(!response.status().is_success()); + let request = srv.request(http::Method::GET, srv.url("/routes/test")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + + let request = srv.request(http::Method::GET, srv.url("/routes/test2")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + + let request = srv.request(http::Method::POST, srv.url("/routes/test")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + let request = srv.request(http::Method::GET, srv.url("/custom_resource_name")); let response = request.send().await.unwrap(); assert!(response.status().is_success()); diff --git a/actix-web-codegen/tests/trybuild.rs b/actix-web-codegen/tests/trybuild.rs index b2d9ce186c9..17ece57da3f 100644 --- a/actix-web-codegen/tests/trybuild.rs +++ b/actix-web-codegen/tests/trybuild.rs @@ -12,6 +12,8 @@ fn compile_macros() { t.compile_fail("tests/trybuild/route-unexpected-method-fail.rs"); t.compile_fail("tests/trybuild/route-malformed-path-fail.rs"); + t.pass("tests/trybuild/routes-ok.rs"); + t.pass("tests/trybuild/docstring-ok.rs"); t.pass("tests/trybuild/test-runtime.rs"); diff --git a/actix-web-codegen/tests/trybuild/routes-ok.rs b/actix-web-codegen/tests/trybuild/routes-ok.rs new file mode 100644 index 00000000000..98b5e553e77 --- /dev/null +++ b/actix-web-codegen/tests/trybuild/routes-ok.rs @@ -0,0 +1,23 @@ +use actix_web_codegen::*; + +#[routes] +#[get("/")] +#[post("/")] +async fn index() -> String { + "Hello World!".to_owned() +} + +#[actix_web::main] +async fn main() { + use actix_web::App; + + let srv = actix_test::start(|| App::new().service(index)); + + let request = srv.get("/"); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + + let request = srv.post("/"); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); +} diff --git a/actix-web/src/lib.rs b/actix-web/src/lib.rs index 4eab24cece4..8d9e2dbcdbf 100644 --- a/actix-web/src/lib.rs +++ b/actix-web/src/lib.rs @@ -132,6 +132,7 @@ macro_rules! codegen_reexport { codegen_reexport!(main); codegen_reexport!(test); codegen_reexport!(route); +codegen_reexport!(routes); codegen_reexport!(head); codegen_reexport!(get); codegen_reexport!(post); From cd2599eee89e992ad0172b31d61cf7727b4e28cc Mon Sep 17 00:00:00 2001 From: Roland Fredenhagen Date: Tue, 3 May 2022 19:40:53 +0200 Subject: [PATCH 3/8] error on missing methods --- actix-web-codegen/src/route.rs | 40 +++++++++++-------- actix-web-codegen/tests/trybuild.rs | 1 + .../trybuild/routes-missing-method-fail.rs | 13 ++++++ .../routes-missing-method-fail.stderr | 13 ++++++ 4 files changed, 51 insertions(+), 16 deletions(-) create mode 100644 actix-web-codegen/tests/trybuild/routes-missing-method-fail.rs create mode 100644 actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr diff --git a/actix-web-codegen/src/route.rs b/actix-web-codegen/src/route.rs index adb484c762f..38217b8a0dc 100644 --- a/actix-web-codegen/src/route.rs +++ b/actix-web-codegen/src/route.rs @@ -427,23 +427,31 @@ pub(crate) fn with_methods(input: TokenStream) -> TokenStream { ast.attrs = others.into_iter().map(Result::unwrap_err).collect(); - let methods = match methods - .into_iter() - .map(Result::unwrap) - .map(|(method, attr)| { - attr.parse_meta().and_then(|args| { - if let Meta::List(args) = args { - Args::new(args.nested.into_iter().collect(), Some(method)) - } else { - Err(syn::Error::new_spanned(attr, "Invalid input for macro")) - } + let methods = + match methods + .into_iter() + .map(Result::unwrap) + .map(|(method, attr)| { + attr.parse_meta().and_then(|args| { + if let Meta::List(args) = args { + Args::new(args.nested.into_iter().collect(), Some(method)) + } else { + Err(syn::Error::new_spanned(attr, "Invalid input for macro")) + } + }) }) - }) - .collect() - { - Ok(methods) => methods, - Err(err) => return input_and_compile_error(input, err), - }; + .collect::, _>>() + { + Ok(methods) if methods.is_empty() => return input_and_compile_error( + input, + syn::Error::new( + Span::call_site(), + "The #[routes] macro requires at least one `#[(..)]` attribute.", + ), + ), + Ok(methods) => methods, + Err(err) => return input_and_compile_error(input, err), + }; match Route::multiple(methods, ast) { Ok(route) => route.into_token_stream().into(), diff --git a/actix-web-codegen/tests/trybuild.rs b/actix-web-codegen/tests/trybuild.rs index 17ece57da3f..f690d95703d 100644 --- a/actix-web-codegen/tests/trybuild.rs +++ b/actix-web-codegen/tests/trybuild.rs @@ -13,6 +13,7 @@ fn compile_macros() { t.compile_fail("tests/trybuild/route-malformed-path-fail.rs"); t.pass("tests/trybuild/routes-ok.rs"); + t.compile_fail("tests/trybuild/routes-missing-method-fail.rs"); t.pass("tests/trybuild/docstring-ok.rs"); diff --git a/actix-web-codegen/tests/trybuild/routes-missing-method-fail.rs b/actix-web-codegen/tests/trybuild/routes-missing-method-fail.rs new file mode 100644 index 00000000000..f0271ef44c0 --- /dev/null +++ b/actix-web-codegen/tests/trybuild/routes-missing-method-fail.rs @@ -0,0 +1,13 @@ +use actix_web_codegen::*; + +#[routes] +async fn index() -> String { + "Hello World!".to_owned() +} + +#[actix_web::main] +async fn main() { + use actix_web::App; + + let srv = actix_test::start(|| App::new().service(index)); +} diff --git a/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr b/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr new file mode 100644 index 00000000000..c7de4047fae --- /dev/null +++ b/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr @@ -0,0 +1,13 @@ +error: The #[routes] macro requires at least one `#[(..)]` attribute. + --> tests/trybuild/routes-missing-method-fail.rs:3:1 + | +3 | #[routes] + | ^^^^^^^^^ + | + = note: this error originates in the attribute macro `routes` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `fn() -> impl std::future::Future {index}: HttpServiceFactory` is not satisfied + --> tests/trybuild/routes-missing-method-fail.rs:12:55 + | +12 | let srv = actix_test::start(|| App::new().service(index)); + | ^^^^^ the trait `HttpServiceFactory` is not implemented for `fn() -> impl std::future::Future {index}` From 904cf042367d24cd68b1e1c609a391b777a1c58f Mon Sep 17 00:00:00 2001 From: Roland Fredenhagen Date: Tue, 3 May 2022 20:20:37 +0200 Subject: [PATCH 4/8] Apply suggestions from code review Co-authored-by: Igor Aleksanov --- actix-web-codegen/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actix-web-codegen/src/lib.rs b/actix-web-codegen/src/lib.rs index 693d9d34304..4b6dc43c532 100644 --- a/actix-web-codegen/src/lib.rs +++ b/actix-web-codegen/src/lib.rs @@ -126,8 +126,8 @@ pub fn route(args: TokenStream, input: TokenStream) -> TokenStream { /// ``` /// /// # Attributes -/// The `routes` macro it self has no parameters, but allows specifying the attribute macros for -/// the different methods, e.g. [`GET`](macro@get) or [`POST`](macro@post). +/// The `routes` macro itself has no parameters, but allows specifying the attribute macros for +/// the multiple paths and/or methods, e.g. [`GET`](macro@get) and [`POST`](macro@post). /// /// These helper attributes take the same parameters as the [single method handlers](crate#single-method-handler). /// From 2d5c1ac5fea4561153bc661519a4bf18e638c5bb Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 4 Jul 2022 04:08:39 +0100 Subject: [PATCH 5/8] update test stderr expectation --- actix-web-codegen/CHANGES.md | 2 +- .../tests/trybuild/routes-missing-method-fail.stderr | 4 +++- actix-web/CHANGES.md | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/actix-web-codegen/CHANGES.md b/actix-web-codegen/CHANGES.md index 61d05689002..6b525a441d0 100644 --- a/actix-web-codegen/CHANGES.md +++ b/actix-web-codegen/CHANGES.md @@ -1,7 +1,7 @@ # Changes ## Unreleased - 2022-xx-xx -- Added `#[routes]` macro to support multiple paths for one handler. [#2718] +- Add `#[routes]` macro to support multiple paths for one handler. [#2718] - Minimum supported Rust version (MSRV) is now 1.57 due to transitive `time` dependency. [#2718]: https://github.com/actix/actix-web/pull/2718 diff --git a/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr b/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr index c7de4047fae..b3795d74add 100644 --- a/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr +++ b/actix-web-codegen/tests/trybuild/routes-missing-method-fail.stderr @@ -10,4 +10,6 @@ error[E0277]: the trait bound `fn() -> impl std::future::Future {index}: HttpSer --> tests/trybuild/routes-missing-method-fail.rs:12:55 | 12 | let srv = actix_test::start(|| App::new().service(index)); - | ^^^^^ the trait `HttpServiceFactory` is not implemented for `fn() -> impl std::future::Future {index}` + | ------- ^^^^^ the trait `HttpServiceFactory` is not implemented for `fn() -> impl std::future::Future {index}` + | | + | required by a bound introduced by this call diff --git a/actix-web/CHANGES.md b/actix-web/CHANGES.md index 0144cb912fe..ff85e0c5690 100644 --- a/actix-web/CHANGES.md +++ b/actix-web/CHANGES.md @@ -2,6 +2,7 @@ ## Unreleased - 2022-xx-xx ### Added +- Add `#[routes]` macro to support multiple paths for one handler. [#2718] - Add `ServiceRequest::{parts, request}()` getter methods. [#2786] - Add configuration options for TLS handshake timeout via `HttpServer::{rustls, openssl}_with_config` methods. [#2752] From 8c41b0f5da886dab1f835f4c8e1959055e94f0a2 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 4 Jul 2022 04:58:32 +0100 Subject: [PATCH 6/8] add additional tests --- actix-web-codegen/Cargo.toml | 2 +- actix-web-codegen/src/route.rs | 99 +++++++++++-------- actix-web-codegen/tests/test_macro.rs | 51 +++++++++- actix-web-codegen/tests/trybuild.rs | 2 + .../trybuild/routes-invalid-method-fail.rs | 14 +++ .../routes-invalid-method-fail.stderr | 0 .../trybuild/routes-missing-args-fail.rs | 14 +++ .../trybuild/routes-missing-args-fail.stderr | 0 actix-web/CHANGES.md | 1 + 9 files changed, 137 insertions(+), 46 deletions(-) create mode 100644 actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs create mode 100644 actix-web-codegen/tests/trybuild/routes-invalid-method-fail.stderr create mode 100644 actix-web-codegen/tests/trybuild/routes-missing-args-fail.rs create mode 100644 actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr diff --git a/actix-web-codegen/Cargo.toml b/actix-web-codegen/Cargo.toml index 52094443bf2..a2aac7e68bd 100644 --- a/actix-web-codegen/Cargo.toml +++ b/actix-web-codegen/Cargo.toml @@ -18,7 +18,7 @@ proc-macro = true actix-router = "0.5.0" proc-macro2 = "1" quote = "1" -syn = { version = "1", features = ["full", "parsing"] } +syn = { version = "1", features = ["full", "extra-traits"] } [dev-dependencies] actix-macros = "0.2.3" diff --git a/actix-web-codegen/src/route.rs b/actix-web-codegen/src/route.rs index c767b7527d1..258d71a5ef2 100644 --- a/actix-web-codegen/src/route.rs +++ b/actix-web-codegen/src/route.rs @@ -4,7 +4,7 @@ use actix_router::ResourceDef; use proc_macro::TokenStream; use proc_macro2::{Span, TokenStream as TokenStream2}; use quote::{format_ident, quote, ToTokens, TokenStreamExt}; -use syn::{parse_macro_input, Attribute, AttributeArgs, Ident, LitStr, Meta, NestedMeta, Path}; +use syn::{parse_macro_input, AttributeArgs, Ident, LitStr, Meta, NestedMeta, Path}; enum ResourceType { Async, @@ -101,7 +101,7 @@ impl Args { return Err(syn::Error::new( Span::call_site(), format!( - r#"invalid service definition, expected #[{}("")]"#, + r#"invalid service definition, expected #[{}("")]"#, method .map_or("route", |it| it.as_str()) .to_ascii_lowercase() @@ -202,9 +202,18 @@ impl Args { } pub struct Route { + /// Name of the handler function being annotated. name: syn::Ident, + + /// Args passed to routing macro. + /// + /// When using `#[routes]`, this will contain args for each specific routing macro. args: Vec, + + /// AST of the handler function being annotated. ast: syn::ItemFn, + + /// TODO: remove resource_type: ResourceType, /// The doc comment attributes to copy to generated struct, if any. @@ -251,6 +260,7 @@ impl Route { .collect(); let args = Args::new(args, method)?; + if args.methods.is_empty() { return Err(syn::Error::new( Span::call_site(), @@ -329,47 +339,50 @@ impl ToTokens for Route { let registrations: TokenStream2 = args .iter() - .map( - |Args { - path, - resource_name, - guards, - wrappers, - methods, - }| { - let resource_name = resource_name - .as_ref() - .map_or_else(|| name.to_string(), LitStr::value); - let method_guards = { - let mut others = methods.iter(); - // unwrapping since length is checked to be at least one - let first = others.next().unwrap(); - - if methods.len() > 1 { - quote! { - .guard( - ::actix_web::guard::Any(::actix_web::guard::#first()) - #(.or(::actix_web::guard::#others()))* - ) - } - } else { - quote! { - .guard(::actix_web::guard::#first()) - } + .map(|args| { + let Args { + path, + resource_name, + guards, + wrappers, + methods, + } = args; + + let resource_name = resource_name + .as_ref() + .map_or_else(|| name.to_string(), LitStr::value); + + let method_guards = { + let mut others = methods.iter(); + + // unwrapping since length is checked to be at least one + let first = others.next().unwrap(); + + if methods.len() > 1 { + quote! { + .guard( + ::actix_web::guard::Any(::actix_web::guard::#first()) + #(.or(::actix_web::guard::#others()))* + ) + } + } else { + quote! { + .guard(::actix_web::guard::#first()) } - }; - quote! { - let __resource = ::actix_web::Resource::new(#path) - .name(#resource_name) - #method_guards - #(.guard(::actix_web::guard::fn_guard(#guards)))* - #(.wrap(#wrappers))* - .#resource_type(#name); - - ::actix_web::dev::HttpServiceFactory::register(__resource, __config); } - }, - ) + }; + + quote! { + let __resource = ::actix_web::Resource::new(#path) + .name(#resource_name) + #method_guards + #(.guard(::actix_web::guard::fn_guard(#guards)))* + #(.wrap(#wrappers))* + .#resource_type(#name); + + ::actix_web::dev::HttpServiceFactory::register(__resource, __config); + } + }) .collect(); let stream = quote! { @@ -416,14 +429,14 @@ pub(crate) fn with_methods(input: TokenStream) -> TokenStream { Err(err) => return input_and_compile_error(input, err), }; - let (methods, others): (Vec>, _) = ast + let (methods, others) = ast .attrs .into_iter() .map(|attr| match MethodType::from_path(&attr.path) { Ok(method) => Ok((method, attr)), Err(_) => Err(attr), }) - .partition(Result::is_ok); + .partition::, _>(Result::is_ok); ast.attrs = others.into_iter().map(Result::unwrap_err).collect(); diff --git a/actix-web-codegen/tests/test_macro.rs b/actix-web-codegen/tests/test_macro.rs index 3b282d31799..10e906967f1 100644 --- a/actix-web-codegen/tests/test_macro.rs +++ b/actix-web-codegen/tests/test_macro.rs @@ -8,7 +8,7 @@ use actix_web::{ header::{HeaderName, HeaderValue}, StatusCode, }, - web, App, Error, HttpResponse, Responder, + web, App, Error, HttpRequest, HttpResponse, Responder, }; use actix_web_codegen::{ connect, delete, get, head, options, patch, post, put, route, routes, trace, @@ -99,8 +99,33 @@ async fn routes_test() -> impl Responder { HttpResponse::Ok() } +// routes overlap with the more specific route first, therefore accessible +#[routes] +#[get("/routes/overlap/test")] +#[get("/routes/overlap/{foo}")] +async fn routes_overlapping_test(req: HttpRequest) -> impl Responder { + // foo is only populated when route is not /routes/overlap/test + match req.match_info().get("foo") { + None => assert!(req.uri() == "/routes/overlap/test"), + Some(_) => assert!(req.uri() != "/routes/overlap/test"), + } + + HttpResponse::Ok() +} + +// routes overlap with the more specific route last, therefore inaccessible +#[routes] +#[get("/routes/overlap2/{foo}")] +#[get("/routes/overlap2/test")] +async fn routes_overlapping_inaccessible_test(req: HttpRequest) -> impl Responder { + // foo is always populated even when path is /routes/overlap2/test + assert!(req.match_info().get("foo").is_some()); + + HttpResponse::Ok() +} + #[get("/custom_resource_name", name = "custom")] -async fn custom_resource_name_test<'a>(req: actix_web::HttpRequest) -> impl Responder { +async fn custom_resource_name_test<'a>(req: HttpRequest) -> impl Responder { assert!(req.url_for_static("custom").is_ok()); assert!(req.url_for_static("custom_resource_name_test").is_err()); HttpResponse::Ok() @@ -211,6 +236,8 @@ async fn test_body() { .service(patch_test) .service(test_handler) .service(route_test) + .service(routes_overlapping_test) + .service(routes_overlapping_inaccessible_test) .service(routes_test) .service(custom_resource_name_test) .service(guard_test) @@ -281,6 +308,26 @@ async fn test_body() { let response = request.send().await.unwrap(); assert!(response.status().is_success()); + let request = srv.request(http::Method::GET, srv.url("/routes/not-set")); + let response = request.send().await.unwrap(); + assert!(response.status().is_client_error()); + + let request = srv.request(http::Method::GET, srv.url("/routes/overlap/test")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + + let request = srv.request(http::Method::GET, srv.url("/routes/overlap/bar")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + + let request = srv.request(http::Method::GET, srv.url("/routes/overlap2/test")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + + let request = srv.request(http::Method::GET, srv.url("/routes/overlap2/bar")); + let response = request.send().await.unwrap(); + assert!(response.status().is_success()); + let request = srv.request(http::Method::GET, srv.url("/custom_resource_name")); let response = request.send().await.unwrap(); assert!(response.status().is_success()); diff --git a/actix-web-codegen/tests/trybuild.rs b/actix-web-codegen/tests/trybuild.rs index b4048ac5fe6..ede47f8dfc4 100644 --- a/actix-web-codegen/tests/trybuild.rs +++ b/actix-web-codegen/tests/trybuild.rs @@ -14,6 +14,8 @@ fn compile_macros() { t.pass("tests/trybuild/routes-ok.rs"); t.compile_fail("tests/trybuild/routes-missing-method-fail.rs"); + t.compile_fail("tests/trybuild/routes-invalid-method-fail.rs"); + t.compile_fail("tests/trybuild/routes-missing-args-fail.rs"); t.pass("tests/trybuild/docstring-ok.rs"); diff --git a/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs b/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs new file mode 100644 index 00000000000..65573cf7905 --- /dev/null +++ b/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs @@ -0,0 +1,14 @@ +use actix_web_codegen::*; + +#[routes] +#[get] +async fn index() -> String { + "Hello World!".to_owned() +} + +#[actix_web::main] +async fn main() { + use actix_web::App; + + let srv = actix_test::start(|| App::new().service(index)); +} diff --git a/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.stderr b/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.stderr new file mode 100644 index 00000000000..e69de29bb2d diff --git a/actix-web-codegen/tests/trybuild/routes-missing-args-fail.rs b/actix-web-codegen/tests/trybuild/routes-missing-args-fail.rs new file mode 100644 index 00000000000..65573cf7905 --- /dev/null +++ b/actix-web-codegen/tests/trybuild/routes-missing-args-fail.rs @@ -0,0 +1,14 @@ +use actix_web_codegen::*; + +#[routes] +#[get] +async fn index() -> String { + "Hello World!".to_owned() +} + +#[actix_web::main] +async fn main() { + use actix_web::App; + + let srv = actix_test::start(|| App::new().service(index)); +} diff --git a/actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr b/actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr new file mode 100644 index 00000000000..e69de29bb2d diff --git a/actix-web/CHANGES.md b/actix-web/CHANGES.md index ff85e0c5690..f38282b4183 100644 --- a/actix-web/CHANGES.md +++ b/actix-web/CHANGES.md @@ -9,6 +9,7 @@ ### Changed - Minimum supported Rust version (MSRV) is now 1.57 due to transitive `time` dependency. +[#2718]: https://github.com/actix/actix-web/pull/2718 [#2752]: https://github.com/actix/actix-web/pull/2752 [#2786]: https://github.com/actix/actix-web/pull/2786 From df25c254a51c5b89a63cc3204316bd7b901ff4ad Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 4 Jul 2022 05:02:36 +0100 Subject: [PATCH 7/8] fix stderr output --- actix-web-codegen/tests/trybuild.rs | 1 - .../trybuild/routes-invalid-method-fail.rs | 14 ------------- .../routes-invalid-method-fail.stderr | 0 .../trybuild/routes-missing-args-fail.stderr | 21 +++++++++++++++++++ 4 files changed, 21 insertions(+), 15 deletions(-) delete mode 100644 actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs delete mode 100644 actix-web-codegen/tests/trybuild/routes-invalid-method-fail.stderr diff --git a/actix-web-codegen/tests/trybuild.rs b/actix-web-codegen/tests/trybuild.rs index ede47f8dfc4..1f7996fd0e8 100644 --- a/actix-web-codegen/tests/trybuild.rs +++ b/actix-web-codegen/tests/trybuild.rs @@ -14,7 +14,6 @@ fn compile_macros() { t.pass("tests/trybuild/routes-ok.rs"); t.compile_fail("tests/trybuild/routes-missing-method-fail.rs"); - t.compile_fail("tests/trybuild/routes-invalid-method-fail.rs"); t.compile_fail("tests/trybuild/routes-missing-args-fail.rs"); t.pass("tests/trybuild/docstring-ok.rs"); diff --git a/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs b/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs deleted file mode 100644 index 65573cf7905..00000000000 --- a/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.rs +++ /dev/null @@ -1,14 +0,0 @@ -use actix_web_codegen::*; - -#[routes] -#[get] -async fn index() -> String { - "Hello World!".to_owned() -} - -#[actix_web::main] -async fn main() { - use actix_web::App; - - let srv = actix_test::start(|| App::new().service(index)); -} diff --git a/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.stderr b/actix-web-codegen/tests/trybuild/routes-invalid-method-fail.stderr deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr b/actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr index e69de29bb2d..8efe0682ba5 100644 --- a/actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr +++ b/actix-web-codegen/tests/trybuild/routes-missing-args-fail.stderr @@ -0,0 +1,21 @@ +error: invalid service definition, expected #[get("")] + --> tests/trybuild/routes-missing-args-fail.rs:4:1 + | +4 | #[get] + | ^^^^^^ + | + = note: this error originates in the attribute macro `get` (in Nightly builds, run with -Z macro-backtrace for more info) + +error: Invalid input for macro + --> tests/trybuild/routes-missing-args-fail.rs:4:1 + | +4 | #[get] + | ^^^^^^ + +error[E0277]: the trait bound `fn() -> impl std::future::Future {index}: HttpServiceFactory` is not satisfied + --> tests/trybuild/routes-missing-args-fail.rs:13:55 + | +13 | let srv = actix_test::start(|| App::new().service(index)); + | ------- ^^^^^ the trait `HttpServiceFactory` is not implemented for `fn() -> impl std::future::Future {index}` + | | + | required by a bound introduced by this call From 4e8eb74600acb0070054d70b083d607e520d52e2 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 4 Jul 2022 05:05:34 +0100 Subject: [PATCH 8/8] remove useless ResourceType this is dead code from back when .to and .to_async were different ways to add a service --- actix-web-codegen/src/route.rs | 82 ++++++---------------------------- 1 file changed, 14 insertions(+), 68 deletions(-) diff --git a/actix-web-codegen/src/route.rs b/actix-web-codegen/src/route.rs index 258d71a5ef2..7a0658468e6 100644 --- a/actix-web-codegen/src/route.rs +++ b/actix-web-codegen/src/route.rs @@ -3,21 +3,9 @@ use std::{collections::HashSet, convert::TryFrom}; use actix_router::ResourceDef; use proc_macro::TokenStream; use proc_macro2::{Span, TokenStream as TokenStream2}; -use quote::{format_ident, quote, ToTokens, TokenStreamExt}; +use quote::{quote, ToTokens, TokenStreamExt}; use syn::{parse_macro_input, AttributeArgs, Ident, LitStr, Meta, NestedMeta, Path}; -enum ResourceType { - Async, - Sync, -} - -impl ToTokens for ResourceType { - fn to_tokens(&self, stream: &mut TokenStream2) { - let ident = format_ident!("to"); - stream.append(ident); - } -} - macro_rules! method_type { ( $($variant:ident, $upper:ident, $lower:ident,)+ @@ -213,35 +201,10 @@ pub struct Route { /// AST of the handler function being annotated. ast: syn::ItemFn, - /// TODO: remove - resource_type: ResourceType, - /// The doc comment attributes to copy to generated struct, if any. doc_attributes: Vec, } -fn guess_resource_type(typ: &syn::Type) -> ResourceType { - let mut guess = ResourceType::Sync; - - if let syn::Type::ImplTrait(typ) = typ { - for bound in typ.bounds.iter() { - if let syn::TypeParamBound::Trait(bound) = bound { - for bound in bound.path.segments.iter() { - if bound.ident == "Future" { - guess = ResourceType::Async; - break; - } else if bound.ident == "Responder" { - guess = ResourceType::Sync; - break; - } - } - } - } - } - - guess -} - impl Route { pub fn new( args: AttributeArgs, @@ -268,25 +231,17 @@ impl Route { )); } - let resource_type = if ast.sig.asyncness.is_some() { - ResourceType::Async - } else { - match ast.sig.output { - syn::ReturnType::Default => { - return Err(syn::Error::new_spanned( - ast, - "Function has no return type. Cannot be used as handler", - )); - } - syn::ReturnType::Type(_, ref typ) => guess_resource_type(typ.as_ref()), - } - }; + if matches!(ast.sig.output, syn::ReturnType::Default) { + return Err(syn::Error::new_spanned( + ast, + "Function has no return type. Cannot be used as handler", + )); + } Ok(Self { name, args: vec![args], ast, - resource_type, doc_attributes, }) } @@ -303,25 +258,17 @@ impl Route { .cloned() .collect(); - let resource_type = if ast.sig.asyncness.is_some() { - ResourceType::Async - } else { - match ast.sig.output { - syn::ReturnType::Default => { - return Err(syn::Error::new_spanned( - ast, - "Function has no return type. Cannot be used as handler", - )); - } - syn::ReturnType::Type(_, ref typ) => guess_resource_type(typ.as_ref()), - } - }; + if matches!(ast.sig.output, syn::ReturnType::Default) { + return Err(syn::Error::new_spanned( + ast, + "Function has no return type. Cannot be used as handler", + )); + } Ok(Self { name, args, ast, - resource_type, doc_attributes, }) } @@ -333,7 +280,6 @@ impl ToTokens for Route { name, ast, args, - resource_type, doc_attributes, } = self; @@ -378,7 +324,7 @@ impl ToTokens for Route { #method_guards #(.guard(::actix_web::guard::fn_guard(#guards)))* #(.wrap(#wrappers))* - .#resource_type(#name); + .to(#name); ::actix_web::dev::HttpServiceFactory::register(__resource, __config); }