Support multipart/form-data and file uploads by ahirner · Pull Request #609 · oxidecomputer/progenitor

ahirner · 2023-10-22T12:30:52Z

Fixes #518 and #402.

This approach factors out binary form data as reqwest::multipart::Part parameter. This has the following advantages:

Clients gains the ability to stream large files
Bodies remain serde::Serialize which they couldn't if a Part was made from an async stream

The disadvantage is leaking a reqwest type. However, we could probably wrap and imple Into for an internal type if required.

An internal media heavy API compiled well. I'm looking for guidance and tips. Todos:

decide and finalize how to typify binary formatted strings
some work on cli
some work on httpmock
tests

* own OperationParamaterType * delare replaced PartMeta

Todo: * some work on cli * some work on httpmock * generate form in method body

except: create form from body

This reverts commit 9d8a789.

adds: use reqwest::multipart::Part;

adds super::Part import

approach has two flaws: * form params optional cannot be recovered from typify or ergonomically from schema * fails for bodies with nested compound types, e.g. Map, Vec

the main benefit is correct treatment of optionals and a baseline support for nested compound parameters

ahl · 2023-10-23T21:31:43Z

I didn't take a close look at this as it appears to still be a work in progress, but I did want to say:

Thank you! great to see folks participating in this project
I've found that one of the best ways to demonstrate new features such as this one is to add an end-to-end test by adding an operation in an existing OpenAPI json file or by adding a new file; this will make the output clear in the updated fixture data.

…m_data

curl -L -o sample_openapi/openai-openapi.yaml https://raw.githubusercontent.com/openai/openai-openapi/84e9aa615dcb81c66d2d63b8d5dad025259223b2/openapi.yaml

* removed endpoints not tagged with Images * removed x-oaiMeta key * inlined end_user_param_configuration still gen fail: TypeError(InvalidValue)

…o feature/form_data

reference types are emitted in modified and and modified versions though

ahirner · 2023-10-28T23:31:02Z

I've found that one of the best ways to demonstrate new features such as this one is to add an end-to-end test

Check out progenitor-impl/tests/output/openai-openapi-*.out. This API features multiple files in one operation and optionals as well.

For an integrated example see: https://github.com/ahirner/openai-progenitor

decide and finalize how to typify

The outputs are fully functional AFAIC. My last gripe is that multipart/form-data body structs are emitted twice. Once with file fields factored out and once from the original ref. My idea would be to inline those definitions before adding them:

progenitor/progenitor-impl/src/lib.rs

Line 223 in a6bc062

self.type_space.add_ref_types(schemas)?;

LMK what you think!
Btw, I think there is also the opportunity to DRY the token generating methods.

, thus no additional todo here

ReferenceOrExt already does everything we need, amazing

.. because a cloned and pruned body type is created separately. We probalby want to get rid of redundant body structs iff. they are referenced in multipart/form-data contents only

…y referenced in such requests

ahirner · 2023-10-30T22:49:54Z

PR is complete and all comments so far adressed.

ahl · 2023-11-13T16:49:32Z

PR is complete and all comments so far adressed.

Sounds good. I'll try to take a first pass this week.

ahirner · 2023-12-11T22:32:56Z

Hi @ahl, is there maybe something I can do in the meantime?

ahl

This is not exhaustive, but what I could get through. I'd encourage you to generalize from these comments and go through the PR yourself again. Thank you for the submission and my apologies for the delay reviewing it.

ahl · 2023-12-13T01:02:30Z

+Note that keeper, buildomat and openai-openapi.yaml have diverged in order to validate support for various features.
+The others should not be changed other than updating them from their source repository.


ahl · 2023-12-13T01:09:22Z

+    /// Binary or text parts of a multipart/form-data body.
+    /// The bool signifies if it is required.
+    // TODO any other body content ought to become optional,
+    // after which flag would be redundant
+    // (see comment in OperationParameterKind)
+    FormData(bool),


Suggested change

/// Binary or text parts of a multipart/form-data body.

/// The bool signifies if it is required.

// TODO any other body content ought to become optional,

// after which flag would be redundant

// (see comment in OperationParameterKind)

FormData(bool),

/// Binary or text parts of a multipart/form-data body.

FormData,

It doesn't appear that this is needed for the test cases provided and if we're going to support optional bodies, let's do it properly in OpationalParameterKind

ahl · 2023-12-13T01:12:39Z

+                    if let OperationParameterKind::Body(
+                        BodyContentType::FormData(required),
+                    ) = param.kind


maybe if let .. else { unreachable!() };

ahl · 2023-12-13T01:54:05Z


-    pub fn generate_tokens(&mut self, spec: &OpenAPI) -> Result<TokenStream> {
+    fn add_ref_types(&mut self, spec: &OpenAPI) -> Result<()> {
        validate_openapi(spec)?;


I don't think this has to do with what I infer to be the purpose of this function.

ahl · 2023-12-13T02:01:13Z

+        // Take unquoted String Value
+        Some(v) => Ok(v.to_string()),
+        // otherwise convert to quoted json
+        None => serde_json::to_string(value).map_err(|_| {


doesn't this make serde_json into a mandatory dependency for generated crates?

ahl · 2023-12-13T02:18:31Z

        let version_str = &spec.info.version;
+        let (to_form_string, pub_part) = if self.uses_form_parts {
+            (
+                Some(quote! { ,to_form_string }),


use it unconditionally (note #[allow(unused_imports)])

ahl · 2023-12-13T02:22:18Z

+        // collect all component names in request body media references
+        // that are only used for multipart/form-data operations.
+        let media_conents = spec
+            .paths
+            .iter()
+            .flat_map(|(_path, ref_or_item)| {
+                // Exclude externally defined path items.
+                let item = ref_or_item.as_item().unwrap();
+                item.iter().map(move |(_method, operation)| operation)
+            })
+            .flat_map(|operation| operation.request_body.iter())
+            .filter_map(|b| b.as_item().map(|b| &b.content))
+            .flat_map(|body_content| body_content.iter());


we're trying to exclude these so we don't generate types that aren't used?

ahl · 2023-12-13T02:22:24Z


+        // collect all component names in request body media references
+        // that are only used for multipart/form-data operations.
+        let media_conents = spec


ahl · 2023-12-13T02:22:54Z

+            .iter()
+            .flat_map(|(_path, ref_or_item)| {
+                // Exclude externally defined path items.
+                let item = ref_or_item.as_item().unwrap();


why is this valid?

ahl · 2023-12-13T02:25:58Z

+/// Return all component names where schema references are keyed
+/// by multipart/form-data but nowhere else. The last part of a media
+/// schema's json path reference is considered the component name.
+fn get_exclusive_formdata_refs<'a>(


these checks seem insufficient to validate if referenced types are used elsewhere e.g. what if the type is used as a body parameter or response parameter. I may be misunderstanding what's going on here, but I don't think we should do any of this.

what if the type is used as a body parameter or response parameter

That's what this function should find out. Alas, it's only partially validated in tests, not provably.

what's going on here

It's to avoid that multipart/form-data body structs are emitted twice. Once with file fields factored out and once from the original ref.

should do any of this

I think so too, but haven't found a cooler way. My initial idea was way to inline types earlier, but I think the current typify API didn't let me. Maybe I'll find a better approach still.

I would suggest we just not do anything clever here: generating orphaned types is fine--we already do it!

Ok... good to know! Then this PR might come to a conclusion.
It's only that without f8baafa, the number of orphaned types was very, very large for our internal uses. This simply went against my sense of aesthetics. Technically, I don't think it added compile times really.

there's probably a bigger, more holistic look one might take: start from all operations, see the types they touch. We could have special handling for these multi-part types (i.e. use in those cases wouldn't count as real use)

Yes. OTOH, the more I thought about that, the larger the PR grew in my mind as well ;)
Intuitively, every operation could probably inferred from a generalized query into a graph of schema units.

ahirner · 2024-03-10T18:55:47Z

Obviously this PR stalled due to my lack of effort.
Overall I suggest to revisit it after #384. #177 would make the generated code leaner also.

ahirner added 15 commits October 19, 2023 07:57

un-require unstable cargo fmt for now

9d8a789

enable reqwest's multipart

95850e3

intro BodyContentType::FormData WIP

92a32a2

impl files as Into<Body> WIP

2ae53f7

* own OperationParamaterType * delare replaced PartMeta

switch binary string repr to some unit for now

5bc61b0

introduce (optional) FormPart WIP

31daaed

Todo: * some work on cli * some work on httpmock * generate form in method body

cut and dry

7ce4a0f

generate form in method body WIP

4298470

except: create form from body

Revert "un-require unstable cargo fmt for now"

0f71c90

This reverts commit 9d8a789.

EXPECTORATE=overwrite cargo test

e89338c

adds: use reqwest::multipart::Part;

fix Part body builder impls

d614dff

EXPECTORATE=overwrite cargo test

45f099a

adds super::Part import

cleanup

348a30f

set params from form WIP

4c2d2b9

approach has two flaws: * form params optional cannot be recovered from typify or ergonomically from schema * fails for bodies with nested compound types, e.g. Map, Vec

serialize form data body as serde strings

2a806f5

the main benefit is correct treatment of optionals and a baseline support for nested compound parameters

ahirner marked this pull request as ready for review October 22, 2023 20:21

ahirner mentioned this pull request Oct 22, 2023

content multipart/form-data is not supported #518

Open

ahirner and others added 10 commits October 26, 2023 06:26

Merge branch 'oxidecomputer:main' into feature/form_data

c8a9f31

Merge branch 'main' of github.com:ahirner/progenitor into feature/for…

38d34d2

…m_data

add openapi-api sample schema

692b02a

curl -L -o sample_openapi/openai-openapi.yaml https://raw.githubusercontent.com/openai/openai-openapi/84e9aa615dcb81c66d2d63b8d5dad025259223b2/openapi.yaml

prune and describe openai-openapi.yaml

89b16ea

* removed endpoints not tagged with Images * removed x-oaiMeta key * inlined end_user_param_configuration still gen fail: TypeError(InvalidValue)

uses_serde_json also depends on multipart/form-data bodies

6489729

no need to filter body_parts for existing types

90f353d

explicit type requirement err

6474706

verify_apis("openai-openapi.yaml") and EXPECTORATE=overwrite cargo test

c3ffd79

Merge branch 'feature/form_data' of github.com:ahirner/progenitor int…

2521259

…o feature/form_data

don't require (cli) form part WIP

aea0b0a

reference types are emitted in modified and and modified versions though

ahirner marked this pull request as draft October 27, 2023 00:37

fix quoting form data string values

191105f

ahirner added 5 commits October 28, 2023 12:19

fix to_form_string builder import

ba30bb1

factor out uses_request_file_part

77ff7d2

let cli consumers override file request part

716193f

httpmock form data to a reasonable extent

35ed72e

blanket form data usage flag to remove unneeded .out diffs

0ab9911

ahirner marked this pull request as ready for review October 28, 2023 23:17

jayvdb reviewed Oct 28, 2023

View reviewed changes

ahirner added 5 commits October 29, 2023 12:56

cargo clippy on new code

91bea0b

clarify use_form_parts comment

fa31f6e

move unreachable variant of method param last

2c9cbdb

document new OperationParameter variants

829199c

docstring for (still hidden) to_form_string

5206425

jayvdb reviewed Oct 29, 2023

View reviewed changes

Comment thread progenitor-impl/src/lib.rs Outdated

ahirner added 5 commits October 30, 2023 17:56

TypeId based FormData bodies for text fields are still required

b4e916d

, thus no additional todo here

nullable binary strings in form-data hardly make sense

106205f

no need for own dereferencing of schema objects

2a8e24a

ReferenceOrExt already does everything we need, amazing

There is no need to treat binary strings at the type id level anymore

9e204ed

.. because a cloned and pruned body type is created separately. We probalby want to get rid of redundant body structs iff. they are referenced in multipart/form-data contents only

remove multipart/form-data structs from output if they are exclusivel…

f8baafa

…y referenced in such requests

ahl requested changes Dec 13, 2023

View reviewed changes

ahirner marked this pull request as draft December 13, 2023 18:59

ahirner closed this Mar 10, 2024

drahnr mentioned this pull request Jan 5, 2026

enable streaming with multiple file types #1278

Open

		Note that keeper, buildomat and openai-openapi.yaml have diverged in order to validate support for various features.
		The others should not be changed other than updating them from their source repository.

Conversation

ahirner commented Oct 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahl commented Oct 23, 2023

Uh oh!

ahirner commented Oct 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahirner commented Oct 30, 2023

Uh oh!

ahl commented Nov 13, 2023

Uh oh!

ahirner commented Dec 11, 2023

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahirner Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahirner commented Mar 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahirner commented Oct 22, 2023 •

edited

Loading

ahirner Dec 13, 2023 •

edited

Loading