Missing path on Readable stream

[michalkvasnicak]

I tried to use capacitor stream directly with form-data but there is no path property set on resulting stream causing form-data to fail generating multipart/form-data request.

https://nodejs.org/api/fs.html#fs_readstream_path

const FormData = require('form-data');
const { createReadStream } = require('fs');
const { WriteStream } = require('fs-capacitor');
const { resolve } = require('path');

const data1 = new FormData();
const data2 = new FormData();
const capacitor = new WriteStream();

const fsStream = createReadStream(resolve(__dirname, './testhandler.js'));
const capacitorStream = capacitor.createReadStream();

fsStream.pipe(capacitor);

data1.append(0, fsStream, 'testhandler.js');
data1.on('end', () => {
  capacitor.destroy();
});

data1.getLength((err, len) => {
  console.log('length 1', len);
});

data2.append(0, capacitorStream, 'testhandler.js');

data2.getLength((err, len) => {
  console.log('length 2', len);
});

Data 1 returns length of 254 which is correct, data 2 returns 221.

So if the stream should be compatible with file streams, it should set path property.

https://github.com/form-data/form-data/blob/master/lib/form_data.js#L104

[mike-marcacci]

Hi @michalkvasnicak thanks for opening this. I'm definitely not opposed to exposing the path property. However, looking through that library to determine what it needs the path for, it's clear that it makes the assumption that the file is in an unchanging state on the filesystem, and at a human-named location.

Given these assumptions, I think that exposing the real file path would actually have undesirable effects. In my opinion, the form-data library should support appending of arbitrary streams (instead of special cases for fs and http) and allow manual specification of metadata like mimetype and size.

What kind of problem are you trying to solve with this, if you don't mind me asking?

[michalkvasnicak]

@mike-marcacci yes you're right the should support arbitrary streams.

What I'm trying to do is following:

• We have central GraphQL service that stitches schemas from multiple GraphQL microservices.
• Some microservices have file upload capabilities but GraphQL stitching doesn't support them out of the box.
• I created an upload link that basically detects files in an operation that will be send to the microservice and then if there are files, it creates multipart/form-data request that is send to the microservice.

For now I'm fixing that by providing just a filename to stream.path so it passes through form-data without any problems.

[gregory]

I have been hunting a bug where i'm trying to proxy a file upload.
The file is uploaded first through a graphql endpoint by using this lib graphql-upload and then uploaded to another endpoint using form-data.

That being said, it seems like you may have pin pointed the issue @mike-marcacci -

In my case, i noticed that value.path was set to an empty string - note that they are using fs-capacitor@^2.0.4.

[gregory]

So after a little more digging, i found that there is a race condition in graphql-upload that is sometimes resolving before crypto.randomBytes or fs.open resolves.
This could lead you to call file.createReadStream before the writestream got a chance to set the path, so this.path = this._writeStream.path will resolve to empty string

I patched thecode to be like this instead:

      capacitor.once('ready', () => upload.resolve(file))

Though there was another issue when used in combination with form-data.
Check out this PR jaydenseric/graphql-upload/#168

[mike-marcacci]

@gregory that is... very interesting to me. Is this something you can demonstrate? It should be safe to begin writing even before it's ready, since we wait internally:

fs-capacitor/src/index.ts

Lines 164 to 172 in a237676

	_write(
	chunk: Buffer,
	encoding: string,
	callback: (error?: null | Error) => any
	): void {
	if (typeof this._fd !== "number") {
	this.once("ready", () => this._write(chunk, encoding, callback));
	return;
	}

If you've found a way to defeat this, I'd love to include it in the tests, as the timing and sequence of things in streams can vary across versions of node.

[mike-marcacci]

OK, I believe I understand what you're getting at (and talked about it here). If that is incorrect, do let me know.

From what I've been able to gather from all these issues is that basically the form-data library doesn't have support for arbitrary streams, and uses duck typing to support metadata extraction from the standard fs and http libraries.

In a sense it's really fortunate that our .path was initially null, as it prevented two critical false-assumptions that would have been made. In part to address this, the (undocumented) .path property has been renamed to ._path to make it more clear that this is just an implementation detail.

I found the corresponding issue in the form-data repo, and I've added a comment describing the scenario and steps they can take (add general support for streams). I'm going to close this here, as this is an issue with that library, but will stay subscribed to that issue, in case further questions arise.

[gregory]

@gregory that is... very interesting to me. Is this something you can demonstrate? It should be safe to begin writing even before it's ready, since we wait internally:

fs-capacitor/src/index.ts

Lines 164 to 172 in a237676

	_write(
	chunk: Buffer,
	encoding: string,
	callback: (error?: null | Error) => any
	): void {
	if (typeof this._fd !== "number") {
	this.once("ready", () => this._write(chunk, encoding, callback));
	return;
	}

If you've found a way to defeat this, I'd love to include it in the tests, as the timing and sequence of things in streams can vary across versions of node.

I was on the v2.0.4 but you are right as far as the race condition.
The issue with form-data is still standing so let me know if you want me to update my PR.