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

Handle body types other than string and plain object #269

Closed
1 task done
wug-ge opened this issue Aug 20, 2023 · 5 comments · Fixed by #273
Closed
1 task done

Handle body types other than string and plain object #269

wug-ge opened this issue Aug 20, 2023 · 5 comments · Fixed by #273
Labels
bug Something isn't working enhancement New feature or request

Comments

@wug-ge
Copy link

wug-ge commented Aug 20, 2023

Describe the feature

Hi,

I've been struggling with routeRules in Nuxt.js along with many other users, see the following issues:

unjs/h3#510
nuxt/nuxt#19188

After a 4h debugging session I found that the root of the problem is ofetch itself.
Let me explain further:

If you create a Nuxt.js routeRule it uses h3's proxyRequest with ofetch behind the curtain.
POST body data (or any other body data) will be converted to a Buffer by h3 internally on this specific line:
https://github.com/unjs/h3/blob/main/src/utils/proxy.ts#L46

This is passed onto ofetch in Nuxt.js then which results in the body data being JSON.stringify'd on this specific line:
https://github.com/unjs/ofetch/blob/main/src/fetch.ts#L169
Given it is a Buffer (object) not a string, which causes an invalid request to be sent.

Example:
Body data = "test"

Body data being send (cause of a JSON stringify that shouldn't happen):
{"type":"Buffer","data":[116,101,115,116]}

This causes the request to fail with a content-length mismatch. (the content-length is given by the proxy before and not valid anymore)

H3 in it's tests however uses node-native-fetch where the issue doesn't seem to occur:
https://github.com/unjs/h3/blob/main/test/proxy.test.ts

Given Buffer's seem to be handled just fine in my tests in native implementations of fetch, I'm suggesting a fix like the following:
4bbcbb2

I'm not familiar with ofetch, please let me know if you see any issues with my proposed fix, otherwise I'd be happy to create a pull request.

Thanks!

Additional information

  • Would you be willing to help implement this feature?
@zefman
Copy link

zefman commented Aug 22, 2023

Can confirm we are hitting this exact problem too, requests that should have a json body end up being sent as a buffer to the proxied endpoint.

@wug-ge
Copy link
Author

wug-ge commented Aug 22, 2023

@zefman as a temporary fix til this is implemented see my commit above.

For me I had to modify the builded file (node_modules/dist/shared/ofetch.d438bb6f.mjs for me) and add those 3 lines from the commit above on line 196.
(Only for dev purpose, I hope this gets fixed quickly)

Note to my commit:
Since native fetch seems to deal just fine with Buffers, it might be better to only insure Buffer's don't get stringified and passing them on as they are instead of converting them toString.

As said if a pull request is wanted, I'm happy to do a little more testing.

@pi0
Copy link
Member

pi0 commented Aug 23, 2023

Hi. This is a tricky one to be fixed in ofetch layer. Please open an issue in either unjs/nitro or nuxt/nuxt repositories with reproduction 👍🏼

@pi0 pi0 closed this as not planned Won't fix, can't repro, duplicate, stale Aug 23, 2023
@wug-ge
Copy link
Author

wug-ge commented Aug 23, 2023

Hi @pi0 ,

thanks for replying, there is already an issue in nuxt/nuxt as linked above and also one in unjs/h3 with the last comment from yesterday confirming the issue.
I think that's probably enough, but I'll try to make a proper replication somewhere so it can be tested further together with a workaround for users facing the issue.

I thought I'll report the issue here since to me the issue lies in:
context.options.body = typeof context.options.body === "string" ? context.options.body : JSON.stringify(context.options.body);
which not only lacks support for Buffer's, but seemed a little unclean.

Either it's properly detecting different data types and trying to send them properly or not automatically converting them at all, forcing users to do JSON.stringify themselves.
I just tested it again and omitting this line solves my issue, but would be a breaking change for current users though.
Only executing this line if the body is NOT a Buffer would be the perfect solution I think, I can't think of a scenario someone wants to have a Buffer to be JSON stringified automatically.

This are just my assumptions as an outsider though, as said I'll do some testing and replication though to confirm the issue and get more details.

@pi0 pi0 reopened this Aug 23, 2023
@pi0 pi0 changed the title Automatically convert Buffer to string / Fix for many reported issues with routeRules proxy in nuxt with POST data Handle body types other than string and plain object Aug 23, 2023
@pi0 pi0 added bug Something isn't working enhancement New feature or request labels Aug 23, 2023
@pi0
Copy link
Member

pi0 commented Aug 23, 2023

Thanks for nice explanations. I have reopened with better title. I think it makes sense that we only do serialization for plain body objects 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants