From 9abf383ea02a1888c474f976920fa053751cc0e4 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Thu, 20 Jan 2022 16:29:06 -0800 Subject: [PATCH 1/2] Use the first line of endpoint doc comments as the OpenAPI summary --- Cargo.lock | 25 +++ dropshot/src/api_description.rs | 10 +- dropshot/src/router.rs | 1 + dropshot/tests/test_openapi.json | 6 +- dropshot/tests/test_openapi.rs | 6 +- dropshot/tests/test_openapi_fuller.json | 6 +- dropshot_endpoint/Cargo.toml | 7 +- dropshot_endpoint/src/lib.rs | 199 +++++++++++++++++++----- 8 files changed, 211 insertions(+), 49 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cb7f4b1c5..16226c142 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -271,6 +271,7 @@ version = "0.6.1-dev" dependencies = [ "proc-macro2", "quote", + "schema", "serde", "serde_tokenstream", "syn", @@ -1084,6 +1085,30 @@ dependencies = [ "winapi", ] +[[package]] +name = "schema" +version = "0.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9c264b3fd73f2b0b7360f7ae10779dbcb3a3d0f2b1326347c16cd4da32a0db43" +dependencies = [ + "proc-macro2", + "quote", + "schema-derive", + "syn", +] + +[[package]] +name = "schema-derive" +version = "0.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1c5081f42984166c05c533e4ae3a75e8c1f973071be7ef442912d7f83fc9e762" +dependencies = [ + "paste", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "schemars" version = "0.8.8" diff --git a/dropshot/src/api_description.rs b/dropshot/src/api_description.rs index e578154dd..03d144074 100644 --- a/dropshot/src/api_description.rs +++ b/dropshot/src/api_description.rs @@ -36,6 +36,7 @@ pub struct ApiEndpoint { pub path: String, pub parameters: Vec, pub response: ApiEndpointResponse, + pub summary: Option, pub description: Option, pub tags: Vec, pub paginated: bool, @@ -63,6 +64,7 @@ impl<'a, Context: ServerContext> ApiEndpoint { path: path.to_string(), parameters: func_parameters.parameters, response, + summary: None, description: None, tags: vec![], paginated: func_parameters.paginated, @@ -70,6 +72,11 @@ impl<'a, Context: ServerContext> ApiEndpoint { } } + pub fn summary(mut self, description: T) -> Self { + self.summary.replace(description.to_string()); + self + } + pub fn description(mut self, description: T) -> Self { self.description.replace(description.to_string()); self @@ -465,6 +472,7 @@ impl ApiDescription { }; let mut operation = openapiv3::Operation::default(); operation.operation_id = Some(endpoint.operation_id.clone()); + operation.summary = endpoint.summary.clone(); operation.description = endpoint.description.clone(); operation.tags = endpoint.tags.clone(); @@ -812,7 +820,7 @@ fn j2oas_schema_object( (None, None) => { openapiv3::SchemaKind::Any(openapiv3::AnySchema::default()) } - _ => panic!("invalid"), + _ => panic!("invalid {:#?}", obj), }; let mut data = openapiv3::SchemaData::default(); diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index 16915af2f..4eb05bb82 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -849,6 +849,7 @@ mod test { success: None, description: None, }, + summary: None, description: None, tags: vec![], paginated: false, diff --git a/dropshot/tests/test_openapi.json b/dropshot/tests/test_openapi.json index 8e9305d85..6d37e80f6 100644 --- a/dropshot/tests/test_openapi.json +++ b/dropshot/tests/test_openapi.json @@ -239,7 +239,8 @@ }, "/test/person": { "get": { - "description": "This is a multi-line comment. It uses Rust-style.", + "summary": "Rust style comment", + "description": "This is a multi-line comment.", "operationId": "handler1", "responses": { "200": { @@ -328,7 +329,8 @@ }, "/test/woman": { "put": { - "description": "This is a multi-line comment. It uses C-style.", + "summary": "C-style comment", + "description": "This is a multi-line comment.", "operationId": "handler2", "parameters": [ { diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/test_openapi.rs index b3d25a429..703037288 100644 --- a/dropshot/tests/test_openapi.rs +++ b/dropshot/tests/test_openapi.rs @@ -15,9 +15,10 @@ use std::{io::Cursor, str::from_utf8, sync::Arc}; method = GET, path = "/test/person", }] +/// Rust style comment +/// /// This is a multi- /// line comment. -/// It uses Rust-style. async fn handler1( _rqctx: Arc>, ) -> Result, HttpError> { @@ -36,9 +37,10 @@ struct QueryArgs { path = "/test/woman", }] /** + * C-style comment + * * This is a multi- * line comment. - * It uses C-style. */ async fn handler2( _rqctx: Arc>, diff --git a/dropshot/tests/test_openapi_fuller.json b/dropshot/tests/test_openapi_fuller.json index fe65fb6bf..5d8a37ce4 100644 --- a/dropshot/tests/test_openapi_fuller.json +++ b/dropshot/tests/test_openapi_fuller.json @@ -247,7 +247,8 @@ }, "/test/person": { "get": { - "description": "This is a multi-line comment. It uses Rust-style.", + "summary": "Rust style comment", + "description": "This is a multi-line comment.", "operationId": "handler1", "responses": { "200": { @@ -336,7 +337,8 @@ }, "/test/woman": { "put": { - "description": "This is a multi-line comment. It uses C-style.", + "summary": "C-style comment", + "description": "This is a multi-line comment.", "operationId": "handler2", "parameters": [ { diff --git a/dropshot_endpoint/Cargo.toml b/dropshot_endpoint/Cargo.toml index f92d0d046..0fc4cd351 100644 --- a/dropshot_endpoint/Cargo.toml +++ b/dropshot_endpoint/Cargo.toml @@ -13,12 +13,15 @@ proc-macro = true [dependencies] proc-macro2 = "1" quote = "1" -serde_tokenstream = "0.1.3" +serde_tokenstream = "0.1" [dependencies.serde] version = "1.0" features = [ "derive" ] [dependencies.syn] -version = "1.0.85" +version = "1.0" features = [ "parsing", "printing" ] + +[dev-dependencies] +schema = "0.0.1" diff --git a/dropshot_endpoint/src/lib.rs b/dropshot_endpoint/src/lib.rs index d10edb950..e2ea9b43f 100644 --- a/dropshot_endpoint/src/lib.rs +++ b/dropshot_endpoint/src/lib.rs @@ -166,18 +166,30 @@ fn do_endpoint( let method_ident = format_ident!("{}", method); let visibility = &ast.vis; - let description_text_provided = extract_doc_from_attrs(&ast.attrs); - let description_text_annotated = format!( - "API Endpoint: {}", - description_text_provided.as_ref().unwrap_or(&name_str).as_str().trim() - ); + let (summary_text, description_text) = extract_doc_from_attrs(&ast.attrs); + let comment_text = { + let mut buf = String::new(); + buf.push_str("API Endpoint: "); + buf.push_str(&name_str); + if let Some(s) = &summary_text { + buf.push_str("\n"); + buf.push_str(&s); + } + if let Some(s) = &description_text { + buf.push_str("\n"); + buf.push_str(&s); + } + buf + }; let description_doc_comment = quote! { - #[doc = #description_text_annotated] + #[doc = #comment_text] }; - let description = description_text_provided.map(|description| { - quote! { - .description(#description) - } + + let summary = summary_text.map(|summary| { + quote! { .summary(#summary) } + }); + let description = description_text.map(|description| { + quote! { .description(#description) } }); let tags = metadata @@ -351,6 +363,7 @@ fn do_endpoint( #dropshot::Method::#method_ident, #path, ) + #summary #description #(#tags)* #visible @@ -421,49 +434,80 @@ fn to_compile_errors(errors: Vec) -> proc_macro2::TokenStream { quote!(#(#compile_errors)*) } -fn extract_doc_from_attrs(attrs: &[syn::Attribute]) -> Option { +fn extract_doc_from_attrs( + attrs: &[syn::Attribute], +) -> (Option, Option) { let doc = syn::Ident::new("doc", proc_macro2::Span::call_site()); - attrs - .iter() - .filter_map(|attr| { - if let Ok(meta) = attr.parse_meta() { - if let syn::Meta::NameValue(nv) = meta { - if nv.path.is_ident(&doc) { - if let syn::Lit::Str(s) = nv.lit { - return Some(normalize_comment_string(s.value())); - } + let mut lines = attrs.iter().flat_map(|attr| { + if let Ok(meta) = attr.parse_meta() { + if let syn::Meta::NameValue(nv) = meta { + if nv.path.is_ident(&doc) { + if let syn::Lit::Str(s) = nv.lit { + return normalize_comment_string(s.value()); } } } - None - }) - .fold(None, |acc, comment| match acc { - None => Some(comment), - Some(prev) if prev.ends_with('-') => { - Some(format!("{}{}", prev, comment)) + } + Vec::new() + }); + + let summary = lines.next(); + let first = lines.next(); + + match (summary, first) { + (None, _) => (None, None), + (summary, None) => (summary, None), + (Some(summary), Some(first)) => ( + Some(summary), + Some(lines.fold(first, |acc, comment| { + if acc.ends_with('-') || acc.is_empty() { + format!("{}{}", acc, comment) + } else { + format!("{} {}", acc, comment) + } + })), + ), + } +} +fn normalize_comment_string(s: String) -> Vec { + let mut ret = s + .split('\n') + .enumerate() + .map(|(idx, s)| { + if idx == 0 { + s.trim_start() + } else { + let trimmed = s.trim_start(); + trimmed.strip_prefix("* ").unwrap_or_else(|| { + trimmed.strip_prefix('*').unwrap_or(trimmed) + }) } - Some(prev) => Some(format!("{} {}", prev, comment)), }) -} + .map(|s| s.trim_end()) + .map(ToString::to_string) + .collect::>(); -fn normalize_comment_string(s: String) -> String { - let ret = s - .replace("-\n * ", "-") - .replace("\n * ", " ") - .trim_end_matches(&[' ', '\n'] as &[char]) - .to_string(); - if ret.starts_with(' ') && !ret.starts_with(" ") { - // Trim off the first character if the comment - // begins with a single space. - ret.as_str()[1..].to_string() - } else { - ret + loop { + match ret.first() { + Some(s) if s.is_empty() => { + let _ = ret.remove(0); + } + _ => break, + } + } + + while ret.last().map(|s| s.is_empty()) == Some(true) { + ret.pop(); } + + ret } #[cfg(test)] mod tests { + use schema::Schema; + use super::*; #[test] @@ -1104,4 +1148,79 @@ mod tests { Some("Endpoint requires arguments".to_string()) ); } + + #[test] + fn test_extract_summary_description() { + /** + * Javadoc summary + * Maybe there's another name for these... + * ... but Java is the first place I saw these types of comments. + */ + #[derive(Schema)] + struct JavadocComments; + assert_eq!( + extract_doc_from_attrs(&JavadocComments::schema().attrs), + ( + Some("Javadoc summary".to_string()), + Some( + "Maybe there's another name for these... ... but Java \ + is the first place I saw these types of comments." + .to_string() + ) + ) + ); + + /** + * Javadoc summary + * + * Skip that blank. + */ + #[derive(Schema)] + struct JavadocCommentsWithABlank; + assert_eq!( + extract_doc_from_attrs(&JavadocCommentsWithABlank::schema().attrs), + ( + Some("Javadoc summary".to_string()), + Some("Skip that blank.".to_string()) + ) + ); + + /// Rustdoc summary + /// Did other folks do this or what this an invention I can right- + /// fully ascribe to Rust? + #[derive(Schema)] + struct RustdocComments; + assert_eq!( + extract_doc_from_attrs(&RustdocComments::schema().attrs), + ( + Some("Rustdoc summary".to_string()), + Some( + "Did other folks do this or what this an invention \ + I can right-fully ascribe to Rust?" + .to_string() + ) + ) + ); + + /// Rustdoc summary + /// + /// Skip that blank. + #[derive(Schema)] + struct RustdocCommentsWithABlank; + assert_eq!( + extract_doc_from_attrs(&RustdocCommentsWithABlank::schema().attrs), + ( + Some("Rustdoc summary".to_string()), + Some("Skip that blank.".to_string()) + ) + ); + + /// Just a summary + #[derive(Schema)] + struct JustTheSummary; + assert_eq!( + extract_doc_from_attrs(&JustTheSummary::schema().attrs), + (Some("Just a summary".to_string()), None) + ); + } } From b5cc3519d5429e814cc7793f40b0cfdcb6496935 Mon Sep 17 00:00:00 2001 From: "Adam H. Leventhal" Date: Thu, 20 Jan 2022 17:12:45 -0800 Subject: [PATCH 2/2] modify comment handling; improve testing; fix test failure --- dropshot_endpoint/src/lib.rs | 92 +++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 32 deletions(-) diff --git a/dropshot_endpoint/src/lib.rs b/dropshot_endpoint/src/lib.rs index e2ea9b43f..7fd496e1d 100644 --- a/dropshot_endpoint/src/lib.rs +++ b/dropshot_endpoint/src/lib.rs @@ -452,7 +452,13 @@ fn extract_doc_from_attrs( Vec::new() }); - let summary = lines.next(); + // Skip initial blank lines; they make for excessively terse summaries. + let summary = loop { + match lines.next() { + Some(s) if s.is_empty() => (), + next => break next, + } + }; let first = lines.next(); match (summary, first) { @@ -460,48 +466,46 @@ fn extract_doc_from_attrs( (summary, None) => (summary, None), (Some(summary), Some(first)) => ( Some(summary), - Some(lines.fold(first, |acc, comment| { - if acc.ends_with('-') || acc.is_empty() { - format!("{}{}", acc, comment) - } else { - format!("{} {}", acc, comment) - } - })), + Some( + lines + .fold(first, |acc, comment| { + if acc.ends_with('-') + || acc.ends_with('\n') + || acc.is_empty() + { + // Continuation lines and newlines. + format!("{}{}", acc, comment) + } else if comment.is_empty() { + // Handle fully blank comments as newlines we keep. + format!("{}\n", acc) + } else { + // Default to space-separating comment fragments. + format!("{} {}", acc, comment) + } + }) + .trim_end() + .to_string(), + ), ), } } fn normalize_comment_string(s: String) -> Vec { - let mut ret = s - .split('\n') + s.split('\n') .enumerate() .map(|(idx, s)| { + // Rust-style comments are intrinsically single-line. We don't want + // to trim away formatting such as an initial '*'. if idx == 0 { - s.trim_start() + s.trim_start().trim_end() } else { - let trimmed = s.trim_start(); + let trimmed = s.trim_start().trim_end(); trimmed.strip_prefix("* ").unwrap_or_else(|| { trimmed.strip_prefix('*').unwrap_or(trimmed) }) } }) - .map(|s| s.trim_end()) .map(ToString::to_string) - .collect::>(); - - loop { - match ret.first() { - Some(s) if s.is_empty() => { - let _ = ret.remove(0); - } - _ => break, - } - } - - while ret.last().map(|s| s.is_empty()) == Some(true) { - ret.pop(); - } - - ret + .collect() } #[cfg(test)] @@ -1007,11 +1011,11 @@ mod tests { }; #[allow(non_camel_case_types, missing_docs)] - #[doc = "API Endpoint: handle \"xyz\" requests"] + #[doc = "API Endpoint: handler_xyz\nhandle \"xyz\" requests"] struct handler_xyz {} #[allow(non_upper_case_globals, missing_docs)] - #[doc = "API Endpoint: handle \"xyz\" requests"] + #[doc = "API Endpoint: handler_xyz\nhandle \"xyz\" requests"] const handler_xyz: handler_xyz = handler_xyz {}; impl From @@ -1032,7 +1036,7 @@ mod tests { dropshot::Method::GET, "/a/b/c", ) - .description("handle \"xyz\" requests") + .summary("handle \"xyz\" requests") } } }; @@ -1185,6 +1189,14 @@ mod tests { ) ); + /** Terse Javadoc summary */ + #[derive(Schema)] + struct JavadocCommentsTerse; + assert_eq!( + extract_doc_from_attrs(&JavadocCommentsTerse::schema().attrs), + (Some("Terse Javadoc summary".to_string()), None) + ); + /// Rustdoc summary /// Did other folks do this or what this an invention I can right- /// fully ascribe to Rust? @@ -1222,5 +1234,21 @@ mod tests { extract_doc_from_attrs(&JustTheSummary::schema().attrs), (Some("Just a summary".to_string()), None) ); + + /// Summary + /// Text + /// More + /// + /// Even + /// More + #[derive(Schema)] + struct SummaryDescriptionBreak; + assert_eq!( + extract_doc_from_attrs(&SummaryDescriptionBreak::schema().attrs), + ( + Some("Summary".to_string()), + Some("Text More\nEven More".to_string()) + ) + ); } }