impl "multipart/form-data" support #418

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

progenitor-impl/Cargo.toml

@@ -11,6 +11,7 @@ readme = "../README.md"
 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

progenitor-impl/src/method.rs

@@ -328,7 +345,7 @@ impl Generator {

                        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

progenitor-impl/src/method.rs

+        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.

progenitor-impl/src/method.rs

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

progenitor-client/src/progenitor_client.rs

+    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

progenitor-impl/src/lib.rs

+                                    .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

progenitor-impl/src/lib.rs

+                            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

progenitor-impl/src/method.rs

+                ) => {
+                    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:18:36Z

Cargo.toml

+# [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" }


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

progenitor-impl/src/method.rs

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


ahl · 2024-04-24T17:22:57Z

progenitor-impl/src/method.rs

+                //     "format": "binary"
+                // }
+
+                let _mapped = match schema.item(components)? {


we're ignoring the value we create?

ahl · 2024-04-24T17:25:29Z

progenitor-macro/src/lib.rs

    };

+    println!("cargo::rerun-if-changed={}", path_str);


what's going on here?

JMendyk

Very much looking forward to this PR being completed!

While testing the changes myself, I encountered two issues.

JMendyk · 2024-08-02T23:03:16Z

progenitor-impl/src/lib.rs

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


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

JMendyk · 2024-08-02T23:10:47Z

progenitor-impl/src/method.rs

+                                            openapiv3::SchemaKind::Type(openapiv3::Type::String(
+                                                openapiv3::StringType {
+                                                    format:
+                                                        openapiv3::VariantOrUnknownOrEmpty::Item(
+                                                            openapiv3::StringFormat::Binary,


Are you planning to get rid of this limitation as part of this PR? My use-case requires support for booleans and strings with format date-time, uuid or no specified format.

I reviewed your code, but I don't think I'd be able to add support for them without some guidance 🤔

I think it's there be cause my specific use case only required binary.

Could you provide a minimal/trimmed down openapi spec where this'd fail?

Sure, here's a trimmed down spec I'm using: example.json

This is based on the OpenAPI spec for https://github.com/immich-app/immich

JMendyk

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

JMendyk · 2024-08-09T19:48:45Z

progenitor-client/src/progenitor_client.rs

+            .header(
+                reqwest::header::CONTENT_TYPE,
+                reqwest::header::HeaderValue::from_static(
+                    "multipart/form-data",
+                ),
+            )


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

JMendyk · 2024-08-10T19:27:58Z

progenitor-impl/src/lib.rs

+                    let properties = indexmap::IndexMap::<&'_ str, _>::from_iter(
+                        tstru
+                            .properties()
+                            .filter_map(|(prop_name, prop_id)| {


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 👍

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

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

JMendyk reviewed Aug 11, 2024

View reviewed changes

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

impl "multipart/form-data" support #418

Are you sure you want to change the base?

impl "multipart/form-data" support #418

Conversation

drahnr commented Apr 11, 2023

drahnr commented Apr 12, 2023

ahl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlamenHristov commented Apr 2, 2024 • edited Loading

ahl commented Apr 3, 2024

drahnr commented Apr 4, 2024 • edited Loading

PlamenHristov commented Apr 4, 2024

ahl commented Apr 4, 2024

drahnr commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMendyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Aug 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMendyk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

drahnr Aug 3, 2024 •

edited

Loading

JMendyk left a comment •

edited

Loading