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

pass meta to onBeforeUpload #79

Closed
menelike opened this issue May 19, 2016 · 14 comments
Closed

pass meta to onBeforeUpload #79

menelike opened this issue May 19, 2016 · 14 comments
Milestone

Comments

@menelike
Copy link
Contributor

menelike commented May 19, 2016

Sometimes I need to do validations based on user input, e.g. the directory name which the uploaded file should be referenced to. In those cases I need to check permissions on onBeforeUpload, but since the meta obj is not available there is no way to determine the permission.

How would this be solved with this lib? Any chance to add meta as additional arg to onBeforeUpload call?

Thanks in advance.

@dr-dimitru
Copy link
Member

Hi @menelike

  1. Have you checked function's context?
  2. It's about Server, Client or both?

@menelike
Copy link
Contributor Author

menelike commented May 19, 2016

Thanks for the quick response @dr-dimitru

I use this on the server only where context does not contain the meta object. If the meta data does not validate (based on collection references) I want to permit the upload.

@dr-dimitru dr-dimitru added this to the v.Next milestone May 19, 2016
dr-dimitru added a commit that referenced this issue May 19, 2016
@dr-dimitru
Copy link
Member

Implemented in dev branch (see: a265fcf). Could you test on your end?

@menelike
Copy link
Contributor Author

menelike commented May 19, 2016

The solution looks great, the implementation looks valid, now we can predict the document much better.

But apart of that, onBeforeUpload is fired twice (tested on master and dev) and to my surprise, only the second time the meta object exists.

Current master output:

=> Meteor server restarted
I20160519-23:40:47.735(2)? { size: 2308,
I20160519-23:40:47.738(2)?   type: 'text/csv',
I20160519-23:40:47.739(2)?   name: 'foo.csv',
I20160519-23:40:47.739(2)?   ext: 'csv',
I20160519-23:40:47.739(2)?   extension: 'csv',
I20160519-23:40:47.740(2)?   extensionWithDot: '.csv',
I20160519-23:40:47.740(2)?   mime: 'text/csv',
I20160519-23:40:47.740(2)?   'mime-type': 'text/csv' }
I20160519-23:40:47.775(2)? { size: 2308,
I20160519-23:40:47.776(2)?   type: 'text/csv',
I20160519-23:40:47.776(2)?   name: 'foo.csv',
I20160519-23:40:47.777(2)?   ext: 'csv',
I20160519-23:40:47.777(2)?   extension: 'csv',
I20160519-23:40:47.777(2)?   extensionWithDot: '.csv',
I20160519-23:40:47.778(2)?   mime: 'text/csv',
I20160519-23:40:47.778(2)?   'mime-type': 'text/csv' }

Current dev output:

ostrio:files  upgraded from 1.5.3 to 1.5.4

=> Meteor server restarted
I20160519-23:43:58.797(2)? { name: 'foo.csv',
I20160519-23:43:58.804(2)?   extension: 'csv',
I20160519-23:43:58.805(2)?   path: 'assets/app/uploads/FooFiles/WcdLuRfcWEkrKCxWg.csv',
I20160519-23:43:58.805(2)?   meta: {},
I20160519-23:43:58.808(2)?   type: 'text/csv',
I20160519-23:43:58.808(2)?   size: 2308,
I20160519-23:43:58.808(2)?   versions: 
I20160519-23:43:58.808(2)?    { original: 
I20160519-23:43:58.808(2)?       { path: 'assets/app/uploads/FooFiles/WcdLuRfcWEkrKCxWg.csv',
I20160519-23:43:58.809(2)?         size: 2308,
I20160519-23:43:58.809(2)?         type: 'text/csv',
I20160519-23:43:58.809(2)?         extension: 'csv' } },
I20160519-23:43:58.809(2)?   isVideo: false,
I20160519-23:43:58.809(2)?   isAudio: false,
I20160519-23:43:58.809(2)?   isImage: false,
I20160519-23:43:58.810(2)?   isText: true,
I20160519-23:43:58.810(2)?   isJSON: false,
I20160519-23:43:58.810(2)?   _prefix: '0dcd4313d1d69716d073070e28230beb5fbaa875a08b2d4436844bed5283d3f2',
I20160519-23:43:58.810(2)?   _storagePath: 'assets/app/uploads/FooFiles',
I20160519-23:43:58.810(2)?   _downloadRoute: '/cdn/storage',
I20160519-23:43:58.811(2)?   _collectionName: 'FooFiles',
I20160519-23:43:58.811(2)?   _id: 'WcdLuRfcWEkrKCxWg',
I20160519-23:43:58.811(2)?   userId: 'kSc4u7h5a3fjcfukw' }
I20160519-23:43:58.835(2)? { name: 'foo.csv',
I20160519-23:43:58.836(2)?   extension: 'csv',
I20160519-23:43:58.836(2)?   path: 'assets/app/uploads/FooFiles/WcdLuRfcWEkrKCxWg.csv',
I20160519-23:43:58.837(2)?   meta: { fooId: 'pB9fYnRKkhuBEeCP8', username: 'foo' },
I20160519-23:43:58.837(2)?   type: 'text/csv',
I20160519-23:43:58.838(2)?   size: 2308,
I20160519-23:43:58.838(2)?   versions: 
I20160519-23:43:58.838(2)?    { original: 
I20160519-23:43:58.838(2)?       { path: 'assets/app/uploads/FooFiles/WcdLuRfcWEkrKCxWg.csv',
I20160519-23:43:58.839(2)?         size: 2308,
I20160519-23:43:58.839(2)?         type: 'text/csv',
I20160519-23:43:58.839(2)?         extension: 'csv' } },
I20160519-23:43:58.839(2)?   isVideo: false,
I20160519-23:43:58.839(2)?   isAudio: false,
I20160519-23:43:58.839(2)?   isImage: false,
I20160519-23:43:58.840(2)?   isText: true,
I20160519-23:43:58.840(2)?   isJSON: false,
I20160519-23:43:58.840(2)?   _prefix: '0dcd4313d1d69716d073070e28230beb5fbaa875a08b2d4436844bed5283d3f2',
I20160519-23:43:58.840(2)?   _storagePath: 'assets/app/uploads/FooFiles',
I20160519-23:43:58.841(2)?   _downloadRoute: '/cdn/storage',
I20160519-23:43:58.841(2)?   _collectionName: 'FooFiles',
I20160519-23:43:58.841(2)?   _id: 'WcdLuRfcWEkrKCxWg',
I20160519-23:43:58.842(2)?   userId: 'kSc4u7h5a3fjcfukw' }

I'm not sure if this is a bug from my higher logic. From my point of view this should fire only once. Can you reproduce this?

If a new issue is needed here let me know.

@dr-dimitru
Copy link
Member

This is expected behaviour, form the docs:

Callback, triggered right before upload is started on client and right after receiving a chunk on server

So, on server it will be triggered as many chunks it received.

only the second time the meta object exists.

This is also expected behaviour, as metadata sent only after last chunk of the file is received. We can pass metadata along with each chunk, but this will cause extra bytes sent over the wire - this is not what we want.

@menelike
Copy link
Contributor Author

menelike commented May 20, 2016

@dr-dimitru sorry for that, totally missed that in the docs. I agree, passing metadata along every chunk isn't the choice in terms of performance.
Is it possible to transfer metadata on the initial request? I need to abort the transfer directly, not after all chunks have been transferred but before. But again, from a developers perspective a single validation run on start would be the best. Maybe onBeforeUpload does not fit here at all. If metadata is missing on all times > 1 the validation becomes inconsistent. On the first run it returns true because it validates true, on every run after it validates false as the metadata got lost.
A function like onInitialUpload (server only) which is fired only once would fit well in this scenario.

@dr-dimitru
Copy link
Member

dr-dimitru commented May 20, 2016

@menelike unfortunately not as we currently can't define "first chunk" as any of them may be first - upload is asynchronous.


Please see: 5e77d6a
Now meta is sent with chunk labeled as #1, but after running some tests - I can tell you what in 30% this chunk is not coming first to server, but 100% between chunks 4*streams.

@menelike
Copy link
Contributor Author

Understood. Hard to solve. I'll try to patch the source and let you know my results within this day. Thanks for your great support!

@dr-dimitru
Copy link
Member

@menelike have a good coding! Let me know your results then, maybe we can workaround this somehow, or improve the package.

@menelike
Copy link
Contributor Author

menelike commented May 20, 2016

@dr-dimitru

This is a though one. I exposed Meteor.Collection to subclass insert and invalidate the DB insert.
To be honest, exposing the Collection might be a great way to transform the doc, but this is not the proper way to add validation logic (for example the userId isn't reachable and could be only derived from the doc).

Therefore I'm not sure if a PR is welcome. What do you think?

https://github.com/risetechnologies/Meteor-Files/commit/8296de3df67b35953a3d12c439ddb83b10f403a3 adds CollectionClass as a parameter to FilesCollection.

A simple example to get the idea:

class FooFilesCollection extends Mongo.Collection {
  insert(doc, callback) {
    const newDoc = doc;
    ... // set invalid
     if (invalid) {
      FooFiles.unlink(doc); // this is dirty *circular dependency*
      return callback(new Meteor.Error('fooFiles.insert', 'fooError'));
    }
    return super.insert(newDoc, callback);
  }
}

export const FooFiles = new FilesCollection({
  collectionName: 'FooFiles',
  CollectionClass: FooFilesCollection,
});

As a result, data is always transferred over the wire. If the insert throws an error, the doc won't be saved to the database but isn't deleted from the storage, you would need to manually call unlink too.

Without huge additional efforts I do not see how this could be easily solved. The easiest way would be indeed to add the metadata which might impact performance. a265fcf adds more data than needed, but since I do not see any reliable usability I'd revert that commit.

Maybe a flag which controls that meta is always(true)/never(false) accessible from onBeforeUpload would be the simplest and best solution.

@dr-dimitru
Copy link
Member

I believe receiving and checking chunk labeled as #1 (and the last one of course) is enough to validate current upload.

Your suggested approach make it more complex, while I'm trying to keep this package with a little moving parts as possible to leave less weak spots which can be broken.

I agree with add an ability to pass your own Mongo.Collection instance.

Waiting for your point.

@menelike
Copy link
Contributor Author

menelike commented May 21, 2016

@dr-dimitru I fully agree with your approach.

Your implementation resulted in always sending the meta data. In addition the chunkId needs to be accessible on the receiving server part. See PR.

@dr-dimitru
Copy link
Member

chunkId is always transferred between Client and Server back and forward, so we can show percentage and catch EOF

@dr-dimitru
Copy link
Member

Done in v1.5.4
Thank you @menelike

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants