-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add multipart response parsing #472
base: main
Are you sure you want to change the base?
Add multipart response parsing #472
Conversation
As expected it's not very hard to add the multipart parsing itself, but it's a bit trickier to ensure the types make sense. I need to experiment with the FormData API a bit to see how we can handle different content types for individual parts. |
src/runtime/index.ts
Outdated
let data: any = {}; | ||
try { | ||
(await res.formData()).forEach((val, key) => { | ||
data[key] = val; | ||
}); | ||
} catch (err) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Braindump:
For a similar effect you can also use Object.fromEntries(await res.formData())
.
In some scenarios both of these approaches are tricky because FormData can hold multiple values for a single key (See getAll and that can not be easily mapped to an object.
Since val
is always string
or Blob
we'd need to check the OAS spec for how complex types are expected to be mapped onto form-data right?
Don't want to interfere with your train of thought, let me know when there's anything I might help with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Good catch.
The spec mentions how we should deal with complex types. It specifically tells us they should be considered application/json
by default (including arrays of complex types).
It also tells us that arrays of primitive values should be encoded as text/plain
. Presumably they are encoded in the message by repeating the same key (that's at least how it's described for files), and I guess the FormData
API supports this with the way getAll
is implemented. I'll see if I can find a better reference for this.
IIRC FormData.entries()
only picks the first value even if there's multiple with the same key, so that could prove to be a problem using that API. I need to validate this though; it's possible that it actually returns multiple entries, one for each value, but it's converting the entries to an object that only preserves one.
I suppose we'll also want to parse JSON to objects, and not just return the JSON string representation which converting the FormData
to an object will do. For that we'll need the encoding type, howevever, which I'm not sure if we have access to after parsing the response as form-data.
Worst case we'll need to implement a custom parser, but it would ofc be ideal if we could avoid doing that. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these all valid concerns & considerations.
FormData.entries()
is aware of multiple entries with the same key. The problem comes when trying to map that directly to an Object
.
Regarding the application/json would that mean that complex types are JSON encoded and then form-data encoded?
So basically
const fd = new FormData();
fd.append('complex', JSON.stringify({ one: 2 }));
When that's the case then we'll need to pass some additional type meta-information to the runtime so that it knows which keys to JSON.parse and which not. Not sure if the additional complexity needed is worth it or if we'd rather limit the capabilities of the lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the FormData
API for this is a bit awkward, but you could do something like:
const fd = new FormData();
fd.append('key1', 'val1');
fd.append('key2', new Blob([JSON.stringify({ foobar: 123 })], { type: 'application/json' }));
const res = new Response(fd);
console.log(await res.text());
outputs the response body
-----------------------------152258955627515488381513141310
Content-Disposition: form-data; name="key1"
val1
-----------------------------152258955627515488381513141310
Content-Disposition: form-data; name="key2"; filename="blob"
Content-Type: application/json
{"foobar":123}
-----------------------------152258955627515488381513141310--
(Ignore the filename
field, it's not meaningful and only a result of the FormData API)
So the idea is to use Response.formData()
to parse the response, iterate over the fields, check if the value is a blob - and if it is - parse it as json only if Content-Type
is set to application/json
for that specific part. Otherwise we leave it as a blob. If the value is not a blob, we can assume it is a string (text/plain
) and treat it as such.
I've come up with the following to parse a multipart response so far:
(await res.formData()).forEach(async (val, key) => {
// If the value is JSON encoded it is parsed. Other complex types are left as-is.
val =
val instanceof Blob && val.type === "application/json"
? JSON.parse(await val.text())
: val;
if (key in data) {
// If a key is repeated, we join the corresponding values into an array.
if (Array.isArray(data[key])) {
// If an array has already been created, push the new value onto it.
data[key].push(val);
} else {
// Otherwise create a new array containing both values.
data[key] = [data[key], val];
}
} else {
data[key] = val;
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really interesting! I'm learning a lot here, thank you!
This seems like a reasonable approach I'd say 👍
Sorry about the delay on this, I'll do my best to finish the PR coming week! |
No worries! Take your time 👍 |
* test: add complex file upload * test: remove FromData polyfill node has build-in FormData (https://nodejs.org/dist/latest-v20.x/docs/api/globals.html\#class-formdata) * feat: support complex multipart requests - complex data structures - multiple files under same name fix #233 ref #472
Added a basic implementation with passing unit tests is done. I'm having some issues with the FormData API; particularly I'm finding inconsistencies when it comes to field parsing. While the unit tests work, parsing parts with |
Description
Adds support for parsing
multipart/form-data
responses.Related issues
multipart/form-data
responses #471