From 6bd94ac3c52ebdf6b65958dcc37154b85be618b5 Mon Sep 17 00:00:00 2001 From: Rob Ede Date: Sat, 26 Nov 2022 00:33:17 +0000 Subject: [PATCH] improve macro compile errors --- actix-multipart-derive/src/lib.rs | 144 ++++++++++++++++++------------ actix-multipart/CHANGES.md | 5 +- actix-multipart/Cargo.toml | 8 +- actix-multipart/src/form/text.rs | 7 +- actix-multipart/src/server.rs | 16 ++-- 5 files changed, 105 insertions(+), 75 deletions(-) diff --git a/actix-multipart-derive/src/lib.rs b/actix-multipart-derive/src/lib.rs index b99a0bcdb..23e0c4837 100644 --- a/actix-multipart-derive/src/lib.rs +++ b/actix-multipart-derive/src/lib.rs @@ -10,17 +10,11 @@ use std::{collections::HashSet, convert::TryFrom as _}; use darling::{FromDeriveInput, FromField, FromMeta}; use parse_size::parse_size; -use proc_macro2::Ident; +use proc_macro::TokenStream; +use proc_macro2::{Ident, Span}; use quote::quote; use syn::{parse_macro_input, Type}; -#[derive(FromDeriveInput, Default)] -#[darling(attributes(multipart), default)] -struct MultipartFormAttrs { - deny_unknown_fields: bool, - duplicate_field: DuplicateField, -} - #[derive(FromMeta)] enum DuplicateField { Ignore, @@ -34,6 +28,13 @@ impl Default for DuplicateField { } } +#[derive(FromDeriveInput, Default)] +#[darling(attributes(multipart), default)] +struct MultipartFormAttrs { + deny_unknown_fields: bool, + duplicate_field: DuplicateField, +} + #[derive(FromField, Default)] #[darling(attributes(multipart), default)] struct FieldAttrs { @@ -51,16 +52,13 @@ struct ParsedField<'t> { /// Implements the `MultipartFormTrait` for a struct so that it can be used with the `MultipartForm` /// extractor. /// -/// # Examples -/// -/// ## Basic Use +/// # Basic Use /// /// Each field type should implement the `FieldReader` trait: /// /// ``` -/// # use actix_multipart::form::tempfile::Tempfile; -/// # use actix_multipart::form::text::Text; -/// # use actix_multipart::form::MultipartForm; +/// use actix_multipart::form::{tempfile::Tempfile, text::Text, MultipartForm}; +/// /// #[derive(MultipartForm)] /// struct ImageUpload { /// description: Text, @@ -69,17 +67,16 @@ struct ParsedField<'t> { /// } /// ``` /// -/// ## Optional and List Fields +/// # Optional and List Fields /// /// You can also use `Vec` and `Option` provided that `T: FieldReader`. /// -/// A [`Vec`] field corresponds to an upload with multiple parts under the -/// [same field name](https://www.rfc-editor.org/rfc/rfc7578#section-4.3). +/// A [`Vec`] field corresponds to an upload with multiple parts under the [same field +/// name](https://www.rfc-editor.org/rfc/rfc7578#section-4.3). /// /// ``` -/// # use actix_multipart::form::tempfile::Tempfile; -/// # use actix_multipart::form::text::Text; -/// # use actix_multipart::form::MultipartForm; +/// use actix_multipart::form::{tempfile::Tempfile, text::Text, MultipartForm}; +/// /// #[derive(MultipartForm)] /// struct Form { /// category: Option>, @@ -87,13 +84,13 @@ struct ParsedField<'t> { /// } /// ``` /// -/// ## Field Renaming +/// # Field Renaming /// /// You can use the `#[multipart(rename = "foo")]` attribute to receive a field by a different name. /// /// ``` -/// # use actix_multipart::form::tempfile::Tempfile; -/// # use actix_multipart::form::MultipartForm; +/// use actix_multipart::form::{tempfile::Tempfile, MultipartForm}; +/// /// #[derive(MultipartForm)] /// struct Form { /// #[multipart(rename = "files[]")] @@ -101,7 +98,7 @@ struct ParsedField<'t> { /// } /// ``` /// -/// ## Field Limits +/// # Field Limits /// /// You can use the `#[multipart(limit = "")]` attribute to set field level limits. The limit /// string is parsed using [parse_size]. @@ -109,40 +106,41 @@ struct ParsedField<'t> { /// Note: the form is also subject to the global limits configured using `MultipartFormConfig`. /// /// ``` -/// # use actix_multipart::form::tempfile::Tempfile; -/// # use actix_multipart::form::text::Text; -/// # use actix_multipart::form::MultipartForm; +/// use actix_multipart::form::{tempfile::Tempfile, text::Text, MultipartForm}; +/// /// #[derive(MultipartForm)] /// struct Form { -/// #[multipart(limit = "2KiB")] +/// #[multipart(limit = "2 KiB")] /// description: Text, -/// #[multipart(limit = "512MiB")] +/// +/// #[multipart(limit = "512 MiB")] /// files: Vec, /// } /// ``` /// -/// ## Unknown Fields +/// # Unknown Fields /// -/// By default fields with an unknown name are ignored. You can change this using the +/// By default fields with an unknown name are ignored. They can be rejected using the /// `#[multipart(deny_unknown_fields)]` attribute: /// /// ``` -/// # use actix_multipart::form::MultipartForm; +/// use actix_multipart::form::MultipartForm; +/// /// #[derive(MultipartForm)] /// #[multipart(deny_unknown_fields)] /// struct Form { } /// ``` /// -/// ## Duplicate Fields +/// # Duplicate Fields /// -/// You can change the behaviour for when multiple fields are received with the same name using the -/// `#[multipart(duplicate_field = "")]` attribute: +/// The behaviour for when multiple fields with the same name are received can be changed using the +/// `#[multipart(duplicate_field = "")]` attribute: /// -/// - "ignore": Extra fields are ignored (default). +/// - "ignore": (default) Extra fields are ignored. I.e., the first one is persisted. +/// - "deny": A `MultipartError::UnsupportedField` error response is returned. /// - "replace": Each field is processed, but only the last one is persisted. -/// - "deny": A `MultipartError::UnsupportedField` error is returned. /// -/// (Note this option does not apply to `Vec` fields) +/// Note that `Vec` fields will ignore this option. /// /// ``` /// # use actix_multipart::form::MultipartForm; @@ -157,16 +155,34 @@ pub fn impl_multipart_form(input: proc_macro::TokenStream) -> proc_macro::TokenS let input: syn::DeriveInput = parse_macro_input!(input); let name = &input.ident; - let str = match &input.data { - syn::Data::Struct(s) => s, - _ => panic!("This trait can only be derived for a struct"), - }; - let fields = match &str.fields { - syn::Fields::Named(n) => n, - _ => panic!("This trait can only be derived for a struct"), + + let data_struct = match &input.data { + syn::Data::Struct(data_struct) => data_struct, + _ => { + return TokenStream::from( + syn::Error::new( + Span::call_site(), + "This trait can only be derived for a struct", + ) + .to_compile_error(), + ) + } }; - let attrs: MultipartFormAttrs = match MultipartFormAttrs::from_derive_input(&input) { + let fields = match &data_struct.fields { + syn::Fields::Named(fields_named) => fields_named, + _ => { + return TokenStream::from( + syn::Error::new( + Span::call_site(), + "This trait can only be derived for a struct with named fields", + ) + .to_compile_error(), + ) + } + }; + + let attrs = match MultipartFormAttrs::from_derive_input(&input) { Ok(attrs) => attrs, Err(err) => return err.write_errors().into(), }; @@ -177,16 +193,19 @@ pub fn impl_multipart_form(input: proc_macro::TokenStream) -> proc_macro::TokenS .iter() .map(|field| { let rust_name = field.ident.as_ref().unwrap(); - let attrs: FieldAttrs = FieldAttrs::from_field(field)?; + let attrs = FieldAttrs::from_field(field).map_err(|err| err.write_errors())?; let serialization_name = attrs.rename.unwrap_or_else(|| rust_name.to_string()); - let limit = attrs.limit.map(|limit| { - usize::try_from( - parse_size(&limit) - .unwrap_or_else(|_| panic!("Unable to parse limit `{}`", limit)), - ) - .unwrap() - }); + let limit = match attrs.limit.map(|limit| match parse_size(&limit) { + Ok(size) => Ok(usize::try_from(size).unwrap()), + Err(err) => Err(syn::Error::new( + field.ident.as_ref().unwrap().span(), + format!("Unable to parse limit `{}`: {}", limit, err), + )), + }) { + Some(Err(err)) => return Err(TokenStream::from(err.to_compile_error())), + limit => limit.map(Result::unwrap), + }; Ok(ParsedField { serialization_name, @@ -195,17 +214,23 @@ pub fn impl_multipart_form(input: proc_macro::TokenStream) -> proc_macro::TokenS ty: &field.ty, }) }) - .collect::, darling::Error>>() + .collect::, TokenStream>>() { Ok(attrs) => attrs, - Err(err) => return err.write_errors().into(), + Err(err) => return err, }; // Check that field names are unique let mut set = HashSet::new(); - for f in &parsed { - if !set.insert(f.serialization_name.clone()) { - panic!("Multiple fields named: `{}`", f.serialization_name); + for field in &parsed { + if !set.insert(field.serialization_name.clone()) { + return TokenStream::from( + syn::Error::new( + field.rust_name.span(), + format!("Multiple fields named: `{}`", field.serialization_name), + ) + .to_compile_error(), + ); } } @@ -230,6 +255,7 @@ pub fn impl_multipart_form(input: proc_macro::TokenStream) -> proc_macro::TokenS for field in &parsed { let name = &field.serialization_name; let ty = &field.ty; + read_field_impl.extend(quote!( #name => ::std::boxed::Box::pin( <#ty as ::actix_multipart::form::FieldGroupReader>::handle_field(req, field, limits, state, #duplicate_field) diff --git a/actix-multipart/CHANGES.md b/actix-multipart/CHANGES.md index 1aaec2eab..87bb56386 100644 --- a/actix-multipart/CHANGES.md +++ b/actix-multipart/CHANGES.md @@ -1,13 +1,14 @@ # Changes ## Unreleased - 2022-xx-xx +- Added `MultipartForm` typed data extractor. [#2883] +- `Field::content_type()` now returns `Option<&mime::Mime>`. [#2880] - Minimum supported Rust version (MSRV) is now 1.59 due to transitive `time` dependency. -- `Field::content_type()` now returns `Option<&mime::Mime>` [#2880]. -- Added `MultipartForm` typed data extractor [#2883]. [#2880]: https://github.com/actix/actix-web/pull/2880 [#2883]: https://github.com/actix/actix-web/pull/2883 + ## 0.4.0 - 2022-02-25 - No significant changes since `0.4.0-beta.13`. diff --git a/actix-multipart/Cargo.toml b/actix-multipart/Cargo.toml index 5fe5507bf..85d3ad0db 100644 --- a/actix-multipart/Cargo.toml +++ b/actix-multipart/Cargo.toml @@ -24,7 +24,6 @@ path = "src/lib.rs" [dependencies] actix-multipart-derive = { version = "=0.4.0", optional = true } -actix-http = "3" actix-utils = "3" actix-web = { version = "4", default-features = false } @@ -37,14 +36,15 @@ local-waker = "0.1" log = "0.4" memchr = "2.5" mime = "0.3" -serde = "1.0" -serde_json = "1.0" -serde_plain = "1.0" +serde = "1" +serde_json = "1" +serde_plain = "1" # TODO(MSRV 1.60): replace with dep: prefix tempfile-dep = { package = "tempfile", version = "3.3.0", optional = true } tokio = { version = "1.13.1", features = ["sync"] } [dev-dependencies] +actix-http = "3" actix-multipart-rfc7578 = "0.10" actix-rt = "2.2" actix-test = "0.1.0" diff --git a/actix-multipart/src/form/text.rs b/actix-multipart/src/form/text.rs index 43e08c335..bccf66d98 100644 --- a/actix-multipart/src/form/text.rs +++ b/actix-multipart/src/form/text.rs @@ -26,7 +26,10 @@ impl Text { } } -impl<'t, T: DeserializeOwned + 'static> FieldReader<'t> for Text { +impl<'t, T> FieldReader<'t> for Text +where + T: DeserializeOwned + 'static, +{ type Future = LocalBoxFuture<'t, Result>; fn read_field(req: &'t HttpRequest, field: Field, limits: &'t mut Limits) -> Self::Future { @@ -43,7 +46,7 @@ impl<'t, T: DeserializeOwned + 'static> FieldReader<'t> for Text { true }; - if !valid && config.validate_content_type { + if !valid { return Err(MultipartError::Field { field_name, source: config.map_error(req, TextError::ContentType), diff --git a/actix-multipart/src/server.rs b/actix-multipart/src/server.rs index 718163344..d1448b328 100644 --- a/actix-multipart/src/server.rs +++ b/actix-multipart/src/server.rs @@ -864,7 +864,7 @@ impl PayloadBuffer { mod tests { use std::time::Duration; - use actix_http::h1::Payload; + use actix_http::h1; use actix_web::{ http::header::{DispositionParam, DispositionType}, rt, @@ -1124,7 +1124,7 @@ mod tests { #[actix_rt::test] async fn test_basic() { - let (_, payload) = Payload::create(false); + let (_, payload) = h1::Payload::create(false); let mut payload = PayloadBuffer::new(payload); assert_eq!(payload.buf.len(), 0); @@ -1134,7 +1134,7 @@ mod tests { #[actix_rt::test] async fn test_eof() { - let (mut sender, payload) = Payload::create(false); + let (mut sender, payload) = h1::Payload::create(false); let mut payload = PayloadBuffer::new(payload); assert_eq!(None, payload.read_max(4).unwrap()); @@ -1150,7 +1150,7 @@ mod tests { #[actix_rt::test] async fn test_err() { - let (mut sender, payload) = Payload::create(false); + let (mut sender, payload) = h1::Payload::create(false); let mut payload = PayloadBuffer::new(payload); assert_eq!(None, payload.read_max(1).unwrap()); sender.set_error(PayloadError::Incomplete(None)); @@ -1159,7 +1159,7 @@ mod tests { #[actix_rt::test] async fn test_readmax() { - let (mut sender, payload) = Payload::create(false); + let (mut sender, payload) = h1::Payload::create(false); let mut payload = PayloadBuffer::new(payload); sender.feed_data(Bytes::from("line1")); @@ -1176,7 +1176,7 @@ mod tests { #[actix_rt::test] async fn test_readexactly() { - let (mut sender, payload) = Payload::create(false); + let (mut sender, payload) = h1::Payload::create(false); let mut payload = PayloadBuffer::new(payload); assert_eq!(None, payload.read_exact(2)); @@ -1194,7 +1194,7 @@ mod tests { #[actix_rt::test] async fn test_readuntil() { - let (mut sender, payload) = Payload::create(false); + let (mut sender, payload) = h1::Payload::create(false); let mut payload = PayloadBuffer::new(payload); assert_eq!(None, payload.read_until(b"ne").unwrap()); @@ -1235,7 +1235,7 @@ mod tests { #[actix_rt::test] async fn test_multipart_payload_consumption() { // with sample payload and HttpRequest with no headers - let (_, inner_payload) = Payload::create(false); + let (_, inner_payload) = h1::Payload::create(false); let mut payload = actix_web::dev::Payload::from(inner_payload); let req = TestRequest::default().to_http_request();