oven-sh / bun

Incredibly fast JavaScript runtime, bundler, test runner, and package manager – all in one
https://bun.sh
Other
73.17k stars 2.68k forks source link

Get uploaded file using @fastify/multipart not working #5265

Open gabrielrra opened 1 year ago

gabrielrra commented 1 year ago

What version of Bun is running?

1.0.1+31aec4ebe325982fc0ef27498984b0ad9969162b

What platform is your computer?

Linux 6.2.0-76060200-generic x86_64 x86_64

What steps can reproduce the bug?

By making a simple javascript server using fastify and @fastify/multipart I cannot retrieve the uploaded file if I run it with bun, whereas if I run it with node the code works just fine.

  1. Create a new bun/node project:
bun init
# package name (test-bun-upload):
# entry point (index.ts): index.js
  1. Add fastify and @fastify/multipart
bun i fastify @fastify/multipart
  1. Change index.js to be as follows:
import { fastify } from 'fastify';
import { fastifyMultipart } from '@fastify/multipart';

const app = fastify();

app.register(fastifyMultipart);

app.post('/upload', async (req, reply) => {
  const data = await req.file();

  if (!data) return reply.status(400).send({ error: 'No file found' });
  return reply.send('File found!');
});

app
  .listen({
    port: 3000,
    host: '0.0.0.0',
  })
  .then((server) => console.log(`Server is running on ${server}`));
  1. Run the server with bun: bun run index.js

  2. Send any file below 1Mb to endoint http://localhost:3000/upload. The response should be 400 with body { "error": "No file found" }

  3. Close the server and run it again using node: node index.js

  4. Send any file below 1Mb to endoint http://localhost:3000/upload. The response should be 200 with message "File found!"

What is the expected behavior?

I should receive a 200 response with the message "File found!".

What do you see instead?

I receive a 400 response with the message { "error": "No file found" }.

This only occurs if req.file() returns undefined, meaning the file sent from the REST client didn't make it to the server.

Additional information

Tested using Node v18.15.0

Erb3 commented 10 months ago

I am able to reproduce this and have yet to find a workaround. Using Node v18.18.2 it returns File found!, while on Bun 1.0.11 it returns {"error": "No file found"}.

cURL command I used to test:


curl --request POST \
  --url http://localhost:3000/upload \
  --header 'Content-Type: multipart/form-data' \
  --form 'file=@Path\To\File'```
PatriikPlays commented 9 months ago

Same behavior with Node v21.4.0, Bun 1.0.15 on Linux 6.1.65-1-lts x86_64 x86_64.

sean256 commented 9 months ago

I'm not an expert with streams, but I did find the issue to be somewhere in the busboy dependency. Likely due to some small difference with how the bun stream API differs.

https://github.com/fastify/busboy/issues/139

michaelfromyeg commented 7 months ago

I just ran into this as well. I think the busboy dependency that is problematic is Dicer which does the byte processing (...though it may be one of its dependencies that is problematic as well). Here's a test file I ran.

import fs from "fs/promises";
import { Readable } from "stream";

// grab an example from dicer's test suite as appropriate
const boundary = "------------------------c26e1ad6378c0515";
const buffer = await fs.readFile("./multipart.bin");

const dicer = new Dicer({ boundary: `${boundary}` });

dicer.on("part", (part) => {
  console.log("a part");

  part.on("header", (header) => {
    console.log("a header");
  });

  part.on("data", (data) => {
    console.log("some data");
  });

  part.on("end", () => {
    console.log("End of part\n");
  });
});

dicer.on("finish", () => {
  console.log("All parts processed");
});

dicer.on("error", (error) => {
  console.error("Dicer error:", error);
});

const bufferStream = new Readable({
  read() {},
});
bufferStream.push(buffer);
bufferStream.push(null);
bufferStream.pipe(dicer);

bufferStream.on("error", (error) => {
  console.error("Stream error:", error);
});

And the outputs:

$ node test.js
a part
a header
a part
a header
some data
some data
End of part

End of part

All parts processed

$ bun test.js
All parts processed

Going to see if there's something specific within Dicer that may be at fault.

michaelfromyeg commented 7 months ago

I think dicer.on("part", ...) isn't being "registered" correctly. If in onInfo I modify

    ev = this._isPreamble ? 'preamble' : 'part';
    console.log(`Emitting ${ev} event`);

    if (true || this._events[ev]) { // force this to be true
      this.emit(ev, this._part);
    } else {
      ignore(this);
    }

the code works as expected. So... maybe something funky with the implementation of on(event: string | symbol, listener: (...args: any[]) => void): this; in WriteableBase?

edit: or even better,

if (this.listenerCount(ev) > 0)) {
 // ...
}

no hack needed!

michaelfromyeg commented 7 months ago

Last comment... here's a work around for those interested to get in working in the context of @fastify/multipart.

  1. Install your dependencies
  2. In node_modules/@fastify/busboy/deps/dicer/lib/Dicer.js, change else if (this._isPreamble !== true && this._events.part) to else if (this._isPreamble !== true && this.listenerCount("part") > 0)
  3. In node_modules/@fastify/busboy/lib/types/multipart.js, change if (!boy._events.file) to if (!boy.listenerCount("file"))
  4. Ensure fastify.register(fastifyMultipart) is called; addFieldsToBody hasn't been tested, so ensure that's false
  5. Verify your endpoint works, e.g.,

server.post(
    "/upload",
    async (request, reply) => {
      const data = await request.file();
      if (!data) {
        return reply.status(400).send({ error: "No file provided" });
      }

      const filename = data.filename;
      const saveTo = path.join(__dirname, "uploads", filename);

      await data
        .toBuffer()
        .then(async (buffer) => {
          await fs.writeFile(saveTo, buffer);
        })
        .catch((err) => {
          reply.send(err);
        });

      reply.send({ uploaded: true, filename });
    },
  );```
shri3k commented 6 months ago

@michaelfromyeg god damn, that actually works. Tested on v1.0.28 and works on attachedFieldToBody: true with/without onFile. Wish we can get this patched upstream though. Either way, Kudos on the workaround. 👏

tousifhabib commented 6 months ago

Can we get a fix on this please? I am also using bun and unable to use fastify multipart due to this issue.

Uzlopak commented 6 months ago

Every body talking about upstreaming this, but nobody is opening a PR in @fastify/busboy...

Uzlopak commented 6 months ago

proposed https://github.com/fastify/busboy/pull/146

michaelfromyeg commented 6 months ago

FWIW I think this isn't an upstream issue (with Busboy and Dicer), but rather an issue with Bun. Something subtle with how it's handling events. Your proposed PR still doesn't resolve the Dicer line I change above (i.e., step #1).

It also seems like bad practice to change the upstream deps if it's actually an issue with Bun to begin with.

edit: @Uzlopak it looks like you noted it's Bun related! https://github.com/fastify/busboy/issues/139#issuecomment-1853495843

Jarred-Sumner commented 6 months ago

Yes this is an issue with Bun and not in Busboy. I'm wondering if somewhere/somewhere it's using our (internally-deprecated) C++ EventEmitter implementation instead of the JS implementation which should support these getters. We only should be using the C++ implementation for the process global.

Uzlopak commented 6 months ago

@michaelfromyeg

I guess the issue is, that _events is an internal undocumented attribute. So maybe thats why it does not work and you are proposing the use of listenerCount. Also my PR contains all the changes you proposed for the workaround and other places, where we use _events attribute.

So if we can solve this by using the documented functions, I am totally fine with doing that. Also using the documented attributes etc.. avoids us getting into issues, like express ran into, because it uses alot of undocumented internals.

Uzlopak commented 6 months ago

We patched it now upstream in busboy. As we are not providing lock files you should be able to update the dependency. Looking forward for your feedback

tousifhabib commented 6 months ago

@Uzlopak thanks a lot, I will try it shortly

brndt commented 6 months ago

We patched it now upstream in busboy. As we are not providing lock files you should be able to update the dependency. Looking forward for your feedback

Didn't work for me with attachFieldsToBody: 'keyValues'

Uzlopak commented 6 months ago

Can you provide a mvce?

brndt commented 6 months ago

Can you provide a mvce?

https://github.com/brndt/fastify-bun-multipart/tree/main

I've made console.log of body

Uzlopak commented 6 months ago

it seems the code is not even with nodejs valid. On nodejs I also get an empty buffer. I am currently not focussing on fastify/multipart, so I wont be deepdiving into it rn.

brndt commented 6 months ago

It's working for node:

Screenshot 2024-03-04 at 00 15 22
Uzlopak commented 6 months ago

Probably postman is not working properly on my machine. I hate to install postman via snap.

github-actions[bot] commented 1 month ago

This issue is stale and may be closed due to inactivity. If you're still running into this, please leave a comment.