[FR] Skipper overwrite existing files

[mdunisch]

If i use diskReceiver he overwrite existing files (if i upload a new file with a existing name).

To make this work out of the Box i think its nessary to add a routine looking of there is a file with the same name and write it as "file_01.jpg" for example

[CWyrtzen]

@mdunisch has your issue been resolved or do you have a specific Feature Request?

Contribution Guide | Stackoverflow | Google Group | Trello | IRC | Build Status

[mdunisch]

It's a feature-request for skipper-disk. By default no overwrite-checking is performed - but i think thats really necessary on default.

[mikermcneil]

@mdunisch somehow I managed to delete that long-ass comment -_- (I'm tethered on my phone at the moment). If you happened to get it in your email, would you repost? Otherwise I'll summarize it again

[mikermcneil]

basically comes down to either (1) use probabilistically-unique UUIDs or (2) do a stat for an existing file w/ the same name before every upload and append a probabilistically-unique UUID to the filename if one does

[mdunisch]

@mikermcneil out of my mails:

Mikermcneil:

@mdunisch well you've definitely identified something we've struggled to come up with a good API for-- the saveAs() function you can pass in as an option is designed to solve this problem, and the reason we don't do any checking is that it adds an extra stat call to every file upload request. You bring up a good point though.

I see two decent approaches we could take to alter the default behavior.

1) Stat the blobstore before every write and see if a file exists with the same filename as the incoming file. If so, append a unique identifier to the filename (before the extension) and write the file using that id.

2) Instead of using the actual filename, use a uuid or something similar to create a unique identifier for the uploaded file (i.e. how PHP does it with tmp uploads). This introduces an unfortunate complexity at scale/when used by multiple server instances at once since you want to ensure uniqueness. On the other hand, if a significantly improbable UUID generation strategy was used, this would probably be ok. Worst case, we could also just stat for the unique filename before writing, but then this is no better than the previous option.

What do you think? In either case, I think we could safely pull this conventional approach into Skipper core rather than implementing disparate strategies on a per-adapter basis. I think I'm partial to the latter option since it is faster, but the first option is compelling since it leaves obvious output in the adapter by default.

[mdunisch]

Thank you for looking at this.

1.) is the way i handle it in our Sails-Apps like this:

// save original file name
var origifile = req.file('testfile')._files[0].stream.filename;
var filename = origifile;

// check if file is existing and change filename if necessary
while(fs.existsSync(".tmp/uploads/" +filename)){
  // Add 4 random chars at he beginning of the filename
  filename = randString(3)+"_"+origifile; 
};

req.file('testfile').upload(filename,function (err, files) {
   ....

I think this check for existing files should be in the skipper-disk adapter. In some kinds of adapters it isn't necessary. For example we use a Adapter to upload images to ImageShack (Image Filehoster) by passing the File to a API. ImageShack handle the naming and gives us a URL back. With Imageshack-API for example it's impossible to check if a Filename exists. I can image other adapters (like file into mysql-blob) that are completely different for this check.

Why you think we should integrate this in the Skipper-Core? I like it that Skipper is very flexible for every kind of adapter.

Suggestion

I would prefer the following way (your 2nd):

• Integrate a var like 'tmp_name' into the Upstream Callbackvar (https://github.com/balderdashy/skipper/blob/master/lib/Upstream.js#L256-260) so the Developer have access to the tmp-name like in PHP (comes form the adapter)
• Also the 'filename' in the same file should be always the filename from the HTML-Form (should be passend from the adapter)
• For Sails-disk: We could use a UUIDv4 for the tmp-filename. So there is no problem with multiple server (v4 is a random number)

@mikermcneil: What you think about that? I have a bit of time this week and could try to implement this.

[mikermcneil]

@mikermcneil out of my mails:

thanks :)

What you think about that? I have a bit of time this week and could try to implement this.

That'd be awesome!

For Sails-disk

Btw- just to clarify, skipper-disk is distinct from sails-disk (this is another thing we've gone back and forth on over time and decided to keep separate for various reasons)

[mikermcneil]

Integrate a var like 'tmp_name' into the Upstream Callbackvar (https://github.com/balderdashy/skipper/blob/master/lib/Upstream.js#L256-260) so the Developer have access to the tmp-name like in PHP (comes form the adapter)

Sounds good- only thing I'd add is maybe to call it something like id for simplicity.

I think they way I'd probably recommend going about this feature, in general, is by including a default saveAs() method that Skipper passes down automatically to the receiver when you call .upload(). saveAs() is an adapter-level thing, and we still want it to be overridable, but implementing it that way would go a long way towards making sure we're eating our own dogfood, or whatever

If you need any help, feel free to hit me up! @robwormald in IRC has my skype, or just shoot me an email. Thanks!

[mdunisch]

@mikermcneil If have finished implementing this (with backward-compatibility) und mocha-test for skipper run without a error. I updated skipper (in a fork) and also skipper-disk. Should i send you all together as a ZIP or make 2 PRs? I worry that its difficulty to understand if you dont have both changes together.