Repository: wafflespeanut/paperclip
Pull request #287 (2021-01-19 00:02:00)

fix(tuples): make doc work in tuple struct path
Add missing schema to unnamed structs with >1 element.
Fix issue with unnamed structs of 1 element caused by:
6429ef4
Fix usage of structs with Path with tuples: Path<(A,B,C)>

Unnamed struct element now get named starting from 0,1..... so that we
can keep the list of properties, though they should not really be used
anywhere else than a Path.

tiagolobocastro: > That looks good, can you please extend test to see if it's handling struct with 2 unnamed arguments? It's handled a bit differently and produces `object` now, would be great if that inherits description too

Yeah it produces object probably because unnamed structs with >1 elem are left with an empty schema.
I don't know if it's possible to improve things but I'll give it a go.

tiagolobocastro: Ok so we can get unnamed structs working by naming them such as they sort of are in rust, with names starting with 0, much like you'd do self.0?
That works pretty well for a Uri Path!
```
struct Test(String, String);
fn test(web::Path(test): web::Path<Test>) ...
```
Less so for a Body because you'd see the openapi scheme as:
```
{
  "0": "string",
  "1": "string"
}
```
But, that's a user choice I suppose... 
I'll push this up once I have time to tidy it up and write up the tests for this.
tiagolobocastro: @dunnock I've pushed, please let me know what you think.
tiagolobocastro: > That's awesome! Sorry it took a while to dig into that

no worries, thank you!
wafflespeanut: Thank you for working on this!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0f121ebe..86f3b920 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - Optional type aliases like `type Email = Option<String>` will not be added to the `required` fields.
+- Actix plugin: Path tuples now inherit field names and descriptions from doc comments
 
 ## [0.5.0] - 2020-11-28
 ### Added
diff --git a/core/src/v2/actix.rs b/core/src/v2/actix.rs
index 108fdea2..2794f99f 100644
--- a/core/src/v2/actix.rs
+++ b/core/src/v2/actix.rs
@@ -321,10 +321,10 @@ macro_rules! impl_param_extractor ({ $ty:ty => $container:ident } => {
                     data_type: def.data_type,
                     format: def.format,
                     enum_: def.enum_,
+                    description: def.description,
                     ..Default::default()
                 }));
             }
-
             for (k, v) in def.properties {
                 op.parameters.push(Either::Right(Parameter {
                     in_: ParameterIn::$container,
@@ -388,19 +388,36 @@ macro_rules! impl_path_tuple ({ $($ty:ident),+ } => {
         where $($ty: Apiv2Schema,)+
     {
         fn update_parameter(op: &mut DefaultOperationRaw) {
-            // NOTE: We're setting empty name, because we don't know
-            // the name in this context. We'll get it when we add services.
             $(
                 let def = $ty::raw_schema();
-                op.parameters.push(Either::Right(Parameter {
-                    name: String::new(),
-                    in_: ParameterIn::Path,
-                    required: true,
-                    data_type: def.data_type,
-                    format: def.format,
-                    enum_: def.enum_,
-                    ..Default::default()
-                }));
+                if def.properties.is_empty() {
+                    op.parameters.push(Either::Right(Parameter {
+                        // NOTE: We're setting empty name, because we don't know
+                        // the name in this context. We'll get it when we add services.
+                        name: String::new(),
+                        in_: ParameterIn::Path,
+                        required: true,
+                        data_type: def.data_type,
+                        format: def.format,
+                        enum_: def.enum_,
+                        description: def.description,
+                        ..Default::default()
+                    }));
+                }
+                for (k, v) in def.properties {
+                    op.parameters.push(Either::Right(Parameter {
+                        in_: ParameterIn::Path,
+                        required: def.required.contains(&k),
+                        data_type: v.data_type,
+                        format: v.format,
+                        enum_: v.enum_,
+                        description: v.description,
+                        collection_format: None, // this defaults to csv
+                        items: v.items.as_deref().map(map_schema_to_items),
+                        name: k,
+                        ..Default::default()
+                    }));
+                }
             )+
         }
     }
diff --git a/macros/src/actix.rs b/macros/src/actix.rs
index 77b8211b..e580c301 100644
--- a/macros/src/actix.rs
+++ b/macros/src/actix.rs
@@ -493,15 +493,7 @@ pub fn emit_v2_definition(input: TokenStream) -> TokenStream {
             match &s.fields {
                 Fields::Named(ref f) => handle_field_struct(f, &props, &mut props_gen),
                 Fields::Unnamed(ref f) => {
-                    if f.unnamed.len() == 1 {
-                        handle_unnamed_field_struct(f, &mut props_gen)
-                    } else {
-                        emit_warning!(
-                            f.span().unwrap(),
-                            "tuple structs do not have named fields and hence will have empty schema.";
-                            help = "{}", &*EMPTY_SCHEMA_HELP;
-                        );
-                    }
+                    handle_unnamed_field_struct(f, &item_ast.attrs, &mut props_gen)
                 }
                 Fields::Unit => {
                     emit_warning!(
@@ -520,6 +512,7 @@ pub fn emit_v2_definition(input: TokenStream) -> TokenStream {
     };
 
     let schema_name = name.to_string();
+    let props_gen_empty = props_gen.is_empty();
     let gen = quote! {
         impl #impl_generics paperclip::v2::schema::Apiv2Schema for #name #ty_generics #where_clause {
             const NAME: Option<&'static str> = Some(#schema_name);
@@ -535,6 +528,12 @@ pub fn emit_v2_definition(input: TokenStream) -> TokenStream {
                     .. Default::default()
                 };
                 #props_gen
+                // props_gen may override the schema for unnamed structs with 1 element
+                // as it replaces the struct type with inner type.
+                // make sure we set the name properly if props_gen is not empty
+                if !#props_gen_empty {
+                    schema.name = Some(#schema_name.into());
+                }
                 schema
             }
         }
@@ -787,13 +786,59 @@ fn get_field_type(field: &Field) -> Option<proc_macro2::TokenStream> {
 }
 
 /// Generates code for a tuple struct with fields.
-fn handle_unnamed_field_struct(fields: &FieldsUnnamed, props_gen: &mut proc_macro2::TokenStream) {
-    let field = fields.unnamed.iter().next().unwrap();
+fn handle_unnamed_field_struct(
+    fields: &FieldsUnnamed,
+    struct_attr: &[Attribute],
+    props_gen: &mut proc_macro2::TokenStream,
+) {
+    if fields.unnamed.len() == 1 {
+        let field = fields.unnamed.iter().next().unwrap();
+
+        if let Some(ty_ref) = get_field_type(&field) {
+            let docs = extract_documentation(struct_attr);
+            let docs = docs.trim();
+
+            props_gen.extend(quote!({
+                let mut s = #ty_ref::raw_schema();
+                if !#docs.is_empty() {
+                    s.description = Some(#docs.to_string());
+                }
+                schema = s;
+            }));
+        }
+    } else {
+        for (inner_field_id, field) in (&fields.unnamed).into_iter().enumerate() {
+            let ty_ref = get_field_type(&field);
+
+            let docs = extract_documentation(&field.attrs);
+            let docs = docs.trim();
+
+            let mut gen = if !SerdeFlatten::exists(&field.attrs) {
+                // this is really not what we'd want to do because that's not how the
+                // deserialized struct will be like, ideally we want an actual tuple
+                // this type should therefore not be used for anything else than `Path`
+                quote!({
+                    let mut s = #ty_ref::raw_schema();
+                    if !#docs.is_empty() {
+                        s.description = Some(#docs.to_string());
+                    }
+                    schema.properties.insert(#inner_field_id.to_string(), s.into());
+                })
+            } else {
+                quote!({
+                    let s = #ty_ref::raw_schema();
+                    schema.properties.extend(s.properties);
+                })
+            };
 
-    if let Some(ty_ref) = get_field_type(&field) {
-        props_gen.extend(quote!({
-            schema = #ty_ref::raw_schema();
-        }));
+            gen.extend(quote! {
+                if #ty_ref::REQUIRED {
+                    schema.required.insert(#inner_field_id.to_string());
+                }
+            });
+
+            props_gen.extend(gen);
+        }
     }
 }
 
diff --git a/plugins/actix-web/src/lib.rs b/plugins/actix-web/src/lib.rs
index 0605f9d6..736515d7 100644
--- a/plugins/actix-web/src/lib.rs
+++ b/plugins/actix-web/src/lib.rs
@@ -169,12 +169,12 @@ where
     where
         F: actix_service::IntoServiceFactory<U>,
         U: ServiceFactory<
-            Config = (),
-            Request = ServiceRequest,
-            Response = ServiceResponse,
-            Error = Error,
-            InitError = (),
-        > + 'static,
+                Config = (),
+                Request = ServiceRequest,
+                Response = ServiceResponse,
+                Error = Error,
+                InitError = (),
+            > + 'static,
         U::InitError: Debug,
     {
         self.inner = self.inner.take().map(|a| a.default_service(f));
diff --git a/plugins/actix-web/src/web.rs b/plugins/actix-web/src/web.rs
index 1a775e95..d7ec1c27 100644
--- a/plugins/actix-web/src/web.rs
+++ b/plugins/actix-web/src/web.rs
@@ -59,12 +59,12 @@ impl Resource {
 impl<T> HttpServiceFactory for Resource<actix_web::Resource<T>>
 where
     T: ServiceFactory<
-        Config = (),
-        Request = ServiceRequest,
-        Response = ServiceResponse,
-        Error = Error,
-        InitError = (),
-    > + 'static,
+            Config = (),
+            Request = ServiceRequest,
+            Response = ServiceResponse,
+            Error = Error,
+            InitError = (),
+        > + 'static,
 {
     fn register(self, config: &mut AppService) {
         self.inner.register(config)
@@ -74,12 +74,12 @@ where
 impl<T> IntoServiceFactory<T> for Resource<actix_web::Resource<T>>
 where
     T: ServiceFactory<
-        Config = (),
-        Request = ServiceRequest,
-        Response = ServiceResponse,
-        Error = Error,
-        InitError = (),
-    > + 'static,
+            Config = (),
+            Request = ServiceRequest,
+            Response = ServiceResponse,
+            Error = Error,
+            InitError = (),
+        > + 'static,
 {
     fn into_factory(self) -> T {
         self.inner.into_factory()
@@ -242,12 +242,12 @@ where
     where
         F: actix_service::IntoServiceFactory<U>,
         U: ServiceFactory<
-            Config = (),
-            Request = ServiceRequest,
-            Response = ServiceResponse,
-            Error = Error,
-            InitError = (),
-        > + 'static,
+                Config = (),
+                Request = ServiceRequest,
+                Response = ServiceResponse,
+                Error = Error,
+                InitError = (),
+            > + 'static,
         U::InitError: Debug,
     {
         self.inner = self.inner.default_service(f);
@@ -302,12 +302,12 @@ impl Scope {
 impl<T> HttpServiceFactory for Scope<actix_web::Scope<T>>
 where
     T: ServiceFactory<
-        Config = (),
-        Request = ServiceRequest,
-        Response = ServiceResponse,
-        Error = Error,
-        InitError = (),
-    > + 'static,
+            Config = (),
+            Request = ServiceRequest,
+            Response = ServiceResponse,
+            Error = Error,
+            InitError = (),
+        > + 'static,
 {
     fn register(self, config: &mut AppService) {
         if let Some(s) = self.inner {
@@ -382,12 +382,12 @@ where
     where
         F: actix_service::IntoServiceFactory<U>,
         U: ServiceFactory<
-            Config = (),
-            Request = ServiceRequest,
-            Response = ServiceResponse,
-            Error = Error,
-            InitError = (),
-        > + 'static,
+                Config = (),
+                Request = ServiceRequest,
+                Response = ServiceResponse,
+                Error = Error,
+                InitError = (),
+            > + 'static,
         U::InitError: Debug,
     {
         self.inner = self.inner.map(|s| s.default_service(f));


Test patch:
diff --git a/tests/test_app.rs b/tests/test_app.rs
index 6de23596..1891a771 100644
--- a/tests/test_app.rs
+++ b/tests/test_app.rs
@@ -351,6 +351,23 @@ fn test_params() {
         name: String,
     }
 
+    /// KnownBadge Id Doc
+    #[derive(Serialize, Deserialize, Apiv2Schema)]
+    struct KnownBadgeId(String);
+
+    /// KnownBadge Id2 Doc
+    #[derive(Deserialize, Apiv2Schema)]
+    struct KnownBadgeId2(u64);
+
+    /// KnownBadge Id3 Doc
+    #[derive(Deserialize, Apiv2Schema)]
+    struct KnownBadgeId3(
+        /// Number Doc
+        pub u64,
+        /// String Doc
+        pub String,
+    );
+
     #[derive(Deserialize, Apiv2Schema)]
     struct BadgeParams {
         res: Option<u16>,
@@ -411,6 +428,21 @@ fn test_params() {
         ready("")
     }
 
+    #[api_v2_operation]
+    fn get_known_badge_3(
+        _p: web::Path<KnownBadgeId>,
+    ) -> impl Future<Output = Result<web::Json<KnownBadgeId>, ()>> {
+        futures::future::ok(web::Json(KnownBadgeId("id".into())))
+    }
+
+    #[api_v2_operation]
+    fn get_known_badge_4(
+        _p1: web::Path<KnownBadgeId>,
+        _p2: web::Path<(KnownBadgeId2, KnownBadgeId3)>,
+    ) -> impl Future<Output = &'static str> {
+        ready("")
+    }
+
     #[api_v2_operation]
     fn post_badge_1(
         _p: web::Path<KnownResourceBadge>,
@@ -464,6 +496,14 @@ fn test_params() {
                                         .route(web::get().to(get_known_badge_2))
                                         .route(web::post().to(post_badge_2)),
                                 )
+                                .service(
+                                    web::resource("/v/{id}")
+                                        .route(web::get().to(get_known_badge_3)),
+                                )
+                                .service(
+                                    web::resource("/v/{id}/{id2}/{id3}/{id4}")
+                                        .route(web::get().to(get_known_badge_4)),
+                                )
                                 .service(
                                     web::resource("/v")
                                         .route(web::post().to(post_badge_3))
@@ -507,7 +547,11 @@ fn test_params() {
                                 }
                             },
                             "type":"object"
-                        }
+                        },
+                        "KnownBadgeId": {
+                            "description": "KnownBadge Id Doc",
+                            "type": "string"
+                        },
                     },
                     "info": {
                         "title": "",
@@ -812,6 +856,65 @@ fn test_params() {
                                 }
                             }
                         },
+                        "/api/v2/{resource}/v/{id}": {
+                            "get": {
+                                "parameters": [
+                                    {
+                                        "description": "KnownBadge Id Doc",
+                                        "in": "path",
+                                        "name": "id",
+                                        "required": true,
+                                        "type": "string"
+                                    }
+                                ],
+                                "responses": {
+                                    "200": {
+                                        "description": "OK",
+                                        "schema": {
+                                            "$ref": "#/definitions/KnownBadgeId"
+                                        }
+                                    }
+                                }
+                            }
+                        },
+                        "/api/v2/{resource}/v/{id}/{id2}/{id3}/{id4}": {
+                            "get": {
+                                "parameters": [
+                                    {
+                                        "description": "KnownBadge Id Doc",
+                                        "in": "path",
+                                        "name": "id",
+                                        "required": true,
+                                        "type": "string"
+                                    },
+                                    {
+                                        "description": "KnownBadge Id2 Doc",
+                                        "format": "int64",
+                                        "in": "path",
+                                        "name": "id2",
+                                        "required": true,
+                                        "type": "integer"
+                                    },
+                                    {
+                                        "description": "Number Doc",
+                                        "format": "int64",
+                                        "in": "path",
+                                        "name": "id3",
+                                        "required": true,
+                                        "type": "integer"
+                                    },
+                                    {
+                                        "description": "String Doc",
+                                        "in": "path",
+                                        "name": "id4",
+                                        "required": true,
+                                        "type": "string"
+                                    },
+                                ],
+                                "responses": {
+                                }
+                            }
+                        },
                         "/api/v2/{resource}/v/{name}": {
                             "get": {
                                 "parameters": [
@@ -2091,12 +2194,12 @@ fn test_multiple_method_routes() {
         F: Fn() -> App<T, B> + Clone + Send + Sync + 'static,
         B: MessageBody + 'static,
         T: ServiceFactory<
-            Config = (),
-            Request = ServiceRequest,
-            Response = ServiceResponse<B>,
-            Error = Error,
-            InitError = (),
-        > + 'static,
+                Config = (),
+                Request = ServiceRequest,
+                Response = ServiceResponse<B>,
+                Error = Error,
+                InitError = (),
+            > + 'static,
     {
         run_and_check_app(f, |addr| {
             let resp = CLIENT
@@ -2576,12 +2679,12 @@ where
     F: Fn() -> App<T, B> + Clone + Send + Sync + 'static,
     B: MessageBody + 'static,
     T: ServiceFactory<
-        Config = (),
-        Request = ServiceRequest,
-        Response = ServiceResponse<B>,
-        Error = Error,
-        InitError = (),
-    > + 'static,
+            Config = (),
+            Request = ServiceRequest,
+            Response = ServiceResponse<B>,
+            Error = Error,
+            InitError = (),
+        > + 'static,
     G: Fn(String) -> U,
 {
     let (tx, rx) = mpsc::channel();