correct handling of parameters (#32)

- improve parameter handling for names that are invalid rust identifiers
- fix broken logic for non-optional query parameters

progenitor-impl/Cargo.toml

@@ -25,3 +25,4 @@ unicode-xid = "0.2"
 
 [dev-dependencies]
 expectorate = "1.0"
+dropshot = { git = "https://github.com/oxidecomputer/dropshot" }

progenitor-impl/src/lib.rs

@@ -69,7 +69,10 @@ enum OperationParameterKind {
     Body,
 }
 struct OperationParameter {
+    /// Sanitize parameter name.
     name: String,
+    /// Original parameter name provided by the API.
+    api_name: String,
     typ: OperationParameterType,
     kind: OperationParameterKind,
 }
@@ -309,11 +312,13 @@ impl Generator {
                         // Path parameters MUST be required.
                         assert!(parameter_data.required);
 
-                        let nam = parameter_data.name.clone();
                         let schema = parameter_data.schema()?.to_schema();
 
                         let name = sanitize(
-                            &format!("{}-{}", operation_id, nam),
+                            &format!(
+                                "{}-{}",
+                                operation_id, &parameter_data.name
+                            ),
                             Case::Pascal,
                         );
                         let typ = self
@@ -322,6 +327,7 @@ impl Generator {
 
                         Ok(OperationParameter {
                             name: sanitize(&parameter_data.name, Case::Snake),
+                            api_name: parameter_data.name.clone(),
                             typ: OperationParameterType::Type(typ),
                             kind: OperationParameterKind::Path,
                         })
@@ -336,13 +342,12 @@ impl Generator {
                             todo!("allow empty value is a no go");
                         }
 
-                        let nam = parameter_data.name.clone();
                         let mut schema = parameter_data.schema()?.to_schema();
                         let name = sanitize(
                             &format!(
                                 "{}-{}",
                                 operation.operation_id.as_ref().unwrap(),
-                                nam
+                                &parameter_data.name,
                             ),
                             Case::Pascal,
                         );
@@ -355,9 +360,13 @@ impl Generator {
                             .type_space
                             .add_type_with_name(&schema, Some(name))?;
 
-                        query.push((nam, !parameter_data.required));
+                        query.push((
+                            parameter_data.name.clone(),
+                            !parameter_data.required,
+                        ));
                         Ok(OperationParameter {
                             name: sanitize(&parameter_data.name, Case::Snake),
+                            api_name: parameter_data.name.clone(),
                             typ: OperationParameterType::Type(typ),
                             kind: OperationParameterKind::Query(
                                 parameter_data.required,
@@ -398,6 +407,7 @@ impl Generator {
 
             raw_params.push(OperationParameter {
                 name: "body".to_string(),
+                api_name: "body".to_string(),
                 typ,
                 kind: OperationParameterKind::Body,
             });
@@ -407,12 +417,12 @@ impl Generator {
         raw_params.sort_by(
             |OperationParameter {
                  kind: a_kind,
-                 name: a_name,
+                 api_name: a_name,
                  ..
              },
              OperationParameter {
                  kind: b_kind,
-                 name: b_name,
+                 api_name: b_name,
                  ..
              }| {
                 match (a_kind, b_kind) {
@@ -421,10 +431,18 @@ impl Generator {
                         OperationParameterKind::Path,
                         OperationParameterKind::Path,
                     ) => {
-                        let a_index =
+                        let a_index = names
-                            names.iter().position(|x| x == a_name).unwrap();
+                            .iter()
-                        let b_index =
+                            .position(|x| x == a_name)
-                            names.iter().position(|x| x == b_name).unwrap();
+                            .unwrap_or_else(|| {
+                                panic!("{} missing from path", a_name)
+                            });
+                        let b_index = names
+                            .iter()
+                            .position(|x| x == b_name)
+                            .unwrap_or_else(|| {
+                                panic!("{} missing from path", b_name)
+                            });
                         a_index.cmp(&b_index)
                     }
                     (
@@ -604,13 +622,13 @@ impl Generator {
             .iter()
             .filter_map(|param| match &param.kind {
                 OperationParameterKind::Query(required) => {
-                    let qn = &param.name;
+                    let qn = &param.api_name;
+                    let qn_ident = format_ident!("{}", &param.name);
                     Some(if *required {
                         quote! {
-                            query.push((#qn, #qn.to_string()));
+                            query.push((#qn, #qn_ident .to_string()));
                         }
                     } else {
-                        let qn_ident = format_ident!("{}", qn);
                         quote! {
                             if let Some(v) = & #qn_ident {
                                 query.push((#qn, v.to_string()));
@@ -953,7 +971,7 @@ impl Generator {
             // without "page_token"
             let stream_params =
                 typed_params.iter().filter_map(|(param, stream)| {
-                    if param.name.as_str() == "page_token" {
+                    if param.api_name.as_str() == "page_token" {
                         None
                     } else {
                         Some(stream)
@@ -963,7 +981,7 @@ impl Generator {
             // The values passed to get the first page are the inputs to the
             // stream method with "None" for the page_token.
             let first_params = typed_params.iter().map(|(param, _)| {
-                if param.name.as_str() == "page_token" {
+                if param.api_name.as_str() == "page_token" {
                     // The page_token is None when getting the first page.
                     quote! { None }
                 } else {
@@ -977,7 +995,7 @@ impl Generator {
             // - None for all other query parameters
             // - The method inputs for non-query parameters
             let step_params = typed_params.iter().map(|(param, _)| {
-                if param.name.as_str() == "page_token" {
+                if param.api_name.as_str() == "page_token" {
                     quote! { state.as_deref() }
                 } else if let OperationParameterKind::Query(_) = param.kind {
                     // Query parameters are None; having page_token as Some(_)
@@ -1110,7 +1128,7 @@ impl Generator {
             .iter()
             .filter(|param| {
                 matches!(
-                    (param.name.as_str(), &param.kind),
+                    (param.api_name.as_str(), &param.kind),
                     ("page_token", OperationParameterKind::Query(_))
                         | ("limit", OperationParameterKind::Query(_))
                 )

progenitor-impl/tests/output/test_renamed_parameters.out

@@ -0,0 +1,75 @@
+pub use progenitor_client::{Error, ResponseValue};
+pub mod types {
+    use serde::{Deserialize, Serialize};
+    #[doc = "Error information from a response."]
+    #[derive(Serialize, Deserialize, Debug, Clone)]
+    pub struct Error {
+        #[serde(default, skip_serializing_if = "Option::is_none")]
+        pub error_code: Option<String>,
+        pub message: String,
+        pub request_id: String,
+    }
+}
+
+#[derive(Clone)]
+pub struct Client {
+    baseurl: String,
+    client: reqwest::Client,
+}
+
+impl Client {
+    pub fn new(baseurl: &str) -> Self {
+        let dur = std::time::Duration::from_secs(15);
+        let client = reqwest::ClientBuilder::new()
+            .connect_timeout(dur)
+            .timeout(dur)
+            .build()
+            .unwrap();
+        Self::new_with_client(baseurl, client)
+    }
+
+    pub fn new_with_client(baseurl: &str, client: reqwest::Client) -> Self {
+        Self {
+            baseurl: baseurl.to_string(),
+            client,
+        }
+    }
+
+    pub fn baseurl(&self) -> &String {
+        &self.baseurl
+    }
+
+    pub fn client(&self) -> &reqwest::Client {
+        &self.client
+    }
+
+    #[doc = "renamed_parameters: GET /{ref}/{type}/{trait}"]
+    pub async fn renamed_parameters<'a>(
+        &'a self,
+        ref_: &'a str,
+        type_: &'a str,
+        trait_: &'a str,
+        if_: &'a str,
+        in_: &'a str,
+        use_: &'a str,
+    ) -> Result<ResponseValue<()>, Error<types::Error>> {
+        let url = format ! ("{}/{}/{}/{}" , self . baseurl , progenitor_client :: encode_path (& ref . to_string ()) , progenitor_client :: encode_path (& type . to_string ()) , progenitor_client :: encode_path (& trait . to_string ()) ,);
+        let mut query = Vec::new();
+        query.push(("if", if_.to_string()));
+        query.push(("in", in_.to_string()));
+        query.push(("use", use_.to_string()));
+        let request = self.client.get(url).query(&query).build()?;
+        let result = self.client.execute(request).await;
+        let response = result?;
+        match response.status().as_u16() {
+            204u16 => Ok(ResponseValue::empty(response)),
+            400u16..=499u16 => Err(Error::ErrorResponse(
+                ResponseValue::from_response(response).await?,
+            )),
+            500u16..=599u16 => Err(Error::ErrorResponse(
+                ResponseValue::from_response(response).await?,
+            )),
+            _ => Err(Error::UnexpectedResponse(response)),
+        }
+    }
+}

progenitor-impl/tests/test_specific.rs

@@ -0,0 +1,71 @@
+// Copyright 2022 Oxide Computer Company
+
+use std::{str::from_utf8, sync::Arc};
+
+use dropshot::{
+    endpoint, ApiDescription, HttpError, HttpResponseUpdatedNoContent, Path,
+    Query, RequestContext,
+};
+use openapiv3::OpenAPI;
+use progenitor_impl::Generator;
+use schemars::JsonSchema;
+use serde::Deserialize;
+
+#[allow(dead_code)]
+#[derive(Deserialize, JsonSchema)]
+struct CursedPath {
+    #[serde(rename = "ref")]
+    reef: String,
+    #[serde(rename = "type")]
+    tripe: String,
+    #[serde(rename = "trait")]
+    trade: String,
+}
+
+#[allow(dead_code)]
+#[derive(Deserialize, JsonSchema)]
+struct CursedQuery {
+    #[serde(rename = "if")]
+    iffy: String,
+    #[serde(rename = "in")]
+    inn: String,
+    #[serde(rename = "use")]
+    youse: String,
+}
+
+#[endpoint {
+    method = GET,
+    path = "/{ref}/{type}/{trait}",
+}]
+async fn renamed_parameters(
+    _rqctx: Arc<RequestContext<Vec<String>>>,
+    _path: Path<CursedPath>,
+    _query: Query<CursedQuery>,
+) -> Result<HttpResponseUpdatedNoContent, HttpError> {
+    unimplemented!();
+}
+
+/// Test parameters that conflict with Rust reserved words and therefore must
+/// be renamed.
+#[test]
+fn test_renamed_parameters() {
+    let mut api = ApiDescription::new();
+    api.register(renamed_parameters).unwrap();
+
+    let mut out = Vec::new();
+
+    api.openapi("pagination-demo", "9000")
+        .write(&mut out)
+        .unwrap();
+
+    let out = from_utf8(&out).unwrap();
+
+    let spec = serde_json::from_str::<OpenAPI>(out).unwrap();
+
+    let mut generator = Generator::new();
+    let output = generator.generate_text(&spec).unwrap();
+    expectorate::assert_contents(
+        format!("tests/output/{}.out", "test_renamed_parameters"),
+        &output,
+    )
+}

progenitor/Cargo.toml

@@ -18,7 +18,7 @@ serde_json = "1.0"
 
 [dev-dependencies]
 chrono = { version = "0.4", features = ["serde"] }
-futures = "0.3"
+futures = "0.3.21"
 percent-encoding = "2.1"
 reqwest = { version = "0.11", features = ["json", "stream"] }
 schemars = "0.8"