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

Invalid calculation of file size when uploading multiple files under the same stream/name #4838

Closed
vbogdanov opened this issue Oct 20, 2015 · 9 comments

Comments

@vbogdanov
Copy link

HTML:

<form action="/upload-file" method="POST" enctype="multipart/form-data">
    <label for="file1">Upload file here:</label><input type="file" name="file" id="file1"><br />
    <label for="file2">Upload file here:</label><input type="file" name="file" id="file2"><br />
    <label for="file3">Upload file here:</label><input type="file" name="file" id="file3">
    <input type="submit" value="Upload" />
</form>

Express Route:

router.post('/upload-file', function (req, res, next) {
    req.file('file').upload({
        dirname: path.join(__dirname, '..', 'uploaded'),
        saveAs:  function (__newFileStream,cb) {
            cb(null, __newFileStream.filename);
        },
        maxBytes: 1024*1024*1024*1024
    }, function (err, files) {
        if (err) {
            console.error(err, err.stack);
        }
        console.log(files);
        res.json(files).end();
    });
});

Uploading 3 files, sizes 5844, 197, 710494 bytes, expected sizes in files - 5844, 197, 710494
actual:

[
  {
    "fd": "<proper path here>",
    "size": 716835,
    "type": "image/jpeg",
    "filename": "<proper value here>",
    "status": "bufferingOrWriting",
    "field": "file"
  },
  {
    "fd": "<proper path here>",
    "size": 710841,
    "type": "text/plain",
    "filename": "<proper value here>",
    "status": "bufferingOrWriting",
    "field": "file"
  },
  {
    "fd": "<proper path here>",
    "size": 710494,
    "type": "image/jpeg",
    "filename": "<proper value here>",
    "status": "bufferingOrWriting",
    "field": "file"
  }
]

After a short look at the code my best guess is that the remaining length of the stream is used as file size. This is correct for single files but fails for multiple files as the size of the first equals the size of all files plus the size of the metadata in between. (Again, this is just a guess)

@sailsbot
Copy link

Thanks for posting, @vbogdanov. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@tjwebb tjwebb reopened this Nov 20, 2015
@mikermcneil
Copy link
Member

After a short look at the code my best guess is that the remaining length of the stream is used as file size. This is correct for single files but fails for multiple files as the size of the first equals the size of all files plus the size of the metadata in between. (Again, this is just a guess)

@vbogdanov Hmm, yeah we're just tallying up bytes as chunks arrive (I don't think there's a way we can figure out the remaining length of the stream-- that'd be easier. But something funny does indeed seem to be going on. Would you mind posting your versions of Skipper/Express/Node/NPM for reference?

@vbogdanov
Copy link
Author

Reproducible with
skipper 0.5.8
express 4.13.3
node 4.2.4
npm 2.14.12

@crobinson42
Copy link

@vbogdanov any progress on this?

@crobinson42
Copy link

crobinson42 commented Apr 25, 2016

I'm experiencing the same issue when uploading multiple files under the same key. On the client I use;

fileBlob1 size is 10000 bytes
fileBlob2 size is 20000 bytes

var formData = new FormData();
formData.append('fileKey', fileBlob1);
formData.append('fileKey', fileBlob2);
// then post with $.ajax({..});

backend:

console.log(req.file('fileKey')['_files'].map(file=>{return file.stream.byteCount}));
// returns [ 30000, 20000 ]

The first file size is the aggregate size of all files but every file after that is correct.

node v4.4.0
sails v0.12.1
skipper v0.5.9

@crobinson42
Copy link

This issue is related to the dependency node-multiparty (https://github.com/andrewrk/node-multiparty) and a similar issue pillarjs/multiparty#70.

This issue has to do with the req.headers['content-length']

I'll continue to investigate a solution..

@crobinson42
Copy link

A consistent workaround to get the correct file size:

files = uploadedFiles.map( (file,index) => {
  // if it's the first file, read the actual file size
  if (index === 0) {
    file.size = require('fs').statSync(file.fd).size
  }
  return file;
});

@mikermcneil
Copy link
Member

@crobinson42 thanks for the workaround! Obviously that won't work with other adapters, but it'll help a lot of folks out in the mean time. @sgress454 or I will take a closer look at this as soon as we can.

@mikermcneil
Copy link
Member

@vbogdanov @NAlexandrov @crobinson42 This is now fixed (to the best of our knowledge) in the latest skipper-disk and skipper-s3 on NPM (cc @sgress454)

@johnabrams7 johnabrams7 transferred this issue from sailshq/skipper Apr 29, 2019
@balderdashy balderdashy deleted a comment from sailsbot Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants