From 8c41b0f5da886dab1f835f4c8e1959055e94f0a2 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Mon, 4 Jul 2022 04:58:32 +0100 Subject: [PATCH] add additional tests --- actix-web-codegen/Cargo.toml | 2 +- actix-web-codegen/src/route.rs | 97 +++++++++++-------- 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, 136 insertions(+), 45 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 52094443b..a2aac7e68 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 c767b7527..258d71a5e 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(); + .map(|args| { + let Args { + path, + resource_name, + guards, + wrappers, + methods, + } = args; - 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 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 3b282d317..10e906967 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 b4048ac5f..ede47f8df 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 000000000..65573cf79 --- /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 000000000..e69de29bb 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 000000000..65573cf79 --- /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 000000000..e69de29bb diff --git a/actix-web/CHANGES.md b/actix-web/CHANGES.md index ff85e0c56..f38282b41 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