impl "multipart/form-data" support by drahnr · Pull Request #418 · oxidecomputer/progenitor

drahnr · 2023-04-11T14:18:51Z

Currently form data handling is not implemented.

This PR has an initial, dirty implementation on which I'll iterate over the next weeks.

Closes #402

drahnr · 2023-04-12T07:44:32Z

@ahl if you have a few minutes to spare, it'd be great to get some feedback, in particular on:

is Generator the intended place for additional metadata?
is the boundary between parsing and generation sufficiently upheld in your opinion?
do you expect to panic on failed assumptions - currently the generation uses a filter iterator modifier and hence does not panic.

ahl

Thanks very much for submitting this. A great start. I left some scattered comments at varying levels of detail. The highest order bit comment I offer is to please add test cases--at a minimum modifying one of the openapi documents to include a body parameter of this type.

@ahl if you have a few minutes to spare, it'd be great to get some feedback, in particular on:

is Generator the intended place for additional metadata?

I mean... it can, but in the case of forms my inclination is to inline that code in the body method.

is the boundary between parsing and generation sufficiently upheld in your opinion?

I think so modulo the prior comment.

do you expect to panic on failed assumptions - currently the generation uses a filter iterator modifier and hence does not panic.

I mean... sure? It depends? I think the real answer is "yes" except when it would screw up currently working users in unexpected ways... which is hard to say.

ahl · 2023-04-19T22:16:17Z

 heck = "0.4.1"
 getopts = "0.2"
 indexmap = "1.9"
+itertools = "0.10"


is this used?

Woud you mind if I add a deny rule?

Addressed in 4323c11

ahl · 2023-04-19T22:19:47Z

                        let typ = self
                            .type_space
-                            .add_type_with_name(&schema, Some(name))?;
+                            .add_type_with_name(&schema, Some(dbg!(name)))?;


remove before merging?

I really thought I fixed that..

ahl · 2023-04-19T22:21:13Z

+        let url_renames =
+            HashMap::from_iter(method.params.iter().filter_map(|param| {
+                match &param.kind {
+                    OperationParameterKind::Path => {
+                        Some((&param.api_name, &param.name))
+                    }
+                    _ => None,
                }
-                _ => None,
-            })
-            .collect();
+            }));


Somewhat yes, collect elides what the elemets are being collected into which I personally find a lot easier to reason about code. Will remove.

ahl · 2023-04-19T22:29:53Z

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


Why IntoIterator?

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

ahl · 2023-04-19T22:33:36Z

+                                    .map(|prop_typ| (prop_name, prop_typ))
+                            })
+                        );
+                    let properties = syn::punctuated::Punctuated::<_, syn::Token![,]>::from_iter(


Suggested change

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

let properties =

#(#properties,)* below

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.

ahl · 2023-04-19T22:35:19Z

+                            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())))


Does this mean that all val have to be strings?

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

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

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.

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.

Gentle ping on this one.

ahl · 2023-04-19T22:36:20Z

+                ) => {
+                    Some(quote! {
+                    // This uses progenitor_client::RequestBuilderExt which sets up a simple form data based on bytes
+                    .form_from_raw(body.as_form())?


did you think about unrolled the as_form logic here? That seems potentially cleaner than an impl on the generated type.

I try to avoid writing to much code with quote if it can be formalized as a simple trait implementation without many constraints.

PlamenHristov · 2024-04-02T13:06:17Z

Hey team,

Do you think the above has the potential to be merged. If not, is there anything I can do to complete the PR so we can merge it ?

ahl · 2024-04-03T20:55:25Z

@PlamenHristov I think you'd be welcome to make a fork of this work, and resolve conflicts. As I recall this was pretty close. I don't see any tests and that would be another good addition to this work.

drahnr · 2024-04-04T05:52:48Z

Happy to pick this up again, the silence on my last set of comments and the scarcity of time made me question if it's worth continuing. If @ahl believes it's close to merging, I am happy to push it over the finish line. @PlamenHristov if you want to help, adding test cases is probably the most effective way

PlamenHristov · 2024-04-04T09:36:41Z

@drahnr/ @ahl thank you for both your responses.

@drahnr yeah could do that. Let me know once you believe the implementation is done.

ahl · 2024-04-04T18:13:43Z

Happy to pick this up again, the silence on my last set of comments and the scarcity of time made me question if it's worth continuing. If @ahl believes it's close to merging, I am happy to push it over the finish line. @PlamenHristov if you want to help, adding test cases is probably the most effective way

@drahnr my apologies for discouraging you. I appreciate the contribution and suffer from the same endemic time scarcity!

drahnr · 2024-04-11T12:14:32Z

@ahl *control is requiring auth. It's not needed unless there are more steps hiding behind it. The PR is still incomplete and requires some more testing.

ahl · 2024-04-24T17:21:01Z

+        let url_renames =
+            HashMap::from_iter(method.params.iter().filter_map(|param| {
+                match &param.kind {
+                    OperationParameterKind::Path => {
+                        Some((&param.api_name, &param.name))
+                    }
+                    _ => None,
                }
-                _ => None,
-            })
-            .collect();
+            }));


JMendyk

Very much looking forward to this PR being completed!

While testing the changes myself, I encountered two issues.

JMendyk

Another two observations found while testing the PR and fiddling with progenitor and typify.

drahnr · 2026-06-26T11:24:04Z

@ahl I ran into the same thing again, and if there is commitment to review and merge any further work, I'd be happy to update this (yes, after 2 years hiatus)

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

drahnr · 2026-06-30T09:09:40Z

Addressed all review comments
CC @ahl @JMendyk

first take at form data

af28d71

drahnr force-pushed the main branch 2 times, most recently from 00b610a to 7979fe2 Compare April 11, 2023 16:30

drahnr changed the title ~~first take at form data~~ impl "multipart/form-data" support Apr 12, 2023

refactor: somewhat cleaner approach

f67be29

drahnr force-pushed the main branch from 7979fe2 to f67be29 Compare April 12, 2023 07:41

ahl reviewed Apr 19, 2023

View reviewed changes

drahnr force-pushed the main branch from 2bae531 to f67be29 Compare May 10, 2023 15:35

drahnr mentioned this pull request May 10, 2023

a bunch of feature extensions - WIP #466

Draft

ahirner mentioned this pull request Oct 20, 2023

content multipart/form-data is not supported #518

Open

drahnr added 10 commits April 9, 2024 07:30

maintain ordering

8b6f4ef

clippy and warning silence

2ec5033

clippy

e2eda20

clippy

46a0bfb

add sevdesk snippet sample

cd8f72b

introduce expander for easier debugging

b347eaa

fmt

5acad25

use as_bytes to make it work

1c227b9

Merge main, resolve conflicts

2f63cc2

fix

41a421b

drahnr force-pushed the main branch from e7cb853 to 41a421b Compare April 10, 2024 09:45

drahnr added 2 commits April 11, 2024 18:26

review commeents

7eed031

deny rules

16ec6a0

drahnr added 4 commits April 12, 2024 09:24

lints

4323c11

fix

b57f58a

yikes

13b1e05

fmt

1707387

ahl reviewed Apr 24, 2024

View reviewed changes

drahnr mentioned this pull request Jun 7, 2024

add PROGENITOR_DEBUG=<PATH> environment variable to output macros to files #760

Open

JMendyk reviewed Aug 2, 2024

View reviewed changes

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

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

JMendyk reviewed Aug 11, 2024

View reviewed changes

Comment thread progenitor-client/src/progenitor_client.rs Outdated

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

spikespaz mentioned this pull request Oct 11, 2025

impl "multipart/form-data" support (rebased #418) #1215

Draft

4 tasks

lanej mentioned this pull request Nov 7, 2025

Support for multipart/related content type (RFC 2387) #1240

Open

drahnr mentioned this pull request Jan 5, 2026

enable streaming with multiple file types #1278

Closed

drahnr added 3 commits January 5, 2026 23:26

deps add multipart

4c1031b

type+form

59247b9

fmt

a090aa9

drahnr added 4 commits June 30, 2026 09:21

Merge remote-tracking branch 'origin/main' into once-again

e0d2db0

Cleanup

85e8358

backticks

adc1bf1

better

f9ec560

Copilot AI review requested due to automatic review settings June 30, 2026 08:27

Copilot started reviewing on behalf of drahnr June 30, 2026 08:27 View session

Copilot AI reviewed Jun 30, 2026

drahnr added 2 commits June 30, 2026 10:40

Merge remote-tracking branch 'upstream/main' into once-again

1a0cb63

fixup test

2cd2f95

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

Uh oh!

Conversation

drahnr commented Apr 11, 2023

Uh oh!

drahnr commented Apr 12, 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!

drahnr Apr 12, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

drahnr Apr 9, 2024 • 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!

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!

PlamenHristov commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahl commented Apr 3, 2024

Uh oh!

drahnr commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PlamenHristov commented Apr 4, 2024

Uh oh!

ahl commented Apr 4, 2024

Uh oh!

drahnr commented Apr 11, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JMendyk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JMendyk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

drahnr Apr 12, 2024 •

edited

Loading

drahnr Apr 9, 2024 •

edited

Loading

PlamenHristov commented Apr 2, 2024 •

edited

Loading

drahnr commented Apr 4, 2024 •

edited

Loading

JMendyk left a comment •

edited

Loading

drahnr commented Jun 30, 2026 •

edited

Loading