Skip to content
Open
42 changes: 39 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ You'll need to add the following to `Cargo.toml`:
[dependencies]
+futures = "0.3"
+progenitor = { git = "https://github.com/oxidecomputer/progenitor" }
+reqwest = { version = "0.11", features = ["json", "stream"] }
+reqwest = { version = "0.11", features = ["json", "stream", "multipart"] }
+serde = { version = "1.0", features = ["derive"] }
```

Expand Down Expand Up @@ -123,7 +123,7 @@ You'll need to add the following to `Cargo.toml`:
[dependencies]
+futures = "0.3"
+progenitor-client = { git = "https://github.com/oxidecomputer/progenitor" }
+reqwest = { version = "0.11", features = ["json", "stream"] }
+reqwest = { version = "0.11", features = ["json", "stream", "multipart"] }
+serde = { version = "1.0", features = ["derive"] }

[build-dependencies]
Expand Down Expand Up @@ -180,7 +180,7 @@ bytes = "1.3.0"
chrono = { version = "0.4.23", default-features=false, features = ["serde"] }
futures-core = "0.3.25"
percent-encoding = "2.2.0"
reqwest = { version = "0.11.13", default-features=false, features = ["json", "stream"] }
reqwest = { version = "0.11.13", default-features=false, features = ["json", "stream", "multipart"] }
serde = { version = "1.0.152", features = ["derive"] }
serde_urlencoded = "0.7.1"

Expand Down
2 changes: 1 addition & 1 deletion example-build/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ edition = "2021"
[dependencies]
chrono = { version = "0.4", features = ["serde"] }
progenitor-client = { path = "../progenitor-client" }
reqwest = { version = "0.11.16", features = ["json", "stream"] }
reqwest = { version = "0.11.16", features = ["json", "stream", "multipart"] }
base64 = "0.21"
rand = "0.8"
serde = { version = "1.0", features = ["derive"] }
Expand Down
2 changes: 1 addition & 1 deletion example-macro/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ edition = "2021"
[dependencies]
chrono = { version = "0.4", features = ["serde"] }
progenitor = { path = "../progenitor" }
reqwest = { version = "0.11.16", features = ["json", "stream"] }
reqwest = { version = "0.11.16", features = ["json", "stream", "multipart"] }
schemars = { version = "0.8.12", features = ["uuid1"] }
serde = { version = "1.0", features = ["derive"] }
uuid = { version = "1.3", features = ["serde", "v4"] }
2 changes: 1 addition & 1 deletion progenitor-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ description = "An OpenAPI client generator - client support"
bytes = "1.4.0"
futures-core = "0.3.27"
percent-encoding = "2.2"
reqwest = { version = "0.11.16", default-features = false, features = ["json", "stream"] }
reqwest = { version = "0.11.16", default-features = false, features = ["json", "stream", "multipart"] }
serde = "1.0"
serde_json = "1.0"
serde_urlencoded = "0.7.1"
47 changes: 44 additions & 3 deletions progenitor-client/src/progenitor_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,11 +386,23 @@ pub fn encode_path(pc: &str) -> String {
}

#[doc(hidden)]
pub trait RequestBuilderExt<E> {
pub trait RequestBuilderExt<E>
where
Self: Sized,
{
fn form_urlencoded<T: Serialize + ?Sized>(
self,
body: &T,
) -> Result<RequestBuilder, Error<E>>;

fn form_from_raw<
S: AsRef<str>,
T: AsRef<[u8]>,
I: Sized + IntoIterator<Item = (S, T)>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why IntoIterator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a form can contain multiple streams. That's the least limiting for an Impl and sufficient for the R..BuilderExt trait. But with the comments below this might become obsolete

>(
self,
iter: I,
) -> Result<Self, Error<E>>;
}

impl<E> RequestBuilderExt<E> for RequestBuilder {
Expand All @@ -405,8 +417,37 @@ impl<E> RequestBuilderExt<E> for RequestBuilder {
"application/x-www-form-urlencoded",
),
)
.body(serde_urlencoded::to_string(body).map_err(|_| {
Error::InvalidRequest("failed to serialize body".to_string())
.body(serde_urlencoded::to_string(body).map_err(|e| {
Error::InvalidRequest(format!(
"failed to serialize body: {e:?}"
))
})?))
}

fn form_from_raw<
S: AsRef<str>,
T: AsRef<[u8]>,
I: Sized + IntoIterator<Item = (S, T)>,
>(
self,
mut iter: I,
) -> Result<Self, Error<E>> {
use reqwest::multipart::{Form, Part};

let mut form = Form::new();
for (name, value) in iter {
form = form.part(
name.as_ref().to_owned(),
Part::stream(Vec::from(value.as_ref())),
);
}
Ok(self
.header(
reqwest::header::CONTENT_TYPE,
reqwest::header::HeaderValue::from_static(
"multipart/form-data",
),
)
Comment on lines +480 to +485
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant, the .multipart call done below will add the header (including the boundary parameter).

.multipart(form))
}
}
1 change: 1 addition & 0 deletions progenitor-impl/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ readme = "../README.md"
heck = "0.4.1"
getopts = "0.2"
indexmap = "1.9"
itertools = "0.10"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woud you mind if I add a deny rule?

Copy link
Contributor Author

@drahnr drahnr Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 4323c11

openapiv3 = "1.0.0"
proc-macro2 = "1.0"
quote = "1.0"
Expand Down
61 changes: 52 additions & 9 deletions progenitor-impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use proc_macro2::TokenStream;
use quote::quote;
use serde::Deserialize;
use thiserror::Error;
use typify::{TypeDetails, TypeId};
use typify::{TypeSpace, TypeSpaceSettings};

use crate::to_schema::ToSchema;
Expand Down Expand Up @@ -40,6 +41,7 @@ pub type Result<T> = std::result::Result<T, Error>;

pub struct Generator {
type_space: TypeSpace,
forms: HashSet<TypeId>,
settings: GenerationSettings,
uses_futures: bool,
uses_websockets: bool,
Expand Down Expand Up @@ -163,6 +165,7 @@ impl Default for Generator {
type_space: TypeSpace::new(
TypeSpaceSettings::default().with_type_mod("types"),
),
forms: Default::default(),
settings: Default::default(),
uses_futures: Default::default(),
uses_websockets: Default::default(),
Expand Down Expand Up @@ -204,6 +207,7 @@ impl Generator {
Self {
type_space: TypeSpace::new(&type_settings),
settings: settings.clone(),
forms: Default::default(),
uses_futures: false,
uses_websockets: false,
}
Expand Down Expand Up @@ -262,6 +266,43 @@ impl Generator {

let types = self.type_space.to_stream();

let extra_impl = TokenStream::from_iter(
self.forms
.iter()
.map(|type_id| {
let typ = self.get_type_space().get_type(type_id).unwrap();
let form_name = typ.name();
let td = typ.details();
let TypeDetails::Struct(tstru) = td else { unreachable!() };
let properties = indexmap::IndexMap::<&'_ str, _>::from_iter(
tstru
.properties()
.filter_map(|(prop_name, prop_id)| {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prop_name seems not to be the right choice here 🤔 It's the name of the property from the Rust struct and not the name of the property from the OpenAPI spec.

In my case, the OpenAPI spec uses lowerCamelCase for properties, whereas Rust uses snake_case and the generated code uses the latter which makes the request invalid.

I went through progenitor and typify's source code and I believe that despite the original-cased information being available in StructProperty, it's not then exposed via TypeStructPropInfo. I made a fork that changes that (diff) and integrated into your PR (diff). Now generated as_form has proper casing 👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the best approach, possibly one of the maintainers could suggest a better one.

self.get_type_space()
.get_type(&prop_id).ok()
.map(|prop_typ| (prop_name, prop_typ))
})
);
let properties = syn::punctuated::Punctuated::<_, syn::Token![,]>::from_iter(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let properties = syn::punctuated::Punctuated::<_, syn::Token![,]>::from_iter(
let properties =

#(#properties,)* below

Copy link
Contributor Author

@drahnr drahnr Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above creates a punctuated list, which can both be used with quote to create the correct expansion, while also being iterable, can change to Vec<TokenStream> and use quote!{ #(#propeties),* if desired.

properties
.into_iter()
.map(|(prop_name, prop_ty)| {
let ident = quote::format_ident!("{}", prop_name);
quote!{ (#prop_name, &self. #ident) }
}));

let form_name = quote::format_ident!("{}",typ.name());

quote! {
impl #form_name {
pub fn as_form<'f>(&'f self) -> impl std::iter::Iterator<Item=(&'static str, &'f [u8])> {
[#properties]
.into_iter()
.filter_map(|(name, val)| val.as_ref().map(|val| (name, val.as_slice())))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that all val have to be strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not at all. It means all values have to be convertible to a stream of bytes. The representation is limiting and should be improved, i.e. it only allows for basic usage key but not for keys derived from objects where the keys end up as foo[bar].sub

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Are there types we might encounter here other than String and Vec[u8] that satisfy this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any type really, such that we could call as_form for nested types. That was the direction I wanted to go into. Hence avoiding the direct expansion. There are a few pitfalls, i.e. metadata desc such as mime type and filename that are commonly expected but not defined in the openapi spec. I am not yet sure how to handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the PR here is in a state of rough draft/idea/make my immediate needs meet, so I am open to changing direction as you see fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gentle ping on this one.

}
}
}
}));
// Generate an implementation of a `Self::as_inner` method, if an inner
// type is defined.
let maybe_inner = self.settings.inner_type.as_ref().map(|inner| {
Expand Down Expand Up @@ -290,20 +331,20 @@ impl Generator {
});

let client_docstring = {
let mut s = format!("Client for {}", spec.info.title);
let mut doc = format!("Client for {}", spec.info.title);

if let Some(ss) = &spec.info.description {
s.push_str("\n\n");
s.push_str(ss);
if let Some(desc) = &spec.info.description {
doc.push_str("\n\n");
doc.push_str(desc);
}
if let Some(ss) = &spec.info.terms_of_service {
s.push_str("\n\n");
s.push_str(ss);
if let Some(tos) = &spec.info.terms_of_service {
doc.push_str("\n\n");
doc.push_str(tos);
}

s.push_str(&format!("\n\nVersion: {}", &spec.info.version));
doc.push_str(&format!("\n\nVersion: {}", &spec.info.version));

s
doc
};

let version_str = &spec.info.version;
Expand All @@ -325,6 +366,8 @@ impl Generator {
use std::convert::TryFrom;

#types

#extra_impl
}

#[derive(Clone, Debug)]
Expand Down
Loading