Skip to content

Added maxBytes support.#45

Open
dmedina2015 wants to merge 4 commits into
willhuang85:masterfrom
dmedina2015:feature/maxBytes
Open

Added maxBytes support.#45
dmedina2015 wants to merge 4 commits into
willhuang85:masterfrom
dmedina2015:feature/maxBytes

Conversation

@dmedina2015
Copy link
Copy Markdown

@dmedina2015 dmedina2015 commented Mar 10, 2021

(1) Added support to maxBytes option, using similar logic from skipper-disk. Behavior:

  • An error is thrown when upload stream exceeds bytes defined in maxBytes parameter.
  • Limit is applied for all upload stream (all files in the same request, not for each file individually)
  • Files uploaded before limit is reached are saved in GridFS. Only the one that exceeds and the subsequent are not saved.
  • Garbage of unfinished upload is removed using GridFSBucketWriteStream.abort() function

(2) Added support to onProgress using same logic from skipper-disk

@mikermcneil or @willhuang85 , could you review it?

@willhuang85
Copy link
Copy Markdown
Owner

Hey @dmedina2015, awesome work! I’ll try to review your PRs over the weekend as I haven’t really had time to work with SailsJS in awhile.

@dmedina2015
Copy link
Copy Markdown
Author

dmedina2015 commented Mar 11, 2021

Hey @willhuang85, thank you very much for your message. I'm happy to help the project. Below are some attention points for your analysis:

(1) I commented the listeners __newFile.once('close') and __newFile.once('done') since they were being called together with its outs__ correspondents. As both emits events in receiver__, receiver__.once('error') was being called twice every time an error (like E_EXCEEDS_UPLOAD_LIMIT for instance).

(2) I used the exact same logic of skipper-disk for the onProgress feature. However, I'm a little bit concerned about this piece of code:

module.exports = function buildProgressStream (options, __newFile, receiver__, outs__) {
    options = options || {};
    var log = options.log || function noOpLog(){};
 
    // Generate a progress stream and unique id for this file
    // then pipe the bytes down to the outs___ stream
    // We will pipe the incoming file stream to this, which will
    var localID = _.uniqueId();
    var guessedTotal = 0;
    var writtenSoFar = 0;
    var __progress__ = new TransformStream();
    __progress__._transform = function(chunk, enctype, next) {

        // Update the guessedTotal to make % estimate
        // more accurate:
        guessedTotal += chunk.length;
        writtenSoFar += chunk.length;

I cannot understand why guessedTotal is initialized with 0 and added by chunk.length every time a new chunk arrives. This is causing guessedTotal and writtenSoFar to be exactly equal every time and the percent is always 100%.
My understanding is that it must be initialized with the total size of the stream: guessedTotal=__newfile.stream.byteCount and not be added by chunk.length() as today. If you are ok with my logic, I can update my branch.

(3) I strongly recommend you to take a look at PR #44 . It fix a bug identified in issue #39 and I think it is important to get in master due to a callback being called twice.

Greetings from Brazil

@dmedina2015 dmedina2015 reopened this Mar 11, 2021
- Adds support for Node14, by doing the following:
  * Update MongoDB Node Driver to latest (3.6.5)
  * Removed unecessary event emitters from adapter.receive()
  * adapter.receive() only calls 'done' callback when 'outs__' stream finishes or rise an error. Previous code was calling callback before finishing the file writing. This was causing some async problems and not passing skipper-adapter-test official tests.
  * Tests are now using official adapter by sailshq. No tweaks are needed to pass the build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants