Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

impl "multipart/form-data" support #418

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
955 changes: 454 additions & 501 deletions Cargo.lock

Large diffs are not rendered by default.

8 changes: 4 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ resolver = "2"
#[patch."https://github.com/oxidecomputer/dropshot"]
#dropshot = { path = "../dropshot/dropshot" }

#[patch."https://github.com/oxidecomputer/typify"]
#typify = { path = "../typify/typify" }
# [patch."https://github.com/oxidecomputer/typify"]
# typify = { path = "../typify/typify" }

#[patch.crates-io]
# [patch.crates-io]
#serde_tokenstream = { path = "../serde_tokenstream" }
#typify = { path = "../typify/typify" }
# typify = { path = "../typify/typify" }
Comment on lines +18 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert?

#rustfmt-wrapper = { path = "../rustfmt-wrapper" }
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,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 @@ -133,7 +133,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 @@ -199,7 +199,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
11 changes: 10 additions & 1 deletion cargo-progenitor/src/main.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
// Copyright 2023 Oxide Computer Company

//! The cargo tool to generate a rust representation of an OpenAPI spec.

#![deny(missing_docs)]
#![deny(unused_crate_dependencies)]
#![deny(unused_imports)]
#![deny(unused_import_braces)]

use std::{
collections::HashMap,
fs::{File, OpenOptions},
Expand All @@ -13,6 +20,7 @@ use openapiv3::OpenAPI;
use progenitor::{GenerationSettings, Generator, InterfaceStyle, TagStyle};
use progenitor_impl::space_out_items;

/// Include the artifact created by `build.rs`.
pub mod built_info {
// The file has been placed there by the build script.
include!(concat!(env!("OUT_DIR"), "/built.rs"));
Expand Down Expand Up @@ -225,6 +233,7 @@ fn main() -> Result<()> {
Ok(())
}

/// List of dependencies required when including the generated code.
pub fn dependencies(builder: Generator, include_client: bool) -> Vec<String> {
let dependency_versions: HashMap<String, String> = built_info::DEPENDENCIES
.iter()
Expand All @@ -234,7 +243,7 @@ pub fn dependencies(builder: Generator, include_client: bool) -> Vec<String> {
let mut deps = vec![
format!("bytes = \"{}\"", dependency_versions.get("bytes").unwrap()),
format!("futures-core = \"{}\"", dependency_versions.get("futures-core").unwrap()),
format!("reqwest = {{ version = \"{}\", default-features=false, features = [\"json\", \"stream\"] }}", dependency_versions.get("reqwest").unwrap()),
format!("reqwest = {{ version = \"{}\", default-features=false, features = [\"json\", \"stream\", \"multipart\"] }}", dependency_versions.get("reqwest").unwrap()),
format!("serde = {{ version = \"{}\", features = [\"derive\"] }}", dependency_versions.get("serde").unwrap()),
format!("serde_urlencoded = \"{}\"", dependency_versions.get("serde_urlencoded").unwrap()),
];
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.27", features = ["json", "stream"] }
reqwest = { version = "0.11.27", features = ["json", "stream", "multipart"] }
base64 = "0.22"
rand = "0.8"
serde = { version = "1.0", features = ["derive"] }
Expand Down
5 changes: 3 additions & 2 deletions example-macro/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ authors = ["Adam H. Leventhal <[email protected]>"]
edition = "2021"

[dependencies]

serde = { version = "1.0", features = ["derive"] }
chrono = { version = "0.4", features = ["serde"] }
progenitor = { path = "../progenitor" }
reqwest = { version = "0.11.27", features = ["json", "stream"] }
reqwest = { version = "0.11.27", features = ["json", "stream", "multipart"] }
schemars = { version = "0.8.16", features = ["uuid1"] }
serde = { version = "1.0", features = ["derive"] }
uuid = { version = "1.8", 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.6.0"
futures-core = "0.3.30"
percent-encoding = "2.3"
reqwest = { version = "0.11.27", default-features = false, features = ["json", "stream"] }
reqwest = { version = "0.11.27", default-features = false, features = ["json", "stream", "multipart"] }
serde = "1.0"
serde_json = "1.0"
serde_urlencoded = "0.7.1"
3 changes: 3 additions & 0 deletions progenitor-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
//! Support for generated clients.

#![deny(missing_docs)]
#![deny(unused_crate_dependencies)]
#![deny(unused_imports)]
#![deny(unused_import_braces)]

mod progenitor_client;

Expand Down
49 changes: 45 additions & 4 deletions progenitor-client/src/progenitor_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::ops::{Deref, DerefMut};

use bytes::Bytes;
use futures_core::Stream;
use reqwest::RequestBuilder;
use reqwest::{multipart::Part, RequestBuilder};
use serde::{de::DeserializeOwned, Serialize};

#[cfg(not(target_arch = "wasm32"))]
Expand Down Expand Up @@ -421,11 +421,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 @@ -440,8 +452,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,
iter: I,
) -> Result<Self, Error<E>> {
use reqwest::multipart::Form;

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: 0 additions & 1 deletion progenitor-impl/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ readme = "../README.md"
[dependencies]
heck = "0.5.0"
http = "0.2.9"
getopts = "0.2"
indexmap = "2.2.6"
openapiv3 = "2.0.0"
proc-macro2 = "1.0"
Expand Down
3 changes: 2 additions & 1 deletion progenitor-impl/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,8 @@ impl Generator {
// are currently...
OperationParameterType::RawBody => None,

OperationParameterType::Type(body_type_id) => {
OperationParameterType::Type(body_type_id)
| OperationParameterType::Form(body_type_id) => {
Some(body_type_id)
}
});
Expand Down
7 changes: 4 additions & 3 deletions progenitor-impl/src/httpmock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ impl Generator {
name, typ, kind, ..
}| {
let arg_type_name = match typ {
OperationParameterType::Type(arg_type_id) => self
OperationParameterType::Type(arg_type_id)
| OperationParameterType::Form(arg_type_id) => self
.type_space
.get_type(arg_type_id)
.unwrap()
Expand Down Expand Up @@ -245,9 +246,9 @@ impl Generator {
OperationParameterKind::Header(_) => quote! { todo!() },
OperationParameterKind::Body(body_content_type) => {
match typ {
OperationParameterType::Type(_) => quote! {
OperationParameterType::Type(_)
| OperationParameterType::Form(_) => quote! {
Self(self.0.json_body_obj(value))

},
OperationParameterType::RawBody => {
match body_content_type {
Expand Down
64 changes: 55 additions & 9 deletions progenitor-impl/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@
//! Core implementation for the progenitor OpenAPI client generator.

#![deny(missing_docs)]
#![cfg_attr(not(test), deny(unused_crate_dependencies))]
#![deny(unused_import_braces)]
#![deny(unused_imports)]

use std::collections::{BTreeMap, HashMap, HashSet};

use indexmap::IndexSet;
use openapiv3::OpenAPI;
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 @@ -48,6 +53,7 @@ pub type Result<T> = std::result::Result<T, Error>;
/// OpenAPI generator.
pub struct Generator {
type_space: TypeSpace,
forms: IndexSet<TypeId>,
settings: GenerationSettings,
uses_futures: bool,
uses_websockets: bool,
Expand Down Expand Up @@ -198,6 +204,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 @@ -240,6 +247,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 @@ -304,6 +312,42 @@ 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 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_bytes())))
Copy link

Choose a reason for hiding this comment

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

When trying to compile I'm getting the following error (rust v1.78.0):

     --> <redacted>/src/lib.rs:12324:47
      |
12324 |                 .filter_map(|(name, val)| val.as_ref().map(|val| (name, val.as_bytes())))
      |                                               ^^^^^^   --- type must be known at this point
      |
help: try using a fully qualified path to specify the expected types
      |
12324 |                 .filter_map(|(name, val)| <std::string::String as AsRef<T>>::as_ref(val).map(|val| (name, val.as_bytes())))
      |                                           ++++++++++++++++++++++++++++++++++++++++++   ~

The specification I'm using includes a single property.

Copy link
Contributor Author

@drahnr drahnr Aug 3, 2024

Choose a reason for hiding this comment

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

I ran into this limitation a few weeks back, and the root cause here being no unified way between &str, Vec<u8>, and others to retrieve a byte slice. Adding a custom trait and implementing fixes that.

That said, It'll be September before I'll be able to work on this PR

}
}
}
}));
// 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 @@ -332,20 +376,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 Down Expand Up @@ -373,6 +417,8 @@ impl Generator {
use std::convert::TryFrom;

#types

#extra_impl
}

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